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: <9a1ededa-8b1f-4118-94b4-d69df766c61e@ovn.org>
Date: Mon, 10 Mar 2025 15:17:17 +0100
From: Ilya Maximets <i.maximets@....org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 netdev@...r.kernel.org, linux-rt-devel@...ts.linux.dev
Cc: 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>, i.maximets@....org
Subject: Re: [ovs-dev] [PATCH net-next 11/18] openvswitch: Use nested-BH
 locking for ovs_actions.

On 3/9/25 15:46, Sebastian Andrzej Siewior wrote:
> ovs_actions is a per-CPU variable and relies on disabled BH for its
> locking. Without per-CPU locking in local_bh_disable() on PREEMPT_RT
> this data structure requires explicit locking.
> The data structure can be referenced recursive and there is a recursion
> counter to avoid too many recursions.
> 
> Add a local_lock_t to the data structure and use
> local_lock_nested_bh() for locking. Add an owner of the struct which is
> the current task and acquire the lock only if the structure is not owned
> by the current task.
> 
> Cc: Pravin B Shelar <pshelar@....org>
> Cc: dev@...nvswitch.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>  net/openvswitch/actions.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 322ca7b30c3bc..c4131e04c1284 100644
> --- 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?

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?

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

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.

Best regards, Ilya Maximets.