linux-kernel - 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <20250821153017.3607708-1-jackzxcui1989@163.com>
Date: Thu, 21 Aug 2025 23:30:17 +0800
From: Xin Zhao <jackzxcui1989@....com>
To: 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

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.

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.


Thanks
Xin Zhao