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

[Jakub Kicinski <kuba@kernel.org>]

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.

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).