Re: [PATCH bpf] bpf: relax zero fixed offset constraint on KF_TRUSTED_ARGS/KF_RCU

[Matt Bobrowski <mattbobrowski@google.com>]

On Tue, Jul 09, 2024 at 11:23:34PM +0200, Kumar Kartikeya Dwivedi wrote:
> On Tue, 9 Jul 2024 at 23:09, Matt Bobrowski <mattbobrowski@google.com> wrote:
> >
> > Currently, BPF kfuncs which accept trusted pointer arguments
> > i.e. those flagged as KF_TRUSTED_ARGS, KF_RCU, or KF_RELEASE, all
> > require an original/unmodified trusted pointer argument to be supplied
> > to them. By original/unmodified, it means that the backing register
> > holding the trusted pointer argument that is to be supplied to the BPF
> > kfunc must have its fixed offset set to zero, or else the BPF verifier
> > will outright reject the BPF program load. However, this zero fixed
> > offset constraint that is currently enforced by the BPF verifier onto
> > BPF kfuncs specifically flagged to accept KF_TRUSTED_ARGS or KF_RCU
> > trusted pointer arguments is rather unnecessary, and can limit their
> > usability in practice. Specifically, it completely eliminates the
> > possibility of constructing a derived trusted pointer from an original
> > trusted pointer. To put it simply, a derived pointer is a pointer
> > which points to one of the nested member fields of the object being
> > pointed to by the original trusted pointer.
> >
> > This patch relaxes the zero fixed offset constraint that is enforced
> > upon BPF kfuncs which specifically accept KF_TRUSTED_ARGS, or KF_RCU
> > arguments. Although, the zero fixed offset constraint technically also
> > applies to BPF kfuncs accepting KF_RELEASE arguments, relaxing this
> > constraint for such BPF kfuncs has subtle and unwanted
> > side-effects. This was discovered by experimenting a little further
> > with an initial version of this patch series [0]. The primary issue
> > with relaxing the zero fixed offset constraint on BPF kfuncs accepting
> > KF_RELEASE arguments is that it'd would open up the opportunity for
> > BPF programs to supply both trusted pointers and derived trusted
> > pointers to them. For KF_RELEASE BPF kfuncs specifically, this could
> > be problematic as resources associated with the backing pointer could
> > be released by the backing BPF kfunc and cause instabilities for the
> > rest of the kernel.
> >
> > With this new fixed offset semantic in-place for BPF kfuncs accepting
> > KF_TRUSTED_ARGS and KF_RCU arguments, we now have more flexibility
> > when it comes to the BPF kfuncs that we're able to introduce moving
> > forward.
> >
> > Early discussions covering the possibility of relaxing the zero fixed
> > offset constraint can be found using the link below. This will provide
> > more context on where all this has stemmed from [1].
> >
> > Notably, pre-existing tests have been updated such that they provide
> > coverage for the updated zero fixed offset
> > functionality. Specifically, the nested offset test was converted from
> > a negative to positive test as it was already designed to assert zero
> > fixed offset semantics of a KF_TRUSTED_ARGS BPF kfunc.
> >
> > [0] https://lore.kernel.org/bpf/ZnA9ndnXKtHOuYMe@google.com/
> > [1] https://lore.kernel.org/bpf/ZhkbrM55MKQ0KeIV@google.com/
> >
> > Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
> > ---
> 
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> 
> Though I'm not sure this is bpf material since it isn't a fix, it
> might be better to base it against bpf-next.

Yes, sorry, this was based off bpf-next. I just happened to screw up
the subject prefix.

Thanks for the review! 

/M