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: <20220720174953.707bcfa9@kernel.org>
Date:   Wed, 20 Jul 2022 17:49:53 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
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

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