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

[Jakub Kicinski <kuba@kernel.org>]

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