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

[Antonio Quartulli <antonio@openvpn.net>]

Hi Eric,

On 30/04/2026 15:37, Eric Dumazet wrote:
> On Thu, Apr 30, 2026 at 6:28 AM Antonio Quartulli <antonio@openvpn.net> wrote:
>>
>> 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?
> 
> Sashiko suggestion seems good to me.

But am I right saying that this bug existed before and it is not 
introduced by this patch?

A concurrent softirq (UDP RX pkt) could already trigger this problem 
before we introduced the local_bh_disable/enable() calls, right?


Regards,


> 
> Proper Fixes: tag would be:
> 
> Fixes: ab66abbc769b ("ovpn: implement basic RX path (UDP)")

-- 
Antonio Quartulli
OpenVPN Inc.