Re: [PATCH bpf-next RFC 2/3] bpf: devmap dynamic map-value storage area based on BTF

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Jesper Dangaard Brouer <brouer@redhat.com> writes:

> On Fri, 29 May 2020 18:39:40 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>> Jesper Dangaard Brouer <brouer@redhat.com> writes:
>> 
>> > The devmap map-value can be read from BPF-prog side, and could be used for a
>> > storage area per device. This could e.g. contain info on headers that need
>> > to be added when packet egress this device.
>> >
>> > This patchset adds a dynamic storage member to struct bpf_devmap_val. More
>> > importantly the struct bpf_devmap_val is made dynamic via leveraging and
>> > requiring BTF for struct sizes above 4. The only mandatory struct member is
>> > 'ifindex' with a fixed offset of zero.
>> >
>> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> > ---
>> >  kernel/bpf/devmap.c |  216 ++++++++++++++++++++++++++++++++++++++++++++-------
>> >  1 file changed, 185 insertions(+), 31 deletions(-)
>> >
>> > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
>> > index 4ab67b2d8159..9cf2dadcc0fe 100644
> [...]
>> > @@ -60,13 +61,30 @@ struct xdp_dev_bulk_queue {
>> >  	unsigned int count;
>> >  };
>> >  
>> > -/* DEVMAP values */
>> > +/* DEVMAP map-value layout.
>> > + *
>> > + * The struct data-layout of map-value is a configuration interface.
>> > + * BPF-prog side have read-only access to this memory.
>> > + *
>> > + * The layout might be different than below, because some struct members are
>> > + * optional.  This is made dynamic by requiring userspace provides an BTF
>> > + * description of the struct layout, when creating the BPF-map. Struct names
>> > + * are important and part of API, as BTF use these names to identify members.
>> > + */
>> >  struct bpf_devmap_val {
>> > -	__u32 ifindex;   /* device index */
>> > +	__u32 ifindex;   /* device index - mandatory */
>> >  	union {
>> >  		int   fd;  /* prog fd on map write */
>> >  		__u32 id;  /* prog id on map read */
>> >  	} bpf_prog;
>> > +	struct {
>> > +		/* This 'storage' member is meant as a dynamically sized area,
>> > +		 * that BPF developer can redefine.  As other members are added
>> > +		 * overtime, this area can shrink, as size can be regained by
>> > +		 * not using members above. Add new members above this struct.
>> > +		 */
>> > +		unsigned char data[24];
>> > +	} storage;  
>> 
>> Why is this needed? Userspace already passes in the value_size, so why
>> can't the kernel just use the BTF to pick out the values it cares about
>> and let the rest be up to userspace?
>
> The kernel cannot just ignore unknown struct members, due to forward
> compatibility. An older kernel that sees a new struct member, cannot
> know what this struct member is used for.  Thus, later I'm rejecting
> map creation if I detect members kernel doesn't know about.
>
> This means, that I need to create a named area (e.g. named 'storage')
> that users can define their own layout within.
>
> This might be difficult to comprehend for other kernel developers,
> because usually we create forward compatibility via walking the binary
> struct and then assume that if an unknown area (in end-of-struct)
> contains zeros, then it means end-user isn't using that unknown feature.
> This doesn't work when the default value, as in this exact case, need
> to be minus-1 do describe "unused" as this is a file descriptor.
>
> Forward compatibility is different here.  If the end-user include the
> member in their BTF description, that means they intend to use it.
> Thus, kernel need to reject map-create if it sees unknown members.

Ah, right, of course. You could still allow such a "user-defined" member
to be any size userspace likes, though, couldn't you?

-Toke