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]
Date:   Sat, 25 Feb 2023 11:41:48 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     stephen@...workplumber.org, dsahern@...il.com,
        netdev@...r.kernel.org, johannes@...solutions.net
Subject: Re: [PATCH iproute2] genl: print caps for all families

On Fri, Feb 24, 2023 at 7:38 PM Jakub Kicinski <kuba@...nel.org> 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.
>
> In fact the entire version check is not strictly necessary.
> The code is not using the old attributes, so on older kernels
> it will simply print nothing either way.
>
> Families can't use flags for random things, 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
>
> Fixes: 26328fc3933f ("Add controller support for new features exposed")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  genl/ctrl.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/genl/ctrl.c b/genl/ctrl.c
> index a2d87af0ad07..8d2e944802ba 100644
> --- a/genl/ctrl.c
> +++ b/genl/ctrl.c
> @@ -57,7 +57,7 @@ static void print_ctrl_cmd_flags(FILE *fp, __u32 fl)
>         fprintf(fp, "\n");
>  }
>
> -static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
> +static int print_ctrl_cmds(FILE *fp, struct rtattr *arg)
>  {
>         struct rtattr *tb[CTRL_ATTR_OP_MAX + 1];
>
> @@ -70,7 +70,7 @@ static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
>                 fprintf(fp, " ID-0x%x ",*id);
>         }
>         /* we are only gonna do this for newer version of the controller */
> -       if (tb[CTRL_ATTR_OP_FLAGS] && ctrl_ver >= 0x2) {
> +       if (tb[CTRL_ATTR_OP_FLAGS]) {
>                 __u32 *fl = RTA_DATA(tb[CTRL_ATTR_OP_FLAGS]);
>                 print_ctrl_cmd_flags(fp, *fl);
>         }
> @@ -78,7 +78,7 @@ 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)
> +static int print_ctrl_grp(FILE *fp, struct rtattr *arg)
>  {
>         struct rtattr *tb[CTRL_ATTR_MCAST_GRP_MAX + 1];
>
> @@ -109,7 +109,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>         int len = n->nlmsg_len;
>         struct rtattr *attrs;
>         FILE *fp = (FILE *) arg;
> -       __u32 ctrl_v = 0x1;
>
>         if (n->nlmsg_type !=  GENL_ID_CTRL) {
>                 fprintf(stderr, "Not a controller message, nlmsg_len=%d "
> @@ -148,7 +147,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>         if (tb[CTRL_ATTR_VERSION]) {
>                 __u32 *v = RTA_DATA(tb[CTRL_ATTR_VERSION]);
>                 fprintf(fp, " Version: 0x%x ",*v);
> -               ctrl_v = *v;
>         }
>         if (tb[CTRL_ATTR_HDRSIZE]) {
>                 __u32 *h = RTA_DATA(tb[CTRL_ATTR_HDRSIZE]);
> @@ -198,7 +196,7 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>                 for (i = 0; i < GENL_MAX_FAM_OPS; i++) {
>                         if (tb2[i]) {
>                                 fprintf(fp, "\t\t#%d: ", i);
> -                               if (0 > print_ctrl_cmds(fp, tb2[i], ctrl_v)) {
> +                               if (0 > print_ctrl_cmds(fp, tb2[i])) {
>                                         fprintf(fp, "Error printing command\n");
>                                 }
>                                 /* for next command */
> @@ -221,7 +219,7 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>                 for (i = 0; i < GENL_MAX_FAM_GRPS; i++) {
>                         if (tb2[i]) {
>                                 fprintf(fp, "\t\t#%d: ", i);
> -                               if (0 > print_ctrl_grp(fp, tb2[i], ctrl_v))
> +                               if (0 > print_ctrl_grp(fp, tb2[i]))
>                                         fprintf(fp, "Error printing group\n");
>                                 /* for next group */
>                                 fprintf(fp,"\n");
> --
> 2.39.2
>

Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal