netdev - Re: [PATCH net-next v6] net: af_packet: Use hrtimer to do the retire operation

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] [day] [month] [year] [list]

Message-ID: <willemdebruijn.kernel.60ba954529f7@gmail.com>
Date: Fri, 22 Aug 2025 02:37:45 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Xin Zhao <jackzxcui1989@....com>, 
 willemdebruijn.kernel@...il.com, 
 edumazet@...gle.com, 
 ferenc@...es.dev
Cc: davem@...emloft.net, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 horms@...nel.org, 
 netdev@...r.kernel.org, 
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6] net: af_packet: Use hrtimer to do the retire
 operation

Xin Zhao wrote:
> On Thu, 2025-08-21 at 22:32 +0800, Willem wrote:
> 
> > Thanks for the analysis.
> > 
> > Using hrtimer_start from within the callback that returns
> > HRTIMER_RESTART does not sound in line with the intention of the API
> > to me.
> > 
> > I think we should just adjust and restart from within the callback and
> > hrtimer_start from tpacket_rcv iff the timer is not yet queued.
> > 
> > Since all these modifications are made while the receive queue lock is
> > held I don't immediately see why we would need additional mutual
> > exclusion beyond that.
> 
> 
> The hrtimer callback is called by __run_hrtimer, if we only use hrtimer_forward_now in the callback,
> it will not restart the time within the callback. The timer will be enqueued after the callback
> return. So when the timer is being enqueued, it is not protected by sk_receive_queue.lock.

I see.
 
> Consider the following timing sequence:
> timer   cpu0 (softirq context, hrtimer timeout)                cpu
> 0       hrtimer_run_softirq
> 1         __hrtimer_run_queues
> 2           __run_hrtimer
> 3             prb_retire_rx_blk_timer_expired
> 4               spin_lock(&po->sk.sk_receive_queue.lock);
> 5               _prb_refresh_rx_retire_blk_timer
> 6                 hrtimer_forward_now
> 7               spin_unlock(&po->sk.sk_receive_queue.lock)
> 8             raw_spin_lock_irq(&cpu_base->lock);              tpacket_rcv
> 9             enqueue_hrtimer                                    spin_lock(&sk->sk_receive_queue.lock);
> 10                                                               packet_current_rx_frame
> 11                                                                 __packet_lookup_frame_in_block
> 12            finish enqueue_hrtimer                                 prb_open_block
> 13                                                                     _prb_refresh_rx_retire_blk_timer
> 14                                                                       hrtimer_is_queued(&pkc->retire_blk_timer) == true
> 15                                                                       hrtimer_forward_now
> 16                                                                         WARN_ON
> 
> On cpu0 in the timing sequence above, enqueue_hrtimer is not protected by sk_receive_queue.lock,
> while the hrtimer_forward_now is not protected by raw_spin_lock_irq(&cpu_base->lock).
> 
> It will cause WARN_ON if we only use 'hrtimer_is_queued(&pkc->retire_blk_timer) == true' to check
> whether to call hrtimer_forward_now.

One way around this may be to keep the is_timer_queued state inside
tpacket_kbdq_core protected by a relevant lock, like the receive queue
lock. Similar to pkc->delete_blk_timer.

Admittedly I have not given this much thought yet. Am traveling for a
few days, have limited time.
 
> 
> Thanks
> Xin Zhao
>