| 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: Thu, 21 Jul 2022 07:45:00 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"idosch@...dia.com" <idosch@...dia.com>,
"petrm@...dia.com" <petrm@...dia.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"mlxsw@...dia.com" <mlxsw@...dia.com>,
"saeedm@...dia.com" <saeedm@...dia.com>,
"snelson@...sando.io" <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 12:25:54AM CEST, jacob.e.keller@...el.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@...nulli.us>
>> Sent: Wednesday, July 20, 2022 8:12 AM
>> To: netdev@...r.kernel.org
>> Cc: davem@...emloft.net; kuba@...nel.org; idosch@...dia.com;
>> petrm@...dia.com; pabeni@...hat.com; edumazet@...gle.com;
>> mlxsw@...dia.com; saeedm@...dia.com; snelson@...sando.io
>> Subject: [patch net-next v3 01/11] net: devlink: make sure that devlink_try_get()
>> works with valid pointer during xarray iteration
>>
>> From: Jiri Pirko <jiri@...dia.com>
>>
>> Remove dependency on devlink_mutex during devlinks xarray iteration.
>>
>> The reason is that devlink_register/unregister() functions taking
>> devlink_mutex would deadlock during devlink reload operation of devlink
>> instance which registers/unregisters nested devlink instances.
>>
>> The devlinks xarray consistency is ensured internally by xarray.
>> There is a reference taken when working with devlink using
>> devlink_try_get(). But there is no guarantee that devlink pointer
>> picked during xarray iteration is not freed before devlink_try_get()
>> is called.
>>
>> Make sure that devlink_try_get() works with valid pointer.
>> Achieve it by:
>> 1) Splitting devlink_put() so the completion is sent only
>> after grace period. Completion unblocks the devlink_unregister()
>> routine, which is followed-up by devlink_free()
>> 2) Iterate the devlink xarray holding RCU read lock.
>>
>> Signed-off-by: Jiri Pirko <jiri@...dia.com>
>
>
>This makes sense as long as its ok to drop the rcu_read_lock while in the body of the xa loops. That feels a bit odd to me...
Yes, it is okay. See my comment below.
>
>> ---
>> v2->v3:
>> - s/enf/end/ in devlink_put() comment
>> - added missing rcu_read_lock() call to info_get_dumpit()
>> - extended patch description by motivation
>> - removed an extra "by" from patch description
>> v1->v2:
>> - new patch (originally part of different patchset)
>> ---
>> net/core/devlink.c | 114 ++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 96 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 98d79feeb3dc..6a3931a8e338 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -70,6 +70,7 @@ struct devlink {
>> u8 reload_failed:1;
>> refcount_t refcount;
>> struct completion comp;
>> + struct rcu_head rcu;
>> char priv[] __aligned(NETDEV_ALIGN);
>> };
>>
>> @@ -221,8 +222,6 @@ static DEFINE_XARRAY_FLAGS(devlinks,
>> XA_FLAGS_ALLOC);
>> /* devlink_mutex
>> *
>> * An overall lock guarding every operation coming from userspace.
>> - * It also guards devlink devices list and it is taken when
>> - * driver registers/unregisters it.
>> */
>> static DEFINE_MUTEX(devlink_mutex);
>>
>> @@ -232,10 +231,21 @@ struct net *devlink_net(const struct devlink *devlink)
>> }
>> EXPORT_SYMBOL_GPL(devlink_net);
>>
>> +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);
>> }
>>
>> struct devlink *__must_check devlink_try_get(struct devlink *devlink)
>> @@ -295,6 +305,7 @@ static struct devlink *devlink_get_from_attrs(struct net
>> *net,
>>
>> lockdep_assert_held(&devlink_mutex);
>>
>> + rcu_read_lock();
>> xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>> if (strcmp(devlink->dev->bus->name, busname) == 0 &&
>> strcmp(dev_name(devlink->dev), devname) == 0 &&
>> @@ -306,6 +317,7 @@ static struct devlink *devlink_get_from_attrs(struct net
>> *net,
>>
>> if (!found || !devlink_try_get(devlink))
>> devlink = ERR_PTR(-ENODEV);
>> + rcu_read_unlock();
>>
>> return devlink;
>> }
>> @@ -1329,9 +1341,11 @@ static int devlink_nl_cmd_rate_get_dumpit(struct
>> sk_buff *msg,
>> int err = 0;
>>
>> mutex_lock(&devlink_mutex);
>> + rcu_read_lock();
>> xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>> if (!devlink_try_get(devlink))
>> continue;
>> + rcu_read_unlock();
>>
>> if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>> goto retry;
>> @@ -1358,7 +1372,9 @@ static int devlink_nl_cmd_rate_get_dumpit(struct
>> sk_buff *msg,
>> devl_unlock(devlink);
>> retry:
>> devlink_put(devlink);
>> + rcu_read_lock();
>> }
>> + rcu_read_unlock();
>> out:
>> mutex_unlock(&devlink_mutex);
>> if (err != -EMSGSIZE)
>> @@ -1432,29 +1448,32 @@ static int devlink_nl_cmd_get_dumpit(struct sk_buff
>> *msg,
>> int err;
>>
>> mutex_lock(&devlink_mutex);
>> + rcu_read_lock();
>> xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>> if (!devlink_try_get(devlink))
>> continue;
>> + rcu_read_unlock();
>>
>
>Is it safe to rcu_read_unlock here while we're still in the middle of the array processing? What happens if something else updates the xarray? is the for_each_marked safe?
Sure, you don't need to hold rcu_read_lock during call to xa_for_each_marked.
The consistency of xarray is itself guaranteed. The only reason to take
rcu_read_lock outside is that the devlink pointer which is
rcu_dereference_check()'ed inside xa_for_each_marked() is still valid
once we devlink_try_get() it.
>
>> - if (!net_eq(devlink_net(devlink), sock_net(msg->sk))) {
>> - devlink_put(devlink);
>> - continue;
>> - }
>> + if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>> + goto retry;
>>
>
>Ahh retry is at the end of the loop, so we'll just skip this one and move to the next one without needing to duplicate both devlink_put and rcu_read_lock.. ok.
Yep.
>
>> - if (idx < start) {
>> - idx++;
>> - devlink_put(devlink);
>> - continue;
>> - }
>> + if (idx < start)
>> + goto inc;
>>
>> err = devlink_nl_fill(msg, devlink, DEVLINK_CMD_NEW,
>> NETLINK_CB(cb->skb).portid,
>> cb->nlh->nlmsg_seq, NLM_F_MULTI);
>> - devlink_put(devlink);
>> - if (err)
>> + if (err) {
>> + devlink_put(devlink);
>> goto out;
>> + }
>> +inc:
>> idx++;
>> +retry:
>> + devlink_put(devlink);
>> + rcu_read_lock();
>> }
>> + rcu_read_unlock();
>> out:
>> mutex_unlock(&devlink_mutex);
>>
[...]
Powered by blists - more mailing lists