netdev - Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 3 Aug 2019 19:13:41 -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 Sat, 3 Aug 2019 18:39:21 +0900, Daniel T. Lee wrote:
> On Sat, Aug 3, 2019 at 3:39 AM Jakub Kicinski wrote:
> > 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.

Hm. Immutable kind of has a meaning in Linux (check out immutable in
extended file attributes, and CAP_LINUX_IMMUTABLE) which is different
than here.. so I'd avoid that one.

Another option I was thinking about was using the same keywords as maps
do: "noexist" - although we don't have a way of doing "exist"
currently, which is kind of breaking the equivalence.

Or maye we should go the same route as iproute2 after all, and set
noreplace by default?  That way we don't need the negation in the 
name. We could use "overwrite", "replace"?

Naming things... :)

> > > > > +}
> > > > > +
> > > > > +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.

Ah, my apologies I misread your message.

> I'm not sure why it is not the best idea to move arg parse logic into a helper.

Output args are kind of ugly, and having to pass each parameter through
output arg seems like something that could get out of hand as the
number grows. 

Usually command handling functions are relatively small and
straightforward in bpftool so it's quite nice to have it all in one
simple procedure.

But I'm not feeling too strongly about it. Feel free to leave the
parsing in separate functions if you prefer!