[PATCH net] devlink: Add missing genlmsg_cancel() in devlink_nl_sb_port_pool_fill()

[PATCH net] devlink: Add missing genlmsg_cancel() in devlink_nl_sb_port_pool_fill()

[Wang Hai]

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@huawei.com>
Signed-off-by: Wang Hai <wanghai38@huawei.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;
+		}
 		if (!err) {
 			if (nla_put_u32(msg, DEVLINK_ATTR_SB_OCC_CUR, cur))
 				goto nla_put_failure;
-- 
2.17.1

Re: [PATCH net] devlink: Add missing genlmsg_cancel() in devlink_nl_sb_port_pool_fill()

[Jakub Kicinski]

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@huawei.com>
> Signed-off-by: Wang Hai <wanghai38@huawei.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,

Re: [PATCH net] devlink: Add missing genlmsg_cancel() in devlink_nl_sb_port_pool_fill()

[wanghai (M)]

在 2020/11/13 1:51, Jakub Kicinski 写道:
> 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().
>> +++ b/net/core/devlink.c
>> @@ -1447,8 +1447,10 @@ static int devlink_nl_sb_port_pool_fill(struct sk_buff *msg,
[...]
>>   			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:
>
[...]
>   static int devlink_nl_cmd_sb_port_pool_get_doit(struct sk_buff *skb,
>
> .

Thanks for your review,  I just sent v2

[PATCH v2 net] devlink: Add missing genlmsg_cancel() in 
devlink_nl_sb_port_pool_fill()