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

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: <YtjpaRtkOwX00azI@nanopsycho>
Date:   Thu, 21 Jul 2022 07:51:37 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, idosch@...dia.com,
        petrm@...dia.com, pabeni@...hat.com, edumazet@...gle.com,
        mlxsw@...dia.com, saeedm@...dia.com, snelson@...sando.io
Subject: Re: [patch net-next v3 01/11] net: devlink: make sure that
 devlink_try_get() works with valid pointer during xarray iteration

Thu, Jul 21, 2022 at 02:49:53AM CEST, kuba@...nel.org wrote:
>On Wed, 20 Jul 2022 17:12:24 +0200 Jiri Pirko wrote:
>> +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);
>>  }
>
>Hm. I always assumed we'd just use the xa_lock(). Unmarking the
>instance as registered takes that lock which provides a natural 
>barrier for others trying to take a reference.

I guess that the xa_lock() scheme could work, as far as I see it. But
what's wrong with the rcu scheme? I actually find it quite neat. No need
to have another odd iteration helpers. We just benefit of xa_array rcu
internals to make sure devlink pointer is valid at the time we make a
reference. Very clear.



>
>Something along these lines (untested):
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 98d79feeb3dc..6321ea123f79 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -278,6 +278,38 @@ void devl_unlock(struct devlink *devlink)
> }
> EXPORT_SYMBOL_GPL(devl_unlock);
> 
>+static struct devlink *devlink_iter_next(unsigned long *index)
>+{
>+	struct devlink *devlink;
>+
>+	xa_lock(&devlinks);
>+	devlink = xa_find_after(&devlinks, index, ULONG_MAX,
>+				DEVLINK_REGISTERED);
>+	if (devlink && !refcount_inc_not_zero(&devlink->refcount))
>+		devlink = NULL;
>+	xa_unlock(&devlinks);
>+
>+	return devlink ?: devlink_iter_next(index);
>+}
>+
>+static struct devlink *devlink_iter_start(unsigned long *index)
>+{
>+	struct devlink *devlink;
>+
>+	xa_lock(&devlinks);
>+	devlink = xa_find(&devlinks, index, ULONG_MAX, DEVLINK_REGISTERED);
>+	if (devlink && !refcount_inc_not_zero(&devlink->refcount))
>+		devlink = NULL;
>+	xa_unlock(&devlinks);
>+
>+	return devlink ?: devlink_iter_next(index);
>+}
>+
>+#define devlink_for_each_get(index, entry)			\
>+	for (index = 0, entry = devlink_iter_start(&index);	\
>+	     entry; entry = devlink_iter_next(&index))
>+
> static struct devlink *devlink_get_from_attrs(struct net *net,
> 					      struct nlattr **attrs)
> {
>@@ -1329,10 +1361,7 @@ static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
> 	int err = 0;
> 
> 	mutex_lock(&devlink_mutex);
>-	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>-		if (!devlink_try_get(devlink))
>-			continue;
>-
>+	devlink_for_each_get(index, devlink) {
> 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
> 			goto retry;
> 
>etc.
>
>Plus we need to be more careful about the unregistering order, I
>believe the correct ordering is:
>
>	clear_unmark()
>	put()
>	wait()
>	notify()
>
>but I believe we'll run afoul of Leon's notification suppression.
>So I guess notify() has to go before clear_unmark(), but we should
>unmark before we wait otherwise we could live lock (once the mutex 
>is really gone, I mean).

Will check.