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] [day] [month] [year] [list]

Message-ID: <fd4c8167-0c2d-4f5f-bf70-1efcdf3de2fb@ovn.org>
Date: Mon, 10 Mar 2025 17:56:09 +0100
From: Ilya Maximets <i.maximets@....org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: i.maximets@....org, 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 3/10/25 15:44, Sebastian Andrzej Siewior wrote:
> 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?

It can be called from the syscall/process context while processing
OVS_PACKET_CMD_EXECUTE request.

> 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 disable BH in this case, so it should be safe (on non-RT).  See the
ovs_packet_cmd_execute() for more details.

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

Ah, you're right.  It does more things, but only under lock debug.

> 
>>> +		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.

My point is: why locking here with some extra non-obvious logic of owner
tracking if we can lock (unconditionally?) in ovs_packet_cmd_execute() and
ovs_dp_process_packet() instead?  We already disable BH in one of those
and take appropriate RCU locks in both.  So, feels like a better place
for the extra locking if necessary.  We will also not need to move around
any code in actions.c if the code there is guaranteed to be safe by holding
locks outside of it.

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

It's not a comprehensive test suite, it covers some cases, but it
doesn't test anything related to preemptions specifically.

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

> 
>> Best regards, Ilya Maximets.
> 
> Sebastian