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

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Tue, 02 Jun 2020 11:23:24 +0200
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> 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?

Yes.  In this implementation the "user-defined" member 'storage' do have
variable size (and can be non-existing).  Do you mean that I have
limited the total size of the struct to be 32 bytes?
(Which is true, and that can also be made dynamic, but I was trying to
limit the scope of patch.  It is hard enough to wrap head around the
binary struct from userspace is becoming dynamic)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer