[PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[Kuniyuki Iwashima]

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(-)

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

Re: [PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[Vineeth Karumanchi]

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/ 



🙏 vineeth

> 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(-)
> 
> 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;

Re: [PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[Kuniyuki Iwashima]

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 ?

Re: [PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[Vineeth Karumanchi]

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

Re: [PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[Kuniyuki Iwashima]

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.

Re: [PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[Jakub Kicinski]

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.

Re: [PATCH v1 net] net: macb: Use rcu_dereference() for idev->ifa_list in macb_suspend().

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html