Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

["Zidenberg, Tsahi" <tsahee@amazon.com>]

On 10/04/2021 14:29, Greg KH wrote:
> On Sun, Apr 04, 2021 at 12:13:46PM +0300, Zidenberg, Tsahi wrote:
>> On 03/04/2021 12:56, Greg KH wrote:
>>>
>>> Again that would make things different from Linus's tree, which is what
>>> we want to avoid if at all possible.
>>>
>>> What commits in 5.8 are you talking about here, if the changes are there
>>> that work here in 5.4, that's fine.
>> In 5.5 (mostly 6ae08ae3dea2) BPF UAPI was changed, bpf_probe_read was split
>> into compat (original), user and kernel variants. Compat here just calls the
>> kernel variant, which means it's still broken.
> That's not a UAPI change, that does not change the userspace-visable
> part, right?  Did something change?
>
>> In 5.8 (8d92db5c04d10), compat was fixed to call user/kernel variants
>> according to address in machines where it makes sense, and disabled on other
>> machines. I am trying to take the fix for machines where it's possible, and
>> leave other machines untouched.
>>
>> As I understand it, there are 3 options:
>> 1)  Implement the same fix as v5.8, while staying with v5.4 code/API.
>>     That's what my original patch did.
>> 2)  Import the new 5.5 API + 5.8 fix. It's not trivial to get API-compatibility
>>     right. Specifically - need to solve skb_output (a7658e1a4164c), another
>>     entry in the BPF enum, introduced before the new read variants.
> What "API-compatibility" are you trying for here?  There is no issues
> with in-kernel changes of apis.
>
> What commits exactly does this require?  It is almost always better to
> keep the same identical patches that are in newer kernels to be
> backported than to do something different like you are doing in 1).
> That way any future changes/fixes can easily also be backported.
> Otherwise it gets harder and harder over time.
This is how I understand it, please correct me if/where I'm wrong:

include/uapi/linux/bpf.h is part of the UAPI. Specifically, bpf_func_id
enum is part of the UAPI. This enum matches function I.D to bpf helper,
and the indexes are kept constant across kernel versions.

Kernel 5.5 added skb_output helper (irrelevant to our fix) to that enum,
and then added probe_read_{user,kernel}* functions on top of that. Taking
probe_read_{user,kernel}* from commit 6ae08ae3dea2 itself is changing
UAPI. The mainline fix in 5.8 (8d92db5c04d10) depends on that UAPI change.

Appending another function is not a terrrible UAPI change, but to
keep these functions at the same index as later kernel versions - we'd
also need to either take skb_output or add a replacement.


Thank you!
Tsahi.