linux-kernel - Re: [PATCH net] devlink: Add missing genlmsg_cancel() in devlink_nl_sb_port_pool

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: <20201112095124.660733a7@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 12 Nov 2020 09:51:24 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Wang Hai <wanghai38@...wei.com>
Cc:     <jiri@...dia.com>, <davem@...emloft.net>, <idosch@...lanox.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] devlink: Add missing genlmsg_cancel() in
 devlink_nl_sb_port_pool_fill()

On Wed, 11 Nov 2020 21:58:53 +0800 Wang Hai wrote:
> If sb_occ_port_pool_get() failed in devlink_nl_sb_port_pool_fill(),
> msg should be canceled by genlmsg_cancel().
> 
> Fixes: df38dafd2559 ("devlink: implement shared buffer occupancy monitoring interface")
> Reported-by: Hulk Robot <hulkci@...wei.com>
> Signed-off-by: Wang Hai <wanghai38@...wei.com>
> ---
>  net/core/devlink.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index a932d95be798..83b4e7f51b35 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -1447,8 +1447,10 @@ static int devlink_nl_sb_port_pool_fill(struct sk_buff *msg,
>  
>  		err = ops->sb_occ_port_pool_get(devlink_port, devlink_sb->index,
>  						pool_index, &cur, &max);
> -		if (err && err != -EOPNOTSUPP)
> +		if (err && err != -EOPNOTSUPP) {
> +			genlmsg_cancel(msg, hdr);
>  			return err;

I guess the driver would have to return -EMSGSIZE for this to matter,
which is quite unlikely but we should indeed fix.

Still, returning in the middle of the function with an epilogue is what
got use here in the first place, so please use a goto. E.g. like this:


diff --git a/net/core/devlink.c b/net/core/devlink.c
index a932d95be798..be8ee96ad188 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1448,7 +1448,7 @@ static int devlink_nl_sb_port_pool_fill(struct sk_buff *msg,
                err = ops->sb_occ_port_pool_get(devlink_port, devlink_sb->index,
                                                pool_index, &cur, &max);
                if (err && err != -EOPNOTSUPP)
-                       return err;
+                       goto sb_occ_get_failure;
                if (!err) {
                        if (nla_put_u32(msg, DEVLINK_ATTR_SB_OCC_CUR, cur))
                                goto nla_put_failure;
@@ -1461,8 +1461,10 @@ static int devlink_nl_sb_port_pool_fill(struct sk_buff *msg,
        return 0;
 
 nla_put_failure:
+       err = -EMSGSIZE;
+sb_occ_get_failure:
        genlmsg_cancel(msg, hdr);
-       return -EMSGSIZE;
+       return err;
 }
 
 static int devlink_nl_cmd_sb_port_pool_get_doit(struct sk_buff *skb,