netdev - Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

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: <20230102145654.24aec331@kernel.org>
Date:   Mon, 2 Jan 2023 14:56:54 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     jacob.e.keller@...el.com, leon@...nel.org, netdev@...r.kernel.org,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [RFC net-next 01/10] devlink: bump the instance index directly
 when iterating

On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote:
> >-struct devlink *
> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
> >-					  unsigned long, xa_mark_t))
> >+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> > {
> >-	struct devlink *devlink;
> >+	struct devlink *devlink = NULL;
> > 
> > 	rcu_read_lock();
> > retry:
> >-	devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
> >+	devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
> > 	if (!devlink)
> > 		goto unlock;
> > 
> >@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> > 	 * This prevents live-lock of devlink_unregister() wait for completion.
> > 	 */
> > 	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
> >-		goto retry;
> >+		goto next;
> > 
> >-	/* For a possible retry, the xa_find_after() should be always used */
> >-	xa_find_fn = xa_find_after;  
> 
> Hmm. Any idea why xa_find_after()? implementation is different to
> xa_find()?

I'm guessing it's because for _after the code needs to take special
care of skipping multi-index entries. If an entry spans 0-3 xa_find(0)
can return the same entry as xa_find(1). But xa_find_after(1) must
return an entry under index 4 or higher.

Since our use of the Xarray is very trivial with no range indexes,
it should not matter.

Let me CC Matthew, just in case. The question boils down to whether:

	for (index = 0; (entry = xa_find(net, &index));	index++)  

is a legal way of iterating over an Xarray without range indexes.

> > 	if (!devlink_try_get(devlink))
> >-		goto retry;
> >+		goto next;
> > 	if (!net_eq(devlink_net(devlink), net)) {
> > 		devlink_put(devlink);
> >-		goto retry;
> >+		goto next;
> > 	}
> > unlock:
> > 	rcu_read_unlock();
> > 	return devlink;
> >-}
> >-
> >-struct devlink *
> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp)
> >-{
> >-	return devlinks_xa_find_get(net, indexp, xa_find);
> >-}
> > 
> >-struct devlink *
> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp)
> >-{
> >-	return devlinks_xa_find_get(net, indexp, xa_find_after);
> >+next:
> >+	(*indexp)++;
> >+	goto retry;
> > }
> > 
> > /**
> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> >index 1d7ab11f2f7e..ef0369449592 100644
> >--- a/net/devlink/devl_internal.h
> >+++ b/net/devlink/devl_internal.h
> >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
> >  * in loop body in order to release the reference.
> >  */
> > #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
> >-	for (index = 0,							\
> >-	     devlink = devlinks_xa_find_get_first(net, &index);	\
> >-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
> >-
> >-struct devlink *
> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
> >-					  unsigned long, xa_mark_t));
> >-struct devlink *
> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
> >-struct devlink *
> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
> >+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)