Re: [PATCH net 2/3] ovpn: ensure packet delivery happens with BH disabled

Re: [PATCH net 2/3] ovpn: ensure packet delivery happens with BH disabled

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ovpn: ensure packet delivery happens with BH disabled

This commit wraps gro_cells_receive() and dev_dstats_rx_add() with
local_bh_disable() and local_bh_enable() in ovpn_netdev_write(). This
prevents deadlocks and per-cpu state corruption when packet delivery occurs
in process context for TCP connections.

> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -91,12 +91,18 @@ 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()
> +	 * and dev_dstats_rx_add() do not get corrupted or enter deadlock
> +	 */
> +	local_bh_disable();
>  	ret = gro_cells_receive(&peer->ovpn->gro_cells, skb);
>  	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, pkt_len);
>  	}
> +	local_bh_enable();
>  }

This protects the success path, but do the error and drop paths also need
BH protection?

If ovpn_decrypt_post() is called in process context for TCP connections, an
error condition jumps to the drop label:

ovpn_decrypt_post() {
        ...
        if (unlikely(ret < 0))
                goto drop;
        ...
drop:
        if (unlikely(skb))
                dev_dstats_rx_dropped(peer->ovpn->dev);
        ...
}

Since dev_dstats_rx_dropped() updates the same per-CPU dstats structure
without disabling bottom halves, could it still be vulnerable to softirq
preemption and stat corruption?

Additionally, do the TX paths in the TCP implementation have similar issues?
It looks like ovpn_tcp_send_skb() and ovpn_tcp_send_sock_skb() might call
dev_dstats_tx_dropped() without disabling bottom halves.

Similarly, ovpn_tcp_send_sock() uses preempt_disable() around
dev_dstats_tx_add(), which does not prevent softirqs from running upon
return from a hardware interrupt on the same CPU. Does this leave a similar
race window open?
-- 
pw-bot: cr

Re: [PATCH net 2/3] ovpn: ensure packet delivery happens with BH disabled

[Antonio Quartulli]

Hi Jakub

On 06/05/2026 03:00, Jakub Kicinski wrote:
[...]
> This protects the success path, but do the error and drop paths also need
> BH protection?
> 
> If ovpn_decrypt_post() is called in process context for TCP connections, an
> error condition jumps to the drop label:
> 
> ovpn_decrypt_post() {
>          ...
>          if (unlikely(ret < 0))
>                  goto drop;
>          ...
> drop:
>          if (unlikely(skb))
>                  dev_dstats_rx_dropped(peer->ovpn->dev);
>          ...
> }
> 
> Since dev_dstats_rx_dropped() updates the same per-CPU dstats structure
> without disabling bottom halves, could it still be vulnerable to softirq
> preemption and stat corruption?
> 

Actually we were already looking into this.
However, since this needs a separate analysis, I wanted to get this 
fixed in a follow up patch.

Would it be ok to pull this PR as is, so we don't hold back the 
outstanding fixes?

Then we will address the issue highlighted by Sashiko in a new patch.
The problem is similar, but may need to be fixed differently.


Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net 2/3] ovpn: ensure packet delivery happens with BH disabled

[Jakub Kicinski]

On Wed, 6 May 2026 11:00:20 +0200 Antonio Quartulli wrote:
> > ovpn_decrypt_post() {
> >          ...
> >          if (unlikely(ret < 0))
> >                  goto drop;
> >          ...
> > drop:
> >          if (unlikely(skb))
> >                  dev_dstats_rx_dropped(peer->ovpn->dev);
> >          ...
> > }
> > 
> > Since dev_dstats_rx_dropped() updates the same per-CPU dstats structure
> > without disabling bottom halves, could it still be vulnerable to softirq
> > preemption and stat corruption?
> >   
> 
> Actually we were already looking into this.
> However, since this needs a separate analysis, I wanted to get this 
> fixed in a follow up patch.
> 
> Would it be ok to pull this PR as is, so we don't hold back the 
> outstanding fixes?
> 
> Then we will address the issue highlighted by Sashiko in a new patch.
> The problem is similar, but may need to be fixed differently.


Ugh, fair. The way the AI formatted the output made me think
it's an error path in the same function, which would make sense 
to address with a single patch.. Pulling now.