Re: [PATCH 1/2] bpf: relax zero fixed offset constraint on trusted pointer arguments

[Matt Bobrowski <mattbobrowski@google.com>]

Hey Eduard,

Thank you for taking a look at this patch series, much appreciated.

On Mon, Jun 17, 2024 at 10:38:56PM -0700, Eduard Zingerman wrote:
> On Mon, 2024-06-17 at 13:43 +0000, Matt Bobrowski wrote:
> 
> [...]
> 
> > * For OBJ_RELEASE and KF_RELEASE BPF helpers and kfuncs:
> > 
> >  * If the expected argument type is of an untyped pointer i.e. void *,
> >    then we continue to enforce a zero fixed offset as we need to
> >    ensure that the correct referenced pointer is handed off correctly
> >    to the relevant deallocation routine
> > 
> >  * If the expected argument is backed by BTF, then we relax the strict
> >    zero fixed offset and allow it only if we successfully type matched
> >    between the register and argument. A failed type match between
> >    register and argument will result in the legacy strict zero offset
> >    semantics
> > 
> > * For KF_TRUSTED_ARGS BPF kfuncs:
> > 
> >  * The fixed zero offset constraint has been lifted, such that
> >    KF_TRUSTED_ARGS BPF kfuncs can now accept a trusted pointer
> >    argument with a non-zero fixed offset providing that register and
> >    argument BTF has type matched successfully
> 
> [...]
> 
> Hi Matt,
> 
> I've read this and the next patch once, but need more time to provide
> feedback. Two quick notes:
> - It seems something is wrong with the way this patch set was sent:
>   for some reason it is not organized as a single thread (e.g. on vger).

Yeah, I don't know what happened there, I must've screwed something up
whilst sending out the patches. The process I use to send these
patches out is rather manual ATM, so I should probably look to revamp
it so I can avoid such hiccups. Apologies about this, but the V2
edition of this patch series will come through structured as intended,
I promise.

> - I see how OBJ_RELEASE arguments trigger btf_struct_ids_match() in
>   check_release_arg_reg_off(), but I don't see how KF_TRUSTED_ARGS
>   trigger similar logic.

Right, the btf_struct_ids_match() is done at a slightly later point
from within process_kf_arg_ptr_to_btf_id().

>   Do you have some positive tests that verify newly added functionality?

The only positive test which demonstrates this new usability
improvement can be seen in the BPF program test_nested_offset, which
I've also included within this patch in:

* tools/testing/selftests/bpf/progs/nested_trust_success.c

The test_nested_offset BPF program is effectively a positively
converted version of the test_invalid_nested_offset BPF program which
resided in:

* tools/testing/selftests/bpf/progs/nested_trust_failure.c

Maybe I could look at some other positive tests too, but the above was
an immediate and obvious candidate. I also intend to post out another
patch series which adds a bunch of new BPF kfuncs. These new BPF
kfuncs will basically piggyback and rely on this new relaxed register
fixed offset checking, so we'll get some implicit positive test
coverage there too I guess.

/M