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
| ||
|
Date: Fri, 22 Jul 2022 08:15:03 +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. > >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() Fixed. > >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).
Powered by blists - more mailing lists