Message-ID: <20190802113935.63be803a@cakuba.netronome.com>
Date:   Fri, 2 Aug 2019 11:39:35 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     "Daniel T. Lee" <danieltimlee@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP
 on interface

On Fri, 2 Aug 2019 14:02:29 +0900, Daniel T. Lee wrote:
> On Fri, Aug 2, 2019 at 8:36 AM Jakub Kicinski  wrote:
> > On Thu,  1 Aug 2019 17:11:32 +0900, Daniel T. Lee wrote:  
> > > By this commit, using `bpftool net attach`, user can attach XDP prog on
> > > interface. New type of enum 'net_attach_type' has been made, as stated at
> > > cover-letter, the meaning of 'attach' is, prog will be attached on interface.
> > >
> > > BPF prog will be attached through libbpf 'bpf_set_link_xdp_fd'.
> > >
> > > Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> > > ---
> > > Changes in v2:
> > >   - command 'load' changed to 'attach' for the consistency
> > >   - 'NET_ATTACH_TYPE_XDP_DRIVE' changed to 'NET_ATTACH_TYPE_XDP_DRIVER'
> > >
> > >  tools/bpf/bpftool/net.c | 107 +++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 106 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > > index 67e99c56bc88..f3b57660b303 100644
> > > --- a/tools/bpf/bpftool/net.c
> > > +++ b/tools/bpf/bpftool/net.c
> > > @@ -55,6 +55,35 @@ struct bpf_attach_info {
> > >       __u32 flow_dissector_id;
> > >  };
> > >
> > > +enum net_attach_type {
> > > +     NET_ATTACH_TYPE_XDP,
> > > +     NET_ATTACH_TYPE_XDP_GENERIC,
> > > +     NET_ATTACH_TYPE_XDP_DRIVER,
> > > +     NET_ATTACH_TYPE_XDP_OFFLOAD,
> > > +     __MAX_NET_ATTACH_TYPE
> > > +};
> > > +
> > > +static const char * const attach_type_strings[] = {
> > > +     [NET_ATTACH_TYPE_XDP] = "xdp",
> > > +     [NET_ATTACH_TYPE_XDP_GENERIC] = "xdpgeneric",
> > > +     [NET_ATTACH_TYPE_XDP_DRIVER] = "xdpdrv",
> > > +     [NET_ATTACH_TYPE_XDP_OFFLOAD] = "xdpoffload",
> > > +     [__MAX_NET_ATTACH_TYPE] = NULL,  
> >
> > Not sure if the terminator is necessary,
> > ARRAY_SIZE(attach_type_strings) should suffice?  
> 
> Yes, ARRAY_SIZE is fine though. But I was just trying to make below
> 'parse_attach_type' consistent with 'parse_attach_type' from the 'prog.c'.
> At 'prog.c', It has same terminator at 'attach_type_strings'.
> 
> Should I change it or keep it?

Oh well, I guess there is some precedent for that :S

Quick grep for const char * const reveals we have around 7 non-NULL
terminated arrays, and 2 NULL terminated. Plus the NULL-terminated
don't align the '=' sign, while most do.

it's not a big deal, my preference is for not NULL terminating here,
and aligning '='.

> > > +     NEXT_ARG();
> > > +     if (!REQ_ARGS(1))
> > > +             return -EINVAL;  
> >
> > Error message needed here.
> >  
> 
> Actually it provides error message like:
> Error: 'xdp' needs at least 1 arguments, 0 found
> 
> are you suggesting that any additional error message is necessary?

Ah, sorry, I missed REQ_ARGS() there!

> > > +             return -EINVAL;
> > > +     }  
> >
> > Please require the dev keyword before the interface name.
> > That'll make it feel closer to prog load syntax.  
> 
> If adding the dev keyword before interface name, will it be too long to type in?

I think it's probably muscle memory for most. Plus we have excellent
bash completions.

> and also `bpftool prog` use extra keyword (such as dev) when it is
> optional keyword.
> 
>        bpftool prog dump jited  PROG [{ file FILE | opcodes | linum }]
>        bpftool prog pin   PROG FILE
>        bpftool prog { load | loadall } OBJ  PATH \
> 
> as you can see here, FILE uses optional keyword 'file' when the
> argument is optional.

Not sure I follow 🤔

>        bpftool prog { load | loadall } OBJ  PATH \
>                          [type TYPE] [dev NAME] \
>                          [map { idx IDX | name NAME } MAP]\
>                          [pinmaps MAP_DIR]
> 
> Yes, bpftool prog load has dev keyword with it,
> 
> but first, like previous, the argument is optional so i think it is
> unnecessary to use optional keyword 'dev'.

The keyword should not be optional if device name is specified.
Maybe lack of coffee on my side..

> and secondly, 'bpftool net attach' isn't really related to 'bpftool prog load'.
>
> At previous version patch, I was using word 'load' instead of
> 'attach', since XDP program is not
> considered as 'BPF_PROG_ATTACH', so it might give a confusion. However
> by the last patch discussion,
> word 'load' has been replaced to 'attach'.
> 
> Keeping the consistency is very important, but I was just wandering
> about making command
> similar to 'bpftool prog load' syntax.

In case of TC the device argument is optional. You may specify it, or
you can refer to TC blocks instead. So for that reason alone I think
it'll be much cleaner to require dev before the interface name.

> > > +     return 0;
> > > +}
> > > +
> > > +static int do_attach_detach_xdp(int *progfd, enum net_attach_type *attach_type,
> > > +                             int *ifindex)
> > > +{
> > > +     __u32 flags;
> > > +     int err;
> > > +
> > > +     flags = XDP_FLAGS_UPDATE_IF_NOEXIST;  
> >
> > Please add this as an option so that user can decide whether overwrite
> > is allowed or not.  
> 
> Adding force flag to bpftool seems necessary.
> I will add an optional argument for this.

Right, I was wondering if we want to call it force, though? force is
sort of a reuse of iproute2 concept. But it's kind of hard to come up
with names.

Just to be sure - I mean something like:

bpftool net attach xdp id xyz dev ethN noreplace

Rather than:

bpftool -f net attach xdp id xyz dev ethN

> > > +     if (*attach_type == NET_ATTACH_TYPE_XDP_GENERIC)
> > > +             flags |= XDP_FLAGS_SKB_MODE;
> > > +     if (*attach_type == NET_ATTACH_TYPE_XDP_DRIVER)
> > > +             flags |= XDP_FLAGS_DRV_MODE;
> > > +     if (*attach_type == NET_ATTACH_TYPE_XDP_OFFLOAD)
> > > +             flags |= XDP_FLAGS_HW_MODE;
> > > +
> > > +     err = bpf_set_link_xdp_fd(*ifindex, *progfd, flags);
> > > +
> > > +     return err;  
> >
> > no need for the err variable here.  
> 
> My apologies, but I'm not sure why err variable isn't needed at here.
> AFAIK, 'bpf_set_link_xdp_fd' from libbpf returns the netlink_recv result,
> and in order to propagate error, err variable is necessary, I guess?

	return bpf_set_link_xdp_fd(*ifindex, *progfd, flags);

Is what I meant.

> > > +}
> > > +
> > > +static int do_attach(int argc, char **argv)
> > > +{
> > > +     enum net_attach_type attach_type;
> > > +     int err, progfd, ifindex;
> > > +
> > > +     err = parse_attach_args(argc, argv, &progfd, &attach_type, &ifindex);
> > > +     if (err)
> > > +             return err;  
> >
> > Probably not the best idea to move this out into a helper.  
> 
> Again, just trying to make consistent with 'prog.c'.
> 
> But clearly it has differences with do_attach/detach from 'prog.c'.
> From it, it uses the same parse logic 'parse_attach_detach_args' since
> the two command 'bpftool prog attach/detach' uses the same argument format.
> 
> However, in here, 'bpftool net' attach and detach requires different number of
> argument, so function for parse argument has been defined separately.
> The situation is little bit different, but keeping argument parse logic as an
> helper, I think it's better in terms of consistency.

Well they won't share the same arguments if you add the keyword for
controlling IF_NOEXIST :(

> About the moving parse logic to a helper, I was trying to keep command
> entry (do_attach)
> as simple as possible. Parse all the argument in command entry will
> make function longer
> and might make harder to understand what it does.
> 
> And I'm not pretty sure that argument parse logic will stays the same
> after other attachment
> type comes in. What I mean is, the argument count or type might be
> added and to fulfill
> all that specific cases, the code might grow larger.
> 
> So for the consistency, simplicity and extensibility, I prefer to keep
> it as a helper.
> 
> > > +     if (is_prefix("xdp", attach_type_strings[attach_type]))
> > > +             err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);  
> >
> > Hm. We either need an error to be reported if it's not xdp or since we
> > only accept XDP now perhaps the if() is superfluous?  
> 
> Well, if the attach_type isn't xdp, the error will be occurred from
> the argument parse,
> Will it be necessary to reinforce with error logic to make it more secure?

Hm. it should already be fine, no? For non-xdp parse_attach_type() will
return __MAX_NET_ATTACH_TYPE, then parsing returns EINVAL and we exit.
Not sure I follow.

Powered by blists - more mailing lists