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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

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.

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;
		}

		// ...
	}

	// ...
}

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com