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: 2026 bytes --]

On Mon, Mar 16, 2026 at 06:59:35PM +0100, Théo Lebrun wrote:
> Hello Kevin,
> 
> 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.

Thanks,
Kevin

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