netdev - Re: [ovs-dev] [PATCH net-next 11/18] openvswitch: Use nested-BH locking for ovs

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250313140220.Lyr40G7J@linutronix.de>
Date: Thu, 13 Mar 2025 15:02:20 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Ilya Maximets <i.maximets@....org>
Cc: netdev@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
	dev@...nvswitch.org, Eric Dumazet <edumazet@...gle.com>,
	Simon Horman <horms@...nel.org>, Jakub Kicinski <kuba@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Paolo Abeni <pabeni@...hat.com>,
	"David S. Miller" <davem@...emloft.net>,
	Aaron Conole <aconole@...hat.com>,
	Eelco Chaudron <echaudro@...hat.com>
Subject: Re: [ovs-dev] [PATCH net-next 11/18] openvswitch: Use nested-BH
 locking for ovs_actions.

On 2025-03-13 14:23:16 [+0100], Ilya Maximets wrote:
> > originate from within the recursion.
> 
> It's true that ovs_packet_cmd_execute() can not be re-intered, while
> ovs_dp_process_packet() can be re-entered if the packet leaves OVS and
> then comes back from another port.  It's still better to handle all the
> locking within datapath.c and not lock for RT in actions.c and for non-RT
> in datapath.c.

Okay.

> >>>> Also, the name of the struct ovs_action doesn't make a lot of sense,
> >>>> I'd suggest to call it pcpu_storage or something like that instead.
> >>>> I.e. have a more generic name as the fields inside are not directly
> >>>> related to each other.
> >>>
> >>> Understood. ovs_pcpu_storage maybe?
> >>
> >> It's OK, I guess, but see also a point about locking inside datapath.c
> >> instead and probably not needing to change anything in actions.c.
> > 
> > If you say that adding a lock to ovs_dp_process_packet() and another to
> > ovs_packet_cmd_execute() then I can certainly update. However based on
> > what I wrote above, I am not sure.
> 
> I think, it's better if we keep all the locks in datapath.c and let
> actions.c assume that all the operations are always safe as it was
> originally intended.

If you say so. Then I move the logic to the two callers to datapath.c
then. But I would need the same recursive lock-detection as I currently
have in ovs_dp_process_packet(). That means we would have the lock
datapath.c and the data structure it protects in actions.c.

> Cc: Aaron and Eelco, in case they have some thoughts on this as well.

While at it, I would keep "openvswitch: Merge three per-CPU structures
into one." since it looks like a nice clean up.

> Best regards, Ilya Maximets.

Sebastian