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]

Date: Fri, 13 Oct 2023 13:01:00 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
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

On Fri, 13 Oct 2023 19:07:05 +0200 Jiri Pirko wrote:
> >> Not sure what obvious bug you mean. If you mean the parent-child
> >> lifetime change, I don't know how that would help here. I don't see how.
> >> 
> >> Plus it has performance implications. When user removes SF port under
> >> instance lock, the SF itself is removed asynchonously out of the lock.
> >> You suggest to remove it synchronously holding the instance lock,
> >> correct?   
> >
> >The SF is deleted by calling ->port_del() on the PF instance, correct?  
> 
> That or setting opstate "inactive".

The opstate also set on the port (i.e. from the PF), right?

> >> SF removal does not need that lock. Removing thousands of SFs
> >> would take much longer as currently, they are removed in parallel.
> >> You would serialize the removals for no good reason.  
> >
> >First of all IDK what the removal rate you're targeting is, and what
> >is achievable under PF's lock. Handwaving "we need parallelism" without
> >data is not a serious argument.  
> 
> Oh there are data and there is a need. My colleagues are working
> on parallel creation/removal within mlx5 driver as we speak. What you
> suggest would be huge setback :/

The only part that needs to be synchronous is un-linking.
Once the SF is designated for destruction we can live without the link,
it's just waiting to be garbage-collected.

> >> Not sure what you mean by that. Locking is quite clear. Why weird?
> >> What's weird exactly? What do you mean by "random dependencies"?
> >> 
> >> I have to say I feel we got a bit lost in the conversation.  
> >
> >You have a rel object, which is refcounted, xarray with a lock, and 
> >an async work for notifications.  
> 
> Yes. The async work for notification is something you would need anyway,
> even with object lifetime change you suggest. It's about locking order.

I don't think I would. If linking is always done under PF's lock we can
safely send any ntf.

> Please see the patchset I sent today (v3), I did put in a documentation
> describing that (3 last patches). That should make it clear.

It's unnecessarily complicated, but whatever, I'm not touching it.