netdev - Re: [patch net-next 06/11] net: devlink: don't take devlink_mutex for devlink_compat

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: <20190322134452.GD2211@nanopsycho>
Date:   Fri, 22 Mar 2019 14:44:52 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, mlxsw@...lanox.com,
        idosch@...lanox.com, f.fainelli@...il.com, andrew@...n.ch,
        vivien.didelot@...il.com
Subject: Re: [patch net-next 06/11] net: devlink: don't take devlink_mutex
 for devlink_compat_*

Thu, Mar 21, 2019 at 08:08:24PM CET, jakub.kicinski@...ronome.com wrote:
>On Thu, 21 Mar 2019 14:20:14 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@...lanox.com>
>> 
>> The netdevice is guaranteed to not disappear so we can rely that
>> devlink_port and devlink won't disappear as well. No need to take
>> devlink_mutex so don't take it here.
>
>I'm not sure the port can't disappear, just looking at this series it
>seems bnxt registers the port after netdev (maybe it unregisters in 
>the other order).  Not that it matters here, we use the main devlink
>instance for the compat helpers, and devlink instance should
>definitely exist.

You are right. Btw, the devlink_port-netdev registration order should be
fixed as well. Niting it down to my todo.


>
>So FWIW the entire series looks good to me.
>
>I've also pushed my port patches out:
>
>https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/log/?h=devlink-pci-ports
>
>if that's of any use to you (e.g. the patch which changes the order of
>port vs netdev registration).

Thanks.


>
>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 3dc51ddf7451..1e125c3b890c 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -6444,17 +6444,15 @@ void devlink_compat_running_version(struct net_device *dev,
>>  	dev_hold(dev);
>>  	rtnl_unlock();
>>  
>> -	mutex_lock(&devlink_mutex);
>>  	devlink = netdev_to_devlink(dev);
>>  	if (!devlink || !devlink->ops->info_get)
>> -		goto unlock_list;
>> +		goto out;
>>  
>>  	mutex_lock(&devlink->lock);
>>  	__devlink_compat_running_version(devlink, buf, len);
>>  	mutex_unlock(&devlink->lock);
>> -unlock_list:
>> -	mutex_unlock(&devlink_mutex);
>>  
>> +out:
>>  	rtnl_lock();
>>  	dev_put(dev);
>>  }
>> @@ -6462,22 +6460,22 @@ void devlink_compat_running_version(struct net_device *dev,
>>  int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
>>  {
>>  	struct devlink *devlink;
>> -	int ret = -EOPNOTSUPP;
>> +	int ret;
>>  
>>  	dev_hold(dev);
>>  	rtnl_unlock();
>>  
>> -	mutex_lock(&devlink_mutex);
>>  	devlink = netdev_to_devlink(dev);
>> -	if (!devlink || !devlink->ops->flash_update)
>> -		goto unlock_list;
>> +	if (!devlink || !devlink->ops->flash_update) {
>> +		ret = -EOPNOTSUPP;
>> +		goto out;
>> +	}
>>  
>>  	mutex_lock(&devlink->lock);
>>  	ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
>>  	mutex_unlock(&devlink->lock);
>> -unlock_list:
>> -	mutex_unlock(&devlink_mutex);
>>  
>> +out:
>>  	rtnl_lock();
>>  	dev_put(dev);
>>  
>