| Message ID | 20240808040021.6971-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6e1918ff680527ce4be77426aa537012b5aa997c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1,net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend(). | expand |
Hi Kuniyuki, On 08/08/24 9:30 am, Kuniyuki Iwashima wrote: > In macb_suspend(), idev->ifa_list is fetched with rcu_access_pointer() > and later the pointer is dereferenced as ifa->ifa_local. > > So, idev->ifa_list must be fetched with rcu_dereference(). > Is there any functional breakage ? I sent initial patch with rcu_dereference, but there is a review comment: https://lore.kernel.org/netdev/a02fac3b21a97dc766d65c4ed2d080f1ed87e87e.camel@redhat.com/
From: Vineeth Karumanchi <vineeth.karumanchi@amd.com> Date: Thu, 8 Aug 2024 09:53:42 +0530 > Hi Kuniyuki, > > On 08/08/24 9:30 am, Kuniyuki Iwashima wrote: > > In macb_suspend(), idev->ifa_list is fetched with rcu_access_pointer() > > and later the pointer is dereferenced as ifa->ifa_local. > > > > So, idev->ifa_list must be fetched with rcu_dereference(). > > > > Is there any functional breakage ? rcu_dereference() triggers lockdep splat if not called under rcu_read_lock(). Also in include/linux/rcupdate.h: /** * rcu_access_pointer() - fetch RCU pointer with no dereferencing ... * It is usually best to test the rcu_access_pointer() return value * directly in order to avoid accidental dereferences being introduced * by later inattentive changes. In other words, assigning the * rcu_access_pointer() return value to a local variable results in an * accident waiting to happen. > I sent initial patch with rcu_dereference, but there is a review comment: > > https://lore.kernel.org/netdev/a02fac3b21a97dc766d65c4ed2d080f1ed87e87e.camel@redhat.com/ I guess the following ifa_local was missed then ?
On 08/08/24 10:15 am, Kuniyuki Iwashima wrote: > From: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > Date: Thu, 8 Aug 2024 09:53:42 +0530 >> Hi Kuniyuki, >> >> On 08/08/24 9:30 am, Kuniyuki Iwashima wrote: >>> In macb_suspend(), idev->ifa_list is fetched with rcu_access_pointer() >>> and later the pointer is dereferenced as ifa->ifa_local. >>> >>> So, idev->ifa_list must be fetched with rcu_dereference(). >>> >> >> Is there any functional breakage ? > > rcu_dereference() triggers lockdep splat if not called under > rcu_read_lock(). > > Also in include/linux/rcupdate.h: > > /** > * rcu_access_pointer() - fetch RCU pointer with no dereferencing > ... > * It is usually best to test the rcu_access_pointer() return value > * directly in order to avoid accidental dereferences being introduced > * by later inattentive changes. In other words, assigning the > * rcu_access_pointer() return value to a local variable results in an > * accident waiting to happen. > > >> I sent initial patch with rcu_dereference, but there is a review comment: >> >> https://lore.kernel.org/netdev/a02fac3b21a97dc766d65c4ed2d080f1ed87e87e.camel@redhat.com/ > > I guess the following ifa_local was missed then ? I am ok to use rcu_dereference(), just curios why the check idev->ifa_list was removed ? vineeth
From: Vineeth Karumanchi <vineeth.karumanchi@amd.com> Date: Mon, 12 Aug 2024 14:41:40 +0530 > On 08/08/24 10:15 am, Kuniyuki Iwashima wrote: > > From: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > > Date: Thu, 8 Aug 2024 09:53:42 +0530 > >> Hi Kuniyuki, > >> > >> On 08/08/24 9:30 am, Kuniyuki Iwashima wrote: > >>> In macb_suspend(), idev->ifa_list is fetched with rcu_access_pointer() > >>> and later the pointer is dereferenced as ifa->ifa_local. > >>> > >>> So, idev->ifa_list must be fetched with rcu_dereference(). > >>> > >> > >> Is there any functional breakage ? > > > > rcu_dereference() triggers lockdep splat if not called under > > rcu_read_lock(). > > > > Also in include/linux/rcupdate.h: > > > > /** > > * rcu_access_pointer() - fetch RCU pointer with no dereferencing > > ... > > * It is usually best to test the rcu_access_pointer() return value > > * directly in order to avoid accidental dereferences being introduced > > * by later inattentive changes. In other words, assigning the > > * rcu_access_pointer() return value to a local variable results in an > > * accident waiting to happen. > > > > > >> I sent initial patch with rcu_dereference, but there is a review comment: > >> > >> https://lore.kernel.org/netdev/a02fac3b21a97dc766d65c4ed2d080f1ed87e87e.camel@redhat.com/ > > > > I guess the following ifa_local was missed then ? > > I am ok to use rcu_dereference(), just curios why the check > idev->ifa_list was removed ? "if (idev->ifa_list)" could fetch an invalid pointer due to lack of READ_ONCE(), it's not a big problem, but neither we should write it as if (rcu_assign_pointer(idev->ifa_list)) ifa = rcu_dereference(idev->ifa_list) because rcu_dereference()ed ifa is not guaranteed as the same value (non-NULL) with rcu_assign_pointer(), so we need the following check anyway: if (!ifa) Basically, we can't use rcu_assign_pointer() with rcu_dereference(). list_first_or_null_rcu() is a nice example: include/linux/rculist.h /* * Where are list_empty_rcu() and list_first_entry_rcu()? * * They do not exist because they would lead to subtle race conditions: * * if (!list_empty_rcu(mylist)) { * struct foo *bar = list_first_entry_rcu(mylist, struct foo, list_member); * do_something(bar); * } * * The list might be non-empty when list_empty_rcu() checks it, but it * might have become empty by the time that list_first_entry_rcu() rereads * the ->next pointer, which would result in a SEGV.
On Wed, 7 Aug 2024 21:00:21 -0700 Kuniyuki Iwashima wrote: > In macb_suspend(), idev->ifa_list is fetched with rcu_access_pointer() > and later the pointer is dereferenced as ifa->ifa_local. > > So, idev->ifa_list must be fetched with rcu_dereference(). I don't see where this driver takes rcu_read_lock, but __in_dev_get_rcu() will already splat so we're not making it any worse.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 7 Aug 2024 21:00:21 -0700 you wrote: > In macb_suspend(), idev->ifa_list is fetched with rcu_access_pointer() > and later the pointer is dereferenced as ifa->ifa_local. > > So, idev->ifa_list must be fetched with rcu_dereference(). > > Fixes: 0cb8de39a776 ("net: macb: Add ARP support to WOL") > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > [...] Here is the summary with links: - [v1,net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend(). https://git.kernel.org/netdev/net/c/6e1918ff6805 You are awesome, thank you!
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 11665be3a22c..dcd3f54ed0cf 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -5250,8 +5250,8 @@ static int __maybe_unused macb_suspend(struct device *dev) if (bp->wol & MACB_WOL_ENABLED) { /* Check for IP address in WOL ARP mode */ idev = __in_dev_get_rcu(bp->dev); - if (idev && idev->ifa_list) - ifa = rcu_access_pointer(idev->ifa_list); + 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;
In macb_suspend(), idev->ifa_list is fetched with rcu_access_pointer() and later the pointer is dereferenced as ifa->ifa_local. So, idev->ifa_list must be fetched with rcu_dereference(). Fixes: 0cb8de39a776 ("net: macb: Add ARP support to WOL") Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- drivers/net/ethernet/cadence/macb_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)