[PATCH net] devlink: protect devlink dump by the instance lock

[PATCH net] devlink: protect devlink dump by the instance lock

[Jakub Kicinski]

Take the instance lock around devlink_nl_fill() when dumping,
doit takes it already.

We are only dumping basic info so in the worst case we were risking
data races around the reload statistics. Also note that the reloads
themselves had not been under the instance lock until recently, so
the selection of the Fixes tag is inherently questionable.

Fixes: a254c264267e ("devlink: Add reload stats")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@nvidia.com
CC: moshe@mellanox.com
---
 net/core/devlink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index d2df30829083..032d6d0a5ce6 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1648,10 +1648,13 @@ static int devlink_nl_cmd_get_dumpit(struct sk_buff *msg,
 			continue;
 		}
 
+		devl_lock(devlink);
 		err = devlink_nl_fill(msg, devlink, DEVLINK_CMD_NEW,
 				      NETLINK_CB(cb->skb).portid,
 				      cb->nlh->nlmsg_seq, NLM_F_MULTI);
+		devl_unlock(devlink);
 		devlink_put(devlink);
+
 		if (err)
 			goto out;
 		idx++;
-- 
2.38.1

Re: [PATCH net] devlink: protect devlink dump by the instance lock

[Jakub Kicinski]

On Thu, 15 Dec 2022 20:41:22 -0800 Jakub Kicinski wrote:
> Take the instance lock around devlink_nl_fill() when dumping,
> doit takes it already.
> 
> We are only dumping basic info so in the worst case we were risking
> data races around the reload statistics. Also note that the reloads
> themselves had not been under the instance lock until recently, so
> the selection of the Fixes tag is inherently questionable.
> 
> Fixes: a254c264267e ("devlink: Add reload stats")

On second thought, the drivers can't call reload, so until we got rid
of the big bad mutex there could have been no race. I'll swap the tag
for:

Fixes: d3efc2a6a6d8 ("net: devlink: remove devlink_mutex")

when/if applying.

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: jiri@nvidia.com
> CC: moshe@mellanox.com
> ---
>  net/core/devlink.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index d2df30829083..032d6d0a5ce6 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -1648,10 +1648,13 @@ static int devlink_nl_cmd_get_dumpit(struct sk_buff *msg,
>  			continue;
>  		}
>  
> +		devl_lock(devlink);
>  		err = devlink_nl_fill(msg, devlink, DEVLINK_CMD_NEW,
>  				      NETLINK_CB(cb->skb).portid,
>  				      cb->nlh->nlmsg_seq, NLM_F_MULTI);
> +		devl_unlock(devlink);
>  		devlink_put(devlink);
> +
>  		if (err)
>  			goto out;
>  		idx++;

Re: [PATCH net] devlink: protect devlink dump by the instance lock

[Jiri Pirko]

Fri, Dec 16, 2022 at 05:44:47AM CET, kuba@kernel.org wrote:
>On Thu, 15 Dec 2022 20:41:22 -0800 Jakub Kicinski wrote:
>> Take the instance lock around devlink_nl_fill() when dumping,
>> doit takes it already.
>> 
>> We are only dumping basic info so in the worst case we were risking
>> data races around the reload statistics. Also note that the reloads
>> themselves had not been under the instance lock until recently, so
>> the selection of the Fixes tag is inherently questionable.
>> 
>> Fixes: a254c264267e ("devlink: Add reload stats")
>
>On second thought, the drivers can't call reload, so until we got rid
>of the big bad mutex there could have been no race. I'll swap the tag
>for:
>
>Fixes: d3efc2a6a6d8 ("net: devlink: remove devlink_mutex")
>
>when/if applying.

You are right.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Thanks!

Re: [PATCH net] devlink: protect devlink dump by the instance lock

[Jacob Keller]

On 12/16/2022 1:18 AM, Jiri Pirko wrote:
> Fri, Dec 16, 2022 at 05:44:47AM CET, kuba@kernel.org wrote:
>> On Thu, 15 Dec 2022 20:41:22 -0800 Jakub Kicinski wrote:
>>> Take the instance lock around devlink_nl_fill() when dumping,
>>> doit takes it already.
>>>
>>> We are only dumping basic info so in the worst case we were risking
>>> data races around the reload statistics. Also note that the reloads
>>> themselves had not been under the instance lock until recently, so
>>> the selection of the Fixes tag is inherently questionable.
>>>
>>> Fixes: a254c264267e ("devlink: Add reload stats")
>>
>> On second thought, the drivers can't call reload, so until we got rid
>> of the big bad mutex there could have been no race. I'll swap the tag
>> for:
>>
>> Fixes: d3efc2a6a6d8 ("net: devlink: remove devlink_mutex")
>>
>> when/if applying.
> 
> You are right.
> 
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> 
> Thanks!

Agreed!

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net] devlink: protect devlink dump by the instance lock

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 15 Dec 2022 20:41:22 -0800 you wrote:
> Take the instance lock around devlink_nl_fill() when dumping,
> doit takes it already.
> 
> We are only dumping basic info so in the worst case we were risking
> data races around the reload statistics. Also note that the reloads
> themselves had not been under the instance lock until recently, so
> the selection of the Fixes tag is inherently questionable.
> 
> [...]

Here is the summary with links:
  - [net] devlink: protect devlink dump by the instance lock
    https://git.kernel.org/netdev/net/c/214964a13ab5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html