Re: [PATCH net 2/2] net: macb: Protect access to net_device::in_ptr with RCU lock

[Kevin Hao <haokexin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3613 bytes --]

On Tue, Mar 17, 2026 at 04:54:11PM +0100, Théo Lebrun wrote:
> On Tue Mar 17, 2026 at 2:27 AM CET, Kevin Hao wrote:
> > On Mon, Mar 16, 2026 at 06:59:35PM +0100, Théo Lebrun wrote:
> >> On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
> >> > @@ -5915,13 +5915,16 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >> >  
> >> >  	if (bp->wol & MACB_WOL_ENABLED) {
> >> >  		/* Check for IP address in WOL ARP mode */
> >> > +		rcu_read_lock();
> >> >  		idev = __in_dev_get_rcu(bp->dev);
> >> >  		if (idev)
> >> >  			ifa = rcu_dereference(idev->ifa_list);
> >> >  		if ((bp->wolopts & WAKE_ARP) && !ifa) {
> >> >  			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
> >> > +			rcu_read_unlock();
> >> >  			return -EOPNOTSUPP;
> >> >  		}
> >> > +
> >> >  		spin_lock_irqsave(&bp->lock, flags);
> >> >  
> >> >  		/* Disable Tx and Rx engines before  disabling the queues,
> >> > @@ -5963,6 +5966,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >> >  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
> >> >  		}
> >> >  		spin_unlock_irqrestore(&bp->lock, flags);
> >> > +		rcu_read_unlock();
> >> >  
> >> >  		/* Change interrupt handler and
> >> >  		 * Enable WoL IRQ on queue 0
> >> 
> >> Instead of making the RCU critical section extend so much, you could
> >> dereference ifa->ifa_local into a stack variable. In particular, it
> >> would avoid the RCU critical section covering a spinlock critical
> >> section.
> >
> > I initially considered using a local variable before submitting this, as I also
> > believe that `ifa->ifa_local` is unlikely to be modified or freed in this
> > context. However, I ultimately decided to protect these sections with RCU for
> > the following reasons:
> >
> > - It is logically more consistent to protect access to `ifa->ifa_local` with
> >   RCU locking.
> >
> > - For section already under spinlock protection, adding RCU locking introduces
> >   negligible overhead, especially in a scenario like this.
> >
> > That said, I do not have a strong preference for either approach. If you prefer
> > using a local variable to keep the RCU region shorter, I can prepare a v2 with
> > that change.
> 
> I was not questioning whether this region should be protected, but
> rather how long you made the RCU critical section. The smaller the
> better, especially if you can remove a spinlock from it.
> 
> On PREEMPT_RT kernels it could even cause trouble because spinlocks
> become sleep-able and that is not allowed inside RCU read-side critical
> section.

I'm a bit confused by this comment. As you know, the ndo_start_xmit() callback
is executed in a context where the RCU lock is acquired. Many network drivers
use spinlocks in their ndo_start_xmit() callbacks. Am I missing something obvious?

> 
> So yes, I do insist that a tiny RCU is better; something like:
> 
> static int macb_suspend(struct device *dev)
> {
> 	u32 ifa_local;
> 
> 	// ...
> 
> 	if (bp->wol & MACB_WOL_ENABLED) {
> 		/* Check for IP address in WOL ARP mode */
> 		rcu_read_lock();
> 		idev = __in_dev_get_rcu(bp->dev);
> 		if (idev)
> 			ifa = rcu_dereference(idev->ifa_list);
> 		if (ifa)
> 			ifa_local = be32_to_cpu(ifa->ifa_local);
> 		rcu_read_unlock();
> 
> 		if ((bp->wolopts & WAKE_ARP) && !ifa) {
> 			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
> 			return -EOPNOTSUPP;
> 		}
> 
> 		// ...
> 	}
> 
> 	// ...
> }

Will reduce the scope of RCU lock in v2. Thanks, Lebrun.

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]