From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dl1-f52.google.com (mail-dl1-f52.google.com [74.125.82.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D4B21366DD9 for ; Wed, 18 Mar 2026 06:31:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773815473; cv=none; b=kerFkCUBgjeOt9836IaIGTFBJjYrWmtvWvHn+aoh4KzLqbrJIDsOW+hviAd/AAiSR0amXch8tDkgYOwyE25v4FIrMG1aYHCSJTL6mct2MTWxyfttp6aW8wo+ZvqTAgx5pcCka5vLCXGQO34llI1dA+VVNicUZT1mmgMfCICWcjY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773815473; c=relaxed/simple; bh=F9cHhWJXb3kHOhBeKBznjA0MZ+sKCxw3//7ZAbHZqyY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Q3rwev2Poj4HL+/Alxv3sDANeQiM4b3lkqcjUStRUYj4y2ih9D/xm+hW58c937uUqkBTKoFAMXTtwEUv3iNkhA7FH0+GGIQ0/Lc45krpTTBM1MDHI+7N8sk4ne5qb0VOWWGoj53tZHrbszdLogNk5gMMxrDo0lsyWWwGPQJPiw4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=d4X87I5d; arc=none smtp.client-ip=74.125.82.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="d4X87I5d" Received: by mail-dl1-f52.google.com with SMTP id a92af1059eb24-128d7db88b9so7960663c88.0 for ; Tue, 17 Mar 2026 23:31:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1773815471; x=1774420271; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=7mjfeB+QehGWxy6pvDSqDvx3PHWjcskLwFx02+x66yE=; b=d4X87I5d4oZX9BeGt42R5MOijiO2+O8lZMhZJAHzdg+hLb6CAPJLi+x5sSTclaV2rT G6tUD/A+kE6oZyDorF6qWelFEdOzFF8/uODylTfharZBioAwalJ+s3GDe58rV6FtTQSC edx1mQfuuNyh87f2XaPIK9OapXY0TPFAAW5M0+MKsmrsjKs07SrWiHGgRTx3qrE5qXim gCErGPzHH+DGrmOQRZNfMK5wD7VmzFMPmlnyHNVt+tMiq0DLMw9H90W3w/T76uF8Y/pP iP4qSSfxuNzMLtXGtH2HerhiMze2cQhS97s0Ax6mETV+Pe7SzZOlqwLEJCqaK+5Ob8jB k5NA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1773815471; x=1774420271; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7mjfeB+QehGWxy6pvDSqDvx3PHWjcskLwFx02+x66yE=; b=U5vEvJPjmRlzSBZSTukOE6nNRv6/rsia9kHeGmm2gI4BYvF4ag/Lkm6KYqiwP6fk2C KDeao8AAs+ZrZ4AEqSLJ0kTprpcQ7+j9dL2EcuDxJ6bQ7Is6yq8ijLvf7PfLh+ID0nhQ 4Cl4oJIvLGSv0Z6sPJrLorz5+cGdn/3xfRvQDOjYz3lW4GH3HZsOuOhHkL/wvT/ZEU3x 2wCN0mgAAgx4Ll093Jnl1j2NjWXvw2WyUFvZYOF7gX4GV5BOEpR1EMD3FO4ouh5Burqc DBBf8jDdfysJfrtcmeX3gTz8ZkRvApOdLn0fJ6BvvPwxBWeIzRt0jNNYA55h/huRtkFh uVSw== X-Gm-Message-State: AOJu0YztrN8wtJeklhEHA3s8R1b4hWZKnydb7/7/toluX23iJUA0CHsU kFJPZDi+K0Cyp0yk5CII5Zb1rWwUzemHzXFDOqNXM1NHYshJKeI08MoQ X-Gm-Gg: ATEYQzwLa31U2cXHtkT1xemPCyzO0U6QNYKmq6qC+VDiAn0kskgKOX5+VCwBOuE/mzh KprXmPC2lkpR0LuK/ql93njHusbMQaCns1ziR11kHs5EhL7jhCfyGuvvXlwgfZ0fnGMSjRzyAzZ rkmSRuj9fg6l/fTjkwnLayUsRie2/Sm+WSPmBcM3bjI7p4lxLXFtt0+2L0Yk1jv7v1+pqkzxzxa WTiJ/Vnp/lV+9zlOvPqjRNNUtST9mjFdDzpcEU+P22uNAjc4yFZjQVIfBOpK0eo1hhadFRbqNvo sCEh8yhAgk1XQBxX6SZUcIqgT2NU4V7JiSB0wpp0VvqlgG/+0oz7Lx2BF/3LI/RBqp9ZBN0a7Kf SGrwZf5okuUy1NiqZ9OIjpe97c6hWasI/YU1hGBGkL9eb6Wb/1BvWxkHoTgF+1MU00FoNRavesB iVXSjqEM2DCYrKVN6bek4= X-Received: by 2002:a05:7022:51b:b0:128:d4be:7418 with SMTP id a92af1059eb24-129a714bc7bmr1050843c88.35.1773815470580; Tue, 17 Mar 2026 23:31:10 -0700 (PDT) Received: from pek-khao-d3 ([128.224.246.2]) by smtp.gmail.com with ESMTPSA id a92af1059eb24-129a723f9d7sm2347316c88.2.2026.03.17.23.31.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Mar 2026 23:31:09 -0700 (PDT) Date: Wed, 18 Mar 2026 14:31:03 +0800 From: Kevin Hao To: =?iso-8859-1?Q?Th=E9o?= Lebrun Cc: netdev@vger.kernel.org, Nicolas Ferre , Claudiu Beznea , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Vineeth Karumanchi , Harini Katakam , stable@vger.kernel.org Subject: Re: [PATCH net 2/2] net: macb: Protect access to net_device::in_ptr with RCU lock Message-ID: References: <20260315-macb-irq-v1-0-0154104cbf61@gmail.com> <20260315-macb-irq-v1-2-0154104cbf61@gmail.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="EH2RervoWIbhLwfW" Content-Disposition: inline In-Reply-To: --EH2RervoWIbhLwfW Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 17, 2026 at 04:54:11PM +0100, Th=E9o 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=E9o 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(struc= t device *dev) > >> > =20 > >> > if (bp->wol & MACB_WOL_ENABLED) { > >> > /* Check for IP address in WOL ARP mode */ > >> > + rcu_read_lock(); > >> > idev =3D __in_dev_get_rcu(bp->dev); > >> > if (idev) > >> > ifa =3D rcu_dereference(idev->ifa_list); > >> > if ((bp->wolopts & WAKE_ARP) && !ifa) { > >> > netdev_err(netdev, "IP address not assigned as required by WoL w= alk ARP\n"); > >> > + rcu_read_unlock(); > >> > return -EOPNOTSUPP; > >> > } > >> > + > >> > spin_lock_irqsave(&bp->lock, flags); > >> > =20 > >> > /* Disable Tx and Rx engines before disabling the queues, > >> > @@ -5963,6 +5966,7 @@ static int __maybe_unused macb_suspend(struct = device *dev) > >> > tmp |=3D MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local)); > >> > } > >> > spin_unlock_irqrestore(&bp->lock, flags); > >> > + rcu_read_unlock(); > >> > =20 > >> > /* Change interrupt handler and > >> > * Enable WoL IRQ on queue 0 > >>=20 > >> 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, a= s I also > > believe that `ifa->ifa_local` is unlikely to be modified or freed in th= is > > context. However, I ultimately decided to protect these sections with R= CU 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 int= roduces > > negligible overhead, especially in a scenario like this. > > > > That said, I do not have a strong preference for either approach. If yo= u prefer > > using a local variable to keep the RCU region shorter, I can prepare a = v2 with > > that change. >=20 > 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. >=20 > 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() callb= ack is executed in a context where the RCU lock is acquired. Many network drive= rs use spinlocks in their ndo_start_xmit() callbacks. Am I missing something o= bvious? >=20 > So yes, I do insist that a tiny RCU is better; something like: >=20 > static int macb_suspend(struct device *dev) > { > u32 ifa_local; >=20 > // ... >=20 > if (bp->wol & MACB_WOL_ENABLED) { > /* Check for IP address in WOL ARP mode */ > rcu_read_lock(); > idev =3D __in_dev_get_rcu(bp->dev); > if (idev) > ifa =3D rcu_dereference(idev->ifa_list); > if (ifa) > ifa_local =3D be32_to_cpu(ifa->ifa_local); > rcu_read_unlock(); >=20 > if ((bp->wolopts & WAKE_ARP) && !ifa) { > netdev_err(netdev, "IP address not assigned as required by WoL walk AR= P\n"); > return -EOPNOTSUPP; > } >=20 > // ... > } >=20 > // ... > } Will reduce the scope of RCU lock in v2. Thanks, Lebrun. Thanks, Kevin --EH2RervoWIbhLwfW Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEHc6qFoLCZqgJD98Zk1jtMN6usXEFAmm6RqcACgkQk1jtMN6u sXHXvAgAlFNTSrFAIlrzgOF5y/xJOsfvyMqnJD9J8eE6UQNtUm8mkF3k0RD3HMW3 DqUXlUnn2RYGZ7gYwG08mbHF25gc+YiBTCDeErkF2vYRpZv/P+2BqlXlhQX6afpy Qn9wjdPhhvb+ed4lIVd48AR/52Y4u1HwLpH1lxg903MoNRwoWSHFKfi5EMn+0J+W IdyYkhGCxcqaWZ1qrlpS1altwwvwS5882sOLGvW/zLk7yJLs82uznK181uxlopor 54ijlCgQb75qvVpPojXFIxd60vELMA4MULL9vhOVhEnAlfaZPcWUTtZ+RzXuyrS2 sGjBM6HQphSxTFUFpcjZw5dlrqJ12A== =AN7D -----END PGP SIGNATURE----- --EH2RervoWIbhLwfW--