On Mon, Apr 12, 2021 at 11:01:59PM +0300, Zidenberg, Tsahi wrote:
>
>
> 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.
So you are just adding new things, not changing existing things.
There's nothing wrong with adding new userspace apis, nothing is
"frozen" in that existing userspace would suddenly be broken here,
right?
> 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.
Yes you need to keep the values identical, that's fine, and expected, we
do that all the time in stable kernels.
thanks,
greg k-h
