netdev - Re: [patch net-next] devlink: don't take instance lock for nested handle put

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: <ZSakg8W+SBgahXtW@nanopsycho>
Date: Wed, 11 Oct 2023 15:34:59 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, davem@...emloft.net,
	edumazet@...gle.com, gal@...dia.com
Subject: Re: [patch net-next] devlink: don't take instance lock for nested
 handle put

Tue, Oct 10, 2023 at 08:16:05PM CEST, kuba@...nel.org wrote:
>On Tue, 10 Oct 2023 17:56:36 +0200 Jiri Pirko wrote:
>> >You understand what I'm saying tho, right?
>> >
>> >If we can depend on the parent not disappearing before the child,
>> >and the hierarchy is a DAG - the locking is much easier, because
>> >parent can lock the child.  
>> 
>> It won't help with the locking though. During GET, the devlink lock
>> is taken and within it, you need to access the nested devlink attributes.
>> 
>> And during reload->notify, we still need work so the lock are taken in
>> proper order.
>
>If parent is guaranteed to exist the read only fields can be accessed
>freely and the read-write fields can be cached on children.

Only reason to access parent currently is netns change notification.
See devlink_rel_nested_in_notify().
It basically just scheduled delayed work by calling:
devlink_rel_nested_in_notify_work_schedule().

When work is processed in
devlink_rel_nested_in_notify_work()
There is no guarantee the parent exists, therefore devlink_index is used
to get the instance and then obj_index to get port/linecard index.

notify_cb() basically sends notification of parent object and that needs
parent instance lock. <--- This is why you need to lock the parent.

I see no way how to cache anything on children as you describe in this
scenario.


>Parent has a list of children, it can store/cache a netns pointer on all
>of them. When reload happens lock them and update that pointer.
>At which point children do not have to lock the parent.

Access of netns pointer is not a problem. See my latest version (v2)
where rcu is used in order to make sure peernet2id_alloc() call is safe:

devlink: call peernet2id_alloc() with net pointer under RCU read lock

       rcu_read_lock();
       devl_net = read_pnet_rcu(&devlink->_net);
       if (!net_eq(net, devl_net)) {
               int id = peernet2id_alloc(net, devl_net, GFP_ATOMIC);

               rcu_read_unlock();
               if (nla_put_s32(msg, DEVLINK_ATTR_NETNS_ID, id))
                       return -EMSGSIZE;
       } else {
               rcu_read_unlock();
       }


>
>> It would only make the rel infrastructure a bit similer. I will look
>> into that. But it's parallel to this patchset really.
>