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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250310144459.wjPdPtUo@linutronix.de>
Date: Mon, 10 Mar 2025 15:44:59 +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>
Subject: Re: [ovs-dev] [PATCH net-next 11/18] openvswitch: Use nested-BH
 locking for ovs_actions.

On 2025-03-10 15:17:17 [+0100], Ilya Maximets wrote:
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -82,6 +82,8 @@ struct ovs_action {
> >  	struct action_fifo action_fifos;
> >  	struct action_flow_keys flow_keys;
> >  	int exec_level;
> > +	struct task_struct *owner;
> > +	local_lock_t bh_lock;
> >  };
> >  
> >  static DEFINE_PER_CPU(struct ovs_action, ovs_actions);
> > @@ -1690,8 +1692,14 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >  			const struct sw_flow_actions *acts,
> >  			struct sw_flow_key *key)
> >  {
> > +	struct ovs_action *ovs_act = this_cpu_ptr(&ovs_actions);
> >  	int err, level;
> >  
> > +	if (ovs_act->owner != current) {
> > +		local_lock_nested_bh(&ovs_actions.bh_lock);
> 
> Wouldn't this cause a warning when we're in a syscall/process context?

My understanding is that is only invoked in softirq context. Did I
misunderstood it? Otherwise that this_cpu_ptr() above should complain
that preemption is not disabled and if preemption is indeed not disabled
how do you ensure that you don't get preempted after the
__this_cpu_inc_return() in several tasks (at the same time) leading to
exceeding the OVS_RECURSION_LIMIT?

> We will also be taking a spinlock in a general case here, which doesn't
> sound particularly great, since we can potentially be holding it for a
> long time and it's also not free to take/release on this hot path.
> Is there a version of this lock that's a no-op on non-RT?

local_lock_nested_bh() does not acquire any lock on !PREEMPT_RT. It only
verifies that in_softirq() is true.

> > +		ovs_act->owner = current;
> > +	}
> > +
> >  	level = __this_cpu_inc_return(ovs_actions.exec_level);
> >  	if (unlikely(level > OVS_RECURSION_LIMIT)) {
> >  		net_crit_ratelimited("ovs: recursion limit reached on datapath %s, probable configuration error\n",
> > @@ -1710,5 +1718,10 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >  
> >  out:
> >  	__this_cpu_dec(ovs_actions.exec_level);
> > +
> > +	if (level == 1) {
> > +		ovs_act->owner = NULL;
> > +		local_unlock_nested_bh(&ovs_actions.bh_lock);
> > +	}
> 
> Seems dangerous to lock every time the owner changes but unlock only
> once on level 1.  Even if this works fine, it seems unnecessarily
> complicated.  Maybe it's better to just lock once before calling
> ovs_execute_actions() instead?

My understanding is this can be invoked recursively. That means on first
invocation owner == NULL and then you acquire the lock at which point
exec_level goes 0->1. On the recursive invocation owner == current and
you skip the lock but exec_level goes 1 -> 2.
On your return path once level becomes 1, then it means that dec made it
go 1 -> 0, you unlock the lock.
The locking part happens only on PREEMPT_RT because !PREEMPT_RT has
softirqs disabled which guarantee that there will be no preemption.

tools/testing/selftests/net/openvswitch should cover this?

> 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?

> Best regards, Ilya Maximets.

Sebastian