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

From: Greg KH <gregkh@linuxfoundation.org>
To: "Zidenberg, Tsahi" <tsahee@amazon.com>
Cc: Sasha Levin <sashal@kernel.org>, stable@vger.kernel.org
Subject: Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()
Date: Sat, 10 Apr 2021 13:29:07 +0200	[thread overview]
Message-ID: <YHGMAxjfn1fKfgGE@kroah.com> (raw)
In-Reply-To: <655b6f67-787f-80dd-975e-3d1ab87b0c49@amazon.com>

On Sun, Apr 04, 2021 at 12:13:46PM +0300, Zidenberg, Tsahi wrote:
> 
> 
> On 03/04/2021 12:56, Greg KH wrote:
> > On Wed, Mar 31, 2021 at 09:37:28PM +0300, Zidenberg, Tsahi wrote:
> >> On 30/03/2021 20:21, Sasha Levin wrote:
> >>
> >>> What stops us from taking that API back to 5.4? I took a look at the
> >>> dependencies and they don't look too scary.
> >>>
> >>> Can we try that route instead? We really don't want to diverge from
> >>> upstream that much.
> >>>
> >> probe_read_{user,kernel}* functions themselves seem simple enough.
> >> Importing them in a forward-compliant way to 5.4 would require either
> >> adding an unused entry in bpf.h's __BPF_FUNC_MAPPER or also pulling
> >> skb_output bpf helper functions into 5.4. To me, it seems like too
> >> much of a UAPI change to go into a stable release.
> > Why is anything changing in the user api here?  The user api should not
> > be changing in incompatible ways, otherwise you would not be able to
> > upgrade to new kernels without breaking things.
> >
> >> Another option would be to import more code to make it somewhat closer
> >> to upstream implementation without changing UAPI. As in v5.8, I could
> >> internally map these helpers to probe_read_compat* functions, which
> >> will in turn call probe_read_{user,kernel}*_common functions.
> >> Func names might seem weird out of context, but it will be closer.
> >> Implementation will still defer, e.g. to avoid warnings on platforms
> >> without ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> >>
> >> Does that sound like a reasonable solution?
> > 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.

thanks,

greg k-h