netdev - Re: [PATCH net-next v4] 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-next>] [day] [month] [year] [list]

Message-Id: <20250820071914.2029093-1-jackzxcui1989@163.com>
Date: Wed, 20 Aug 2025 15:19:14 +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 v4] net: af_packet: Use hrtimer to do the retire operation

On Tue, 2025-08-19 at 22:18 +0800, Willem wrote:

> > -static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc)
> > +static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc,
> > +					     bool start, bool callback)
> >  {
> > -	mod_timer(&pkc->retire_blk_timer,
> > -			jiffies + pkc->tov_in_jiffies);
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> 
> The two environments that can race are the timer callback running in
> softirq context or the open_block from tpacket_rcv in process context.
> 
> So worst case the process context path needs to disable bh?
> 
> As you pointed out, the accesses to the hrtimer fields are already
> protected, by the caller holding sk.sk_receive_queue.lock.
> 
> So it should be sufficient to just test hrtimer_is_queued inside that
> critical section before calling hrtimer_start?
> 
> Side-note: tpacket_rcv calls spin_lock, not spin_lock_bh. But if the
> same lock can also be taken in softirq context, the process context
> caller should use the _bh variant. This is not new with your patch.
> Classical timers also run in softirq context. I may be overlooking
> something, will need to take a closer look at that.
> 
> In any case, I don't think local_irq_save is needed. 

Indeed, there is no need to use local_irq_save. The use case I referenced from
perf_mux_hrtimer_restart is different from ours. Our timer callback does not run in
hard interrupt context, so it is unnecessary to use local_irq_save. I will make this
change in PATCH v6.



On Wed, 2025-08-20 at 4:21 +0800, Willem wrote:
 
> > So worst case the process context path needs to disable bh?
> > 
> > As you pointed out, the accesses to the hrtimer fields are already
> > protected, by the caller holding sk.sk_receive_queue.lock.
> > 
> > So it should be sufficient to just test hrtimer_is_queued inside that
> > critical section before calling hrtimer_start?
> > 
> > Side-note: tpacket_rcv calls spin_lock, not spin_lock_bh. But if the
> > same lock can also be taken in softirq context, the process context
> > caller should use the _bh variant. This is not new with your patch.
> > Classical timers also run in softirq context. I may be overlooking
> > something, will need to take a closer look at that.
> > 
> > In any case, I don't think local_irq_save is needed. 
> 
> 
> 
> 
> I meant prb_open_block
> 
> tpacket_rcv runs in softirq context (from __netif_receive_skb_core)
> or with bottom halves disabled (from __dev_queue_xmit, or if rx uses
> napi_threaded).
> 
> That is likely why the spin_lock_bh variant is not explicitly needed.

Before I saw your reply, I was almost considering replacing spin_lock with
spin_lock_bh in our project before calling packet_current_rx_frame in
tpacket_rcv. I just couldn't understand why we haven't encountered any
deadlocks or RCU issues due to not properly adding _bh in our project until
I saw your reply.
I truly admire your ability to identify all the scenarios that use the
tpacket_rcv function in such a short amount of time. For me, finding all the
instances where tpacket_rcv is assigned to prot_hook.func for proxy calls is
a painful and lengthy task. Even if I manage to find them, I would still
worry about missing some.


Thanks
Xin Zhao