[PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[Breno Leitao]

In the __netpoll_setup() function, when accessing the device's npinfo
pointer, replace rcu_access_pointer() with rtnl_dereference(). This
change is more appropriate, as suggested by Herbert Xu.

The function is called with the RTNL mutex held, and the pointer is
being dereferenced later, so, dereference earlier and just reuse the
pointer for the if/else.

The replacement ensures correct pointer access while maintaining
the existing locking and RCU semantics of the netpoll subsystem.

Fixes: c75964e40e69 ("netpoll: Use rtnl_dereference() for npinfo pointer access")
Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/core/netpoll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 45fb60bc4803958eb07d4038028269fc0c19622e..30152811e0903a369ccca30500e11e696be158fd 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -626,7 +626,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 		goto out;
 	}
 
-	if (!rcu_access_pointer(ndev->npinfo)) {
+	npinfo = rtnl_dereference(ndev->npinfo);
+	if (!npinfo) {
 		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
 		if (!npinfo) {
 			err = -ENOMEM;
@@ -646,7 +647,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 				goto free_npinfo;
 		}
 	} else {
-		npinfo = rtnl_dereference(ndev->npinfo);
 		refcount_inc(&npinfo->refcnt);
 	}
 

---
base-commit: 66418447d27b7f4c027587582a133dd0bc0a663b
change-id: 20241121-netpoll_rcu_herbet_fix-3f0a433b7860

Best regards,
-- 
Breno Leitao <leitao@debian.org>

Re: [PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[Jacob Keller]

On 11/21/2024 6:58 AM, Breno Leitao wrote:
> In the __netpoll_setup() function, when accessing the device's npinfo
> pointer, replace rcu_access_pointer() with rtnl_dereference(). This
> change is more appropriate, as suggested by Herbert Xu.
> 
> The function is called with the RTNL mutex held, and the pointer is
> being dereferenced later, so, dereference earlier and just reuse the
> pointer for the if/else.
> 
> The replacement ensures correct pointer access while maintaining
> the existing locking and RCU semantics of the netpoll subsystem.
> 
> Fixes: c75964e40e69 ("netpoll: Use rtnl_dereference() for npinfo pointer access")
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[Herbert Xu]

On Thu, Nov 21, 2024 at 06:58:58AM -0800, Breno Leitao wrote:
> In the __netpoll_setup() function, when accessing the device's npinfo
> pointer, replace rcu_access_pointer() with rtnl_dereference(). This
> change is more appropriate, as suggested by Herbert Xu.
> 
> The function is called with the RTNL mutex held, and the pointer is
> being dereferenced later, so, dereference earlier and just reuse the
> pointer for the if/else.
> 
> The replacement ensures correct pointer access while maintaining
> the existing locking and RCU semantics of the netpoll subsystem.
> 
> Fixes: c75964e40e69 ("netpoll: Use rtnl_dereference() for npinfo pointer access")
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  net/core/netpoll.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[Eric Dumazet]

On Thu, Nov 21, 2024 at 3:59 PM Breno Leitao <leitao@debian.org> wrote:
>
> In the __netpoll_setup() function, when accessing the device's npinfo
> pointer, replace rcu_access_pointer() with rtnl_dereference(). This
> change is more appropriate, as suggested by Herbert Xu.
>
> The function is called with the RTNL mutex held, and the pointer is
> being dereferenced later, so, dereference earlier and just reuse the
> pointer for the if/else.
>
> The replacement ensures correct pointer access while maintaining
> the existing locking and RCU semantics of the netpoll subsystem.
>
> Fixes: c75964e40e69 ("netpoll: Use rtnl_dereference() for npinfo pointer access")

This seems wrong. This sha1 does not exist, and the title is this patch.

We do not send a patch saying it is fixing itself.

I would suggest instead :

Fixes: c69c5e10adb9 ("netpoll: Use rcu_access_pointer() in __netpoll_setup")

> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  net/core/netpoll.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 45fb60bc4803958eb07d4038028269fc0c19622e..30152811e0903a369ccca30500e11e696be158fd 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -626,7 +626,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
>                 goto out;
>         }
>
> -       if (!rcu_access_pointer(ndev->npinfo)) {
> +       npinfo = rtnl_dereference(ndev->npinfo);
> +       if (!npinfo) {
>                 npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>                 if (!npinfo) {
>                         err = -ENOMEM;
> @@ -646,7 +647,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
>                                 goto free_npinfo;
>                 }
>         } else {
> -               npinfo = rtnl_dereference(ndev->npinfo);
>                 refcount_inc(&npinfo->refcnt);
>         }
>
>
> ---
> base-commit: 66418447d27b7f4c027587582a133dd0bc0a663b
> change-id: 20241121-netpoll_rcu_herbet_fix-3f0a433b7860
>
> Best regards,
> --
> Breno Leitao <leitao@debian.org>
>

Re: [PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[Jakub Kicinski]

On Fri, 22 Nov 2024 00:41:14 +0100 Eric Dumazet wrote:
> > Fixes: c75964e40e69 ("netpoll: Use rtnl_dereference() for npinfo pointer access")  
> 
> This seems wrong. This sha1 does not exist, and the title is this patch.
> 
> We do not send a patch saying it is fixing itself.
> 
> I would suggest instead :
> 
> Fixes: c69c5e10adb9 ("netpoll: Use rcu_access_pointer() in __netpoll_setup")

Or no Fixes tag and net-next...

I'm missing what can go wrong here, seems like a cleanup.
-- 
pw-bot: cr

Re: [PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[Eric Dumazet]

On Fri, Nov 22, 2024 at 4:56 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 22 Nov 2024 00:41:14 +0100 Eric Dumazet wrote:
> > > Fixes: c75964e40e69 ("netpoll: Use rtnl_dereference() for npinfo pointer access")
> >
> > This seems wrong. This sha1 does not exist, and the title is this patch.
> >
> > We do not send a patch saying it is fixing itself.
> >
> > I would suggest instead :
> >
> > Fixes: c69c5e10adb9 ("netpoll: Use rcu_access_pointer() in __netpoll_setup")
>
> Or no Fixes tag and net-next...
>
> I'm missing what can go wrong here, seems like a cleanup.

Nothing wrong, perhaps add a Link: next time to avoid confusion

https://lore.kernel.org/lkml/Zz1cKZYt1e7elibV@gondor.apana.org.au/

Re: [PATCH net] netpoll: Use rtnl_dereference() for npinfo pointer access

[Breno Leitao]

On Thu, Nov 21, 2024 at 07:56:16PM -0800, Jakub Kicinski wrote:
> On Fri, 22 Nov 2024 00:41:14 +0100 Eric Dumazet wrote:
> > > Fixes: c75964e40e69 ("netpoll: Use rtnl_dereference() for npinfo pointer access")  
> > 
> > This seems wrong. This sha1 does not exist, and the title is this patch.
> > 
> > We do not send a patch saying it is fixing itself.
> > 
> > I would suggest instead :
> > 
> > Fixes: c69c5e10adb9 ("netpoll: Use rcu_access_pointer() in __netpoll_setup")
> 
> Or no Fixes tag and net-next...
> 
> I'm missing what can go wrong here, seems like a cleanup.

I decide to sent to net due Herbert's concern about the previous patch.

I will send a v2 targeting net-next, then.

Thanks for the review,
--breno