Re: [patch net-next v3 01/11] net: devlink: make sure that devlink_try_get() works with valid pointer during xarray iteration

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jul 21, 2022 at 12:25:54AM CEST, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Wednesday, July 20, 2022 8:12 AM
>> To: netdev@vger.kernel.org
>> Cc: davem@davemloft.net; kuba@kernel.org; idosch@nvidia.com;
>> petrm@nvidia.com; pabeni@redhat.com; edumazet@google.com;
>> mlxsw@nvidia.com; saeedm@nvidia.com; snelson@pensando.io
>> Subject: [patch net-next v3 01/11] net: devlink: make sure that devlink_try_get()
>> works with valid pointer during xarray iteration
>> 
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Remove dependency on devlink_mutex during devlinks xarray iteration.
>> 
>> The reason is that devlink_register/unregister() functions taking
>> devlink_mutex would deadlock during devlink reload operation of devlink
>> instance which registers/unregisters nested devlink instances.
>> 
>> The devlinks xarray consistency is ensured internally by xarray.
>> There is a reference taken when working with devlink using
>> devlink_try_get(). But there is no guarantee that devlink pointer
>> picked during xarray iteration is not freed before devlink_try_get()
>> is called.
>> 
>> Make sure that devlink_try_get() works with valid pointer.
>> Achieve it by:
>> 1) Splitting devlink_put() so the completion is sent only
>>    after grace period. Completion unblocks the devlink_unregister()
>>    routine, which is followed-up by devlink_free()
>> 2) Iterate the devlink xarray holding RCU read lock.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>
>
>This makes sense as long as its ok to drop the rcu_read_lock while in the body of the xa loops. That feels a bit odd to me...

Yes, it is okay. See my comment below.


>
>> ---
>> v2->v3:
>> - s/enf/end/ in devlink_put() comment
>> - added missing rcu_read_lock() call to info_get_dumpit()
>> - extended patch description by motivation
>> - removed an extra "by" from patch description
>> v1->v2:
>> - new patch (originally part of different patchset)
>> ---
>>  net/core/devlink.c | 114 ++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 96 insertions(+), 18 deletions(-)
>> 
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 98d79feeb3dc..6a3931a8e338 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -70,6 +70,7 @@ struct devlink {
>>  	u8 reload_failed:1;
>>  	refcount_t refcount;
>>  	struct completion comp;
>> +	struct rcu_head rcu;
>>  	char priv[] __aligned(NETDEV_ALIGN);
>>  };
>> 
>> @@ -221,8 +222,6 @@ static DEFINE_XARRAY_FLAGS(devlinks,
>> XA_FLAGS_ALLOC);
>>  /* devlink_mutex
>>   *
>>   * An overall lock guarding every operation coming from userspace.
>> - * It also guards devlink devices list and it is taken when
>> - * driver registers/unregisters it.
>>   */
>>  static DEFINE_MUTEX(devlink_mutex);
>> 
>> @@ -232,10 +231,21 @@ struct net *devlink_net(const struct devlink *devlink)
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_net);
>> 
>> +static void __devlink_put_rcu(struct rcu_head *head)
>> +{
>> +	struct devlink *devlink = container_of(head, struct devlink, rcu);
>> +
>> +	complete(&devlink->comp);
>> +}
>> +
>>  void devlink_put(struct devlink *devlink)
>>  {
>>  	if (refcount_dec_and_test(&devlink->refcount))
>> -		complete(&devlink->comp);
>> +		/* Make sure unregister operation that may await the completion
>> +		 * is unblocked only after all users are after the end of
>> +		 * RCU grace period.
>> +		 */
>> +		call_rcu(&devlink->rcu, __devlink_put_rcu);
>>  }
>> 
>>  struct devlink *__must_check devlink_try_get(struct devlink *devlink)
>> @@ -295,6 +305,7 @@ static struct devlink *devlink_get_from_attrs(struct net
>> *net,
>> 
>>  	lockdep_assert_held(&devlink_mutex);
>> 
>> +	rcu_read_lock();
>>  	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>>  		if (strcmp(devlink->dev->bus->name, busname) == 0 &&
>>  		    strcmp(dev_name(devlink->dev), devname) == 0 &&
>> @@ -306,6 +317,7 @@ static struct devlink *devlink_get_from_attrs(struct net
>> *net,
>> 
>>  	if (!found || !devlink_try_get(devlink))
>>  		devlink = ERR_PTR(-ENODEV);
>> +	rcu_read_unlock();
>> 
>>  	return devlink;
>>  }
>> @@ -1329,9 +1341,11 @@ static int devlink_nl_cmd_rate_get_dumpit(struct
>> sk_buff *msg,
>>  	int err = 0;
>> 
>>  	mutex_lock(&devlink_mutex);
>> +	rcu_read_lock();
>>  	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>>  		if (!devlink_try_get(devlink))
>>  			continue;
>> +		rcu_read_unlock();
>> 
>>  		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>>  			goto retry;
>> @@ -1358,7 +1372,9 @@ static int devlink_nl_cmd_rate_get_dumpit(struct
>> sk_buff *msg,
>>  		devl_unlock(devlink);
>>  retry:
>>  		devlink_put(devlink);
>> +		rcu_read_lock();
>>  	}
>> +	rcu_read_unlock();
>>  out:
>>  	mutex_unlock(&devlink_mutex);
>>  	if (err != -EMSGSIZE)
>> @@ -1432,29 +1448,32 @@ static int devlink_nl_cmd_get_dumpit(struct sk_buff
>> *msg,
>>  	int err;
>> 
>>  	mutex_lock(&devlink_mutex);
>> +	rcu_read_lock();
>>  	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>>  		if (!devlink_try_get(devlink))
>>  			continue;
>> +		rcu_read_unlock();
>> 
>
>Is it safe to rcu_read_unlock here while we're still in the middle of the array processing? What happens if something else updates the xarray? is the for_each_marked safe?

Sure, you don't need to hold rcu_read_lock during call to xa_for_each_marked.
The consistency of xarray is itself guaranteed. The only reason to take
rcu_read_lock outside is that the devlink pointer which is
rcu_dereference_check()'ed inside xa_for_each_marked() is still valid
once we devlink_try_get() it.


>
>> -		if (!net_eq(devlink_net(devlink), sock_net(msg->sk))) {
>> -			devlink_put(devlink);
>> -			continue;
>> -		}
>> +		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>> +			goto retry;
>> 
>
>Ahh retry is at the end of the loop, so we'll just skip this one and move to the next one without needing to duplicate both devlink_put and rcu_read_lock.. ok.

Yep.


>
>> -		if (idx < start) {
>> -			idx++;
>> -			devlink_put(devlink);
>> -			continue;
>> -		}
>> +		if (idx < start)
>> +			goto inc;
>> 
>>  		err = devlink_nl_fill(msg, devlink, DEVLINK_CMD_NEW,
>>  				      NETLINK_CB(cb->skb).portid,
>>  				      cb->nlh->nlmsg_seq, NLM_F_MULTI);
>> -		devlink_put(devlink);
>> -		if (err)
>> +		if (err) {
>> +			devlink_put(devlink);
>>  			goto out;
>> +		}
>> +inc:
>>  		idx++;
>> +retry:
>> +		devlink_put(devlink);
>> +		rcu_read_lock();
>>  	}
>> +	rcu_read_unlock();
>>  out:
>>  	mutex_unlock(&devlink_mutex);
>> 

[...]