Re: [PATCH net 2/2] ovpn: ensure gro_cells_receive() is invoked with BH disabled

[Antonio Quartulli <antonio@openvpn.net>]

Hi Jakub,

sashiko came back with an interesting review of the per-cpu stats update 
in the surrounding code.

As far as I can tell its explanation makes sense, but I am no per-cpu 
expert.

IIUC it basically says that if gro_cells_receive() is invoked with 
bottom halves disabled, the following dev_dstats_rx_add() should be too 
to avoid deadlocks and corruptions.

See below:

On 29/04/2026 14:01, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index d92bb87be2b2..c0fdb9504241 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -91,7 +91,13 @@ static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb)
>   
>   	/* cause packet to be "received" by the interface */
>   	pkt_len = skb->len;
> +	/* we may get here in process context in case of TCP connections,
> +	 * therefore we have to disable BHs to ensure gro_cells_receive()
> +	 * doesn't enter deadlock
> +	 */
> +	local_bh_disable();
>   	ret = gro_cells_receive(&peer->ovpn->gro_cells, skb);
> +	local_bh_enable();
>   	if (likely(ret == NET_RX_SUCCESS)) {
>   		/* update RX stats with the size of decrypted packet */
>   		ovpn_peer_stats_increment_rx(&peer->vpn_stats, pkt_len);
 >>		dev_dstats_rx_add(peer->ovpn->dev...) << not visible in the diff, 
but it's here

from sashiko:

Can this lead to seqcount deadlocks and stat corruption since
dev_dstats_rx_add() is executed with preemption and bottom-halves enabled?
dev_dstats_rx_add() operates on per-CPU statistics and protects 64-bit
updates with a sequence counter (u64_stats_update_begin()). When called
from process context with BH enabled, on 64-bit systems, this_cpu_ptr()
is evaluated with preemption enabled. If the task is migrated mid-update,
it risks cross-CPU stat corruption.
On 32-bit systems, u64_stats_update_begin() disables preemption but not
bottom-halves. If a softirq (e.g., a concurrent UDP packet reception)
interrupts the process context and calls dev_dstats_rx_add() for the same
interface, it will re-enter the seqcount writer lock on the exact same CPU.
This corrupts the sequence counter, causing readers to see an unlocked
sequence during active writes, leading to torn reads and corrupted stats.
Should local_bh_enable() be moved after the statistics updates to ensure
the entire per-CPU update is atomic with respect to softirqs?


Do you have an opinion?

Thanks a lot.

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.