On Sat, Aug 3, 2019 at 3:39 AM Jakub Kicinski
<jakub.kicin...@netronome.com> wrote:
>
> 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 <danieltim...@gmail.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 '='.
>
I think following the majority, is better for this case.
Like you mentioned, those 2 NULL-terminated arrays are at 'cgroup.c'
and 'prog.c'
and the strings they are defining are 'bpf_attach_type' which is from
uapi 'bpf.h'.
And, in my guess, the reason for those 2 arrays uses NULL-terminator
is because enum from 'bpf_attach_type' has '__MAX_BPF_ATTACH_TYPE' at
the end.
Actually I was kind of uncomfortable with adding an enum since it's
not used globally and *only* used in 'net.c' context. Instead of using
hacky enum entry, stick to the majority, ARRAY_SIZE, is better to keep
consistency.
I'll update this to next version with '=' alignment.
>
> > > > + 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
>
command 'dump' has optional argument '[ file FILE ]',
and command 'pin' has required argument with 'FILE'.
I thought the preceding optional keyword 'file' with lower-case is
intended for the optional argument since it isn't preceded when the
argument is required.
> > 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.
>
Previously I didn't really considered TC.
Considering the extensibility, since device argument could be optional,
requiring dev before the interface name seems necessary.
Thank you for letting me know! :)
> > > > + 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
>
How about a word 'immutable'? 'noreplace' seems good though.
Just suggesting an option.
> > > > + 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.
>
Oops. I've got it wrong.
I'll update to next patch.
> > > > +}
> > > > +
> > > > +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 :(
>
My apologies, but I think I'm not following with you.
Currently detach/attach isn't sharing same arguments.
And even after adding the argument for IF_NOEXIST such as [ noreplace ],
it'll stays the same for not sharing same arguments.
I'm not sure why it is not the best idea to move arg parse logic into a helper.
> > 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.
Yes. That was what i meant.
Thank you for taking your time for the review.
