netdev - Re: [PATCH iproute2] genl: print caps for all families

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMnfDhAXsZKY7UqwCxgeXGH1Q-pQdqSycMHw+MSRZSABVA@mail.gmail.com>
Date:   Fri, 24 Feb 2023 10:15:50 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Jakub Kicinski <kuba@...nel.org>, stephen@...workplumber.org,
        dsahern@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH iproute2] genl: print caps for all families

On Fri, Feb 24, 2023 at 3:33 AM Johannes Berg <johannes@...solutions.net> wrote:
>
> On Thu, 2023-02-23 at 17:57 -0800, Jakub Kicinski wrote:
> > On Thu, 23 Feb 2023 17:52:34 -0800 Jakub Kicinski wrote:
> > > Back in 2006 kernel commit 334c29a64507 ("[GENETLINK]: Move
> > > command capabilities to flags.") removed some attributes and
> > > moved the capabilities to flags. Corresponding iproute2
> > > commit 26328fc3933f ("Add controller support for new features
> > > exposed") added the ability to print those caps.
> > >
> > > Printing is gated on version of the family, but we're checking
> > > the version of each individual family rather than the control
> > > family. The format of attributes in the control family
> > > is dictated by the version of the control family alone.
> > >
> > > Families can't use flags for random things, anyway,
> > > because kernel core has a fixed interpretation.
> > >
> > > Thanks to this change caps will be shown for all families
> > > (assuming kernel newer than 2.6.19), not just those which
> > > by coincidence have their local version >= 2.
> > >
> > > For instance devlink, before:
> > >
> > >   $ genl ctrl get name devlink
> > >   Name: devlink
> > >     ID: 0x15  Version: 0x1  header size: 0  max attribs: 179
> > >     commands supported:
> > >             #1:  ID-0x1
> > >             #2:  ID-0x5
> > >             #3:  ID-0x6
> > >             ...
> > >
> > > after:
> > >
> > >   $ genl ctrl get name devlink
> > >   Name: devlink
> > >     ID: 0x15  Version: 0x1  header size: 0  max attribs: 179
> > >     commands supported:
> > >             #1:  ID-0x1
> > >             Capabilities (0xe):
> > >               can doit; can dumpit; has policy
> > >
> > >             #2:  ID-0x5
> > >             Capabilities (0xe):
> > >               can doit; can dumpit; has policy
> > >
> > >             #3:  ID-0x6
> > >             Capabilities (0xb):
> > >               requires admin permission; can doit; has policy
> > >
> > > Leave ctrl_v as 0 if we fail to read the version. Old code used 1
> > > as the default, but 0 or 1 - does not matter, checks are for >= 2.
> > >
> > > Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> > > ---
> > > Not really sure if this is a fix or not..
> >
> > Adding Johannes, that's probably everyone who ever used this
> > command on CC? ;)
>
> Hehe. I'm not even sure I use(d) that part of it frequently ;-)
>
> > > --- a/genl/ctrl.c
> > > +++ b/genl/ctrl.c
> > > @@ -21,6 +21,8 @@
> > >  #define GENL_MAX_FAM_OPS   256
> > >  #define GENL_MAX_FAM_GRPS  256
> > >
> > > +static unsigned int ctrl_v;
>
> You know I looked at this on my phone this morning and missed the fact
> that it's iproute2, and was wondering what you're doing with a global
> variable in the kernel ;-)
>
> There's this code also:
>
> > static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
> > ...
> > static int print_ctrl_grp(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
>
> and it feels a bit pointless to pass a now global ctrl_v to the function
> arguments?
>
> > > @@ -264,6 +313,9 @@ static int ctrl_list(int cmd, int argc, char **argv)
> > >             exit(1);
> > >     }
> > >
> > > +   if (!ctrl_v)
> > > +           ctrl_load_ctrl_version(&rth);
>
> You call this here, but what about this:
>
> > struct genl_util ctrl_genl_util = {
> >         .name = "ctrl",
> >         .parse_genlopt = parse_ctrl,
> >         .print_genlopt = print_ctrl2,
> > };
>
> where print_ctrl2 and hence all the above will be called with a now zero
> ctrl_v, whereas before it would've been - at least in some cases? -
> initialized by ctrl_list() itself?
>
>
> Oh. I see now. The issue was which version we use - the family version
> vs. the controller version. How did I miss that until here ...
>
> Still it seems it should be always initialized in print_ctrl rather than
> in ctrl_list, to capture the case of print_ctrl2? Or maybe in there, but
> that's called inside ctrl_list(), so maybe have parse_ctrl() already
> initialize it, rather than ctrl_list()?

Actually, there is a small gotcha with using a global in the patch -
events (genl ctrl monitor).
If it works, it will be expensive to load the controller for every event.

cheers,
jamal
> johannes