netdev - Re: [PATCH net-next 3/3] genetlink: fit NLMSG

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]

Message-ID: <CANn89iLD0C3An_RUMt8W0d2PiJhXaSGAUsUhyhTg1Qa+50HC+g@mail.gmail.com>
Date: Fri, 1 Mar 2024 09:40:17 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, pabeni@...hat.com, 
	johannes@...solutions.net, fw@...len.de, pablo@...filter.org, 
	idosch@...dia.com, jiri@...nulli.us
Subject: Re: [PATCH net-next 3/3] genetlink: fit NLMSG_DONE into same read()
 as families

On Fri, Mar 1, 2024 at 2:31 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Make sure ctrl_fill_info() returns sensible error codes and
> propagate them out to netlink core. Let netlink core decide
> when to return skb->len and when to treat the exit as an
> error. Netlink core does better job at it, if we always
> return skb->len the core doesn't know when we're done
> dumping and NLMSG_DONE ends up in a separate read().
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: jiri@...nulli.us
> ---
>  net/netlink/genetlink.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 50ec599a5cff..70379ecfb6ed 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1232,7 +1232,7 @@ static int ctrl_fill_info(const struct genl_family *family, u32 portid, u32 seq,
>
>         hdr = genlmsg_put(skb, portid, seq, &genl_ctrl, flags, cmd);
>         if (hdr == NULL)
> -               return -1;
> +               return -EMSGSIZE;
>
>         if (nla_put_string(skb, CTRL_ATTR_FAMILY_NAME, family->name) ||
>             nla_put_u16(skb, CTRL_ATTR_FAMILY_ID, family->id) ||
> @@ -1355,6 +1355,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
>         struct net *net = sock_net(skb->sk);
>         int fams_to_skip = cb->args[0];
>         unsigned int id;
> +       int err;

Don't we have to initialize err to 0 ?

Reviewed-by: Eric Dumazet <edumazet@...gle.com>

Thanks !

>
>         idr_for_each_entry(&genl_fam_idr, rt, id) {
>                 if (!rt->netnsok && !net_eq(net, &init_net))
> @@ -1363,16 +1364,17 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
>                 if (n++ < fams_to_skip)
>                         continue;
>
> -               if (ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid,
> -                                  cb->nlh->nlmsg_seq, NLM_F_MULTI,
> -                                  skb, CTRL_CMD_NEWFAMILY) < 0) {
> +               err = ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid,
> +                                    cb->nlh->nlmsg_seq, NLM_F_MULTI,
> +                                    skb, CTRL_CMD_NEWFAMILY);
> +               if (err) {
>                         n--;
>                         break;
>                 }
>         }
>
>         cb->args[0] = n;
> -       return skb->len;
> +       return err;
>  }
>
>  static struct sk_buff *ctrl_build_family_msg(const struct genl_family *family,
> --
> 2.43.2
>