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

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

Hello Kevin,

On Wed Mar 18, 2026 at 7:36 AM CET, Kevin Hao wrote:
> Access to net_device::ip_ptr and its associated members must be
> protected by an RCU lock. Since we are modifying this piece of code,
> let's also move it to execute only when WAKE_ARP is enabled.
>
> To minimize the duration of the RCU lock, a local variable is used to
> temporarily store the IP address. This change resolves the following
> RCU check warning:
>   WARNING: suspicious RCU usage
>   7.0.0-rc3-next-20260310-yocto-standard+ #122 Not tainted
>   -----------------------------
>   drivers/net/ethernet/cadence/macb_main.c:5944 suspicious rcu_dereference_check() usage!
>
>   other info that might help us debug this:
>
>   rcu_scheduler_active = 2, debug_locks = 1
>   5 locks held by rtcwake/518:
>    #0: ffff000803ab1408 (sb_writers#5){.+.+}-{0:0}, at: vfs_write+0xf8/0x368
>    #1: ffff0008090bf088 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0xbc/0x1c8
>    #2: ffff00080098d588 (kn->active#70){.+.+}-{0:0}, at: kernfs_fop_write_iter+0xcc/0x1c8
>    #3: ffff800081c84888 (system_transition_mutex){+.+.}-{4:4}, at: pm_suspend+0x1ec/0x290
>    #4: ffff0008009ba0f8 (&dev->mutex){....}-{4:4}, at: device_suspend+0x118/0x4f0
>
>   stack backtrace:
>   CPU: 3 UID: 0 PID: 518 Comm: rtcwake Not tainted 7.0.0-rc3-next-20260310-yocto-standard+ #122 PREEMPT
>   Hardware name: ZynqMP ZCU102 Rev1.1 (DT)
>   Call trace:
>    show_stack+0x24/0x38 (C)
>    __dump_stack+0x28/0x38
>    dump_stack_lvl+0x64/0x88
>    dump_stack+0x18/0x24
>    lockdep_rcu_suspicious+0x134/0x1d8
>    macb_suspend+0xd8/0x4c0
>    device_suspend+0x218/0x4f0
>    dpm_suspend+0x244/0x3a0
>    dpm_suspend_start+0x50/0x78
>    suspend_devices_and_enter+0xec/0x560
>    pm_suspend+0x194/0x290
>    state_store+0x110/0x158
>    kobj_attr_store+0x1c/0x30
>    sysfs_kf_write+0xa8/0xd0
>    kernfs_fop_write_iter+0x11c/0x1c8
>    vfs_write+0x248/0x368
>    ksys_write+0x7c/0xf8
>    __arm64_sys_write+0x28/0x40
>    invoke_syscall+0x4c/0xe8
>    el0_svc_common+0x98/0xf0
>    do_el0_svc+0x28/0x40
>    el0_svc+0x54/0x1e0
>    el0t_64_sync_handler+0x84/0x130
>    el0t_64_sync+0x198/0x1a0
>
> Fixes: 0cb8de39a776 ("net: macb: Add ARP support to WOL")
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 2d4304331e297accd91ab48813a9bd4722ce72dc..9856764402b17397928d0a61da61865a3b10484f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -5909,9 +5909,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  	struct macb_queue *queue;
>  	struct in_device *idev;
>  	unsigned long flags;
> +	u32 tmp, ifa_local;
>  	unsigned int q;
>  	int err;
> -	u32 tmp;
>  
>  	if (!device_may_wakeup(&bp->dev->dev))
>  		phy_exit(bp->phy);
> @@ -5920,14 +5920,21 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  		return 0;
>  
>  	if (bp->wol & MACB_WOL_ENABLED) {
> -		/* Check for IP address in WOL ARP mode */
> -		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");
> -			return -EOPNOTSUPP;
> +		if (bp->wolopts & WAKE_ARP) {
> +			/* 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) {
> +				rcu_read_unlock();
> +				netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
> +				return -EOPNOTSUPP;
> +			}
> +			ifa_local = be32_to_cpu(ifa->ifa_local);
> +			rcu_read_unlock();
>  		}
> +
>  		spin_lock_irqsave(&bp->lock, flags);
>  
>  		/* Disable Tx and Rx engines before  disabling the queues,
> @@ -5966,7 +5973,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  		if (bp->wolopts & WAKE_ARP) {
>  			tmp |= MACB_BIT(ARP);
>  			/* write IP address into register */
> -			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
> +			tmp |= MACB_BFEXT(IP, ifa_local);
>  		}
>  		spin_unlock_irqrestore(&bp->lock, flags);
>  

Even better to guard the RCU critical section inside a
`if (bp->wolopts & WAKE_ARP)` block.

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks,

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