[PATCH net 0/2] netpoll: Use RCU primitives for npinfo pointer access

[PATCH net 0/2] netpoll: Use RCU primitives for npinfo pointer access

[Breno Leitao]

The net_device->npinfo pointer is marked with __rcu, indicating it requires
proper RCU access primitives:

  struct net_device {
	...
	struct netpoll_info __rcu *npinfo;
	...
  };

Direct access to this pointer can lead to issues such as:
- Compiler incorrectly caching/reusing stale pointer values
- Missing memory ordering guarantees
- Non-atomic pointer loads

Replace direct NULL checks of npinfo with rcu_access_pointer(),
which provides the necessary memory ordering guarantees without the
overhead of a full RCU dereference, since we only need to verify
if the pointer is NULL.

In both cases, the RCU read lock is not held when the function is being
called. I checked that by using lockdep_assert_in_rcu_read_lock(), and
seeing the warning on both cases.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Breno Leitao (2):
      netpoll: Use rcu_access_pointer() in __netpoll_setup
      netpoll: Use rcu_access_pointer() in netpoll_poll_lock

 include/linux/netpoll.h | 2 +-
 net/core/netpoll.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
---
base-commit: 8ffade77b6337a8767fae9820d57d7a6413dd1a1
change-id: 20241115-netpoll_rcu-eb1296511b71

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

[PATCH net 1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup

[Breno Leitao]

The ndev->npinfo pointer in __netpoll_setup() is RCU-protected but is being
accessed directly for a NULL check. While no RCU read lock is held in this
context, we should still use proper RCU primitives for consistency and
correctness.

Replace the direct NULL check with rcu_access_pointer(), which is the
appropriate primitive when only checking for NULL without dereferencing
the pointer. This function provides the necessary ordering guarantees
without requiring RCU read-side protection.

Signed-off-by: Breno Leitao <leitao@debian.org>
Fixes: 8fdd95ec162a ("netpoll: Allow netpoll_setup/cleanup recursion")
---
 net/core/netpoll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index aa49b92e9194babab17b2e039daf092a524c5b88..45fb60bc4803958eb07d4038028269fc0c19622e 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -626,7 +626,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 		goto out;
 	}
 
-	if (!ndev->npinfo) {
+	if (!rcu_access_pointer(ndev->npinfo)) {
 		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
 		if (!npinfo) {
 			err = -ENOMEM;

-- 
2.43.5

[PATCH net 2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock

[Breno Leitao]

The ndev->npinfo pointer in netpoll_poll_lock() is RCU-protected but is
being accessed directly for a NULL check. While no RCU read lock is held
in this context, we should still use proper RCU primitives for
consistency and correctness.

Replace the direct NULL check with rcu_access_pointer(), which is the
appropriate primitive when only checking for NULL without dereferencing
the pointer. This function provides the necessary ordering guarantees
without requiring RCU read-side protection.

Signed-off-by: Breno Leitao <leitao@debian.org>
Fixes: bea3348eef27 ("[NET]: Make NAPI polling independent of struct net_device objects.")
---
 include/linux/netpoll.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index cd4e28db0cbd77572a579aff2067b5864d1a904a..959a4daacea1f2f76536e309d198bc14407942a4 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -72,7 +72,7 @@ static inline void *netpoll_poll_lock(struct napi_struct *napi)
 {
 	struct net_device *dev = napi->dev;
 
-	if (dev && dev->npinfo) {
+	if (dev && rcu_access_pointer(dev->npinfo)) {
 		int owner = smp_processor_id();
 
 		while (cmpxchg(&napi->poll_owner, -1, owner) != -1)

-- 
2.43.5

Re: [PATCH net 1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup

[Michal Kubiak]

On Mon, Nov 18, 2024 at 03:15:17AM -0800, Breno Leitao wrote:
> The ndev->npinfo pointer in __netpoll_setup() is RCU-protected but is being
> accessed directly for a NULL check. While no RCU read lock is held in this
> context, we should still use proper RCU primitives for consistency and
> correctness.
> 
> Replace the direct NULL check with rcu_access_pointer(), which is the
> appropriate primitive when only checking for NULL without dereferencing
> the pointer. This function provides the necessary ordering guarantees
> without requiring RCU read-side protection.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Fixes: 8fdd95ec162a ("netpoll: Allow netpoll_setup/cleanup recursion")

nitpick: Shouldn't the "Signed-off-by" tag go as the last one?

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

> ---
>  net/core/netpoll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index aa49b92e9194babab17b2e039daf092a524c5b88..45fb60bc4803958eb07d4038028269fc0c19622e 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -626,7 +626,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
>  		goto out;
>  	}
>  
> -	if (!ndev->npinfo) {
> +	if (!rcu_access_pointer(ndev->npinfo)) {
>  		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>  		if (!npinfo) {
>  			err = -ENOMEM;
> 
> -- 
> 2.43.5
> 
> 

Re: [PATCH net 2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock

[Michal Kubiak]

On Mon, Nov 18, 2024 at 03:15:18AM -0800, Breno Leitao wrote:
> The ndev->npinfo pointer in netpoll_poll_lock() is RCU-protected but is
> being accessed directly for a NULL check. While no RCU read lock is held
> in this context, we should still use proper RCU primitives for
> consistency and correctness.
> 
> Replace the direct NULL check with rcu_access_pointer(), which is the
> appropriate primitive when only checking for NULL without dereferencing
> the pointer. This function provides the necessary ordering guarantees
> without requiring RCU read-side protection.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Fixes: bea3348eef27 ("[NET]: Make NAPI polling independent of struct net_device objects.")

nitpick: As for the first patch - please check the tags order.

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Re: [PATCH net 2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock

[Breno Leitao]

On Mon, Nov 18, 2024 at 01:20:30PM +0100, Michal Kubiak wrote:
> On Mon, Nov 18, 2024 at 03:15:18AM -0800, Breno Leitao wrote:
> > The ndev->npinfo pointer in netpoll_poll_lock() is RCU-protected but is
> > being accessed directly for a NULL check. While no RCU read lock is held
> > in this context, we should still use proper RCU primitives for
> > consistency and correctness.
> > 
> > Replace the direct NULL check with rcu_access_pointer(), which is the
> > appropriate primitive when only checking for NULL without dereferencing
> > the pointer. This function provides the necessary ordering guarantees
> > without requiring RCU read-side protection.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Fixes: bea3348eef27 ("[NET]: Make NAPI polling independent of struct net_device objects.")
> 
> nitpick: As for the first patch - please check the tags order.
> 
> Thanks,
> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Thanks for the review.

I am not planning to resend it now, since I think maintainer's tooling will
reorder that.

If that is not true, I am more than happy to resend fixing the order.

Thanks
Breno

Re: [PATCH net 2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock

[Jakub Kicinski]

On Mon, 18 Nov 2024 07:37:43 -0800 Breno Leitao wrote:
> I am not planning to resend it now, since I think maintainer's tooling will
> reorder that.

FWIW ours doesn't, but I fixed up manually.

Re: [PATCH net 1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup

[Herbert Xu]

On Mon, Nov 18, 2024 at 03:15:17AM -0800, Breno Leitao wrote:
> The ndev->npinfo pointer in __netpoll_setup() is RCU-protected but is being
> accessed directly for a NULL check. While no RCU read lock is held in this
> context, we should still use proper RCU primitives for consistency and
> correctness.
> 
> Replace the direct NULL check with rcu_access_pointer(), which is the
> appropriate primitive when only checking for NULL without dereferencing
> the pointer. This function provides the necessary ordering guarantees
> without requiring RCU read-side protection.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Fixes: 8fdd95ec162a ("netpoll: Allow netpoll_setup/cleanup recursion")
> ---
>  net/core/netpoll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index aa49b92e9194babab17b2e039daf092a524c5b88..45fb60bc4803958eb07d4038028269fc0c19622e 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -626,7 +626,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
>  		goto out;
>  	}
>  
> -	if (!ndev->npinfo) {
> +	if (!rcu_access_pointer(ndev->npinfo)) {
>  		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>  		if (!npinfo) {
>  			err = -ENOMEM;

This is completely bogus.  Think about it, we are setting ndev->npinfo,
meaning that we must have some form of synchronisation over this that
guarantees us to be the only writer.

So why does it need RCU protection for reading?

Assuming that this code isn't completely bonkers, then the correct
primitive to use should be rcu_dereference_protected.  Also the
Fixes header should be set to the commit that introduced the broken
RCU marking:

commit 5fbee843c32e5de2d8af68ba0bdd113bb0af9d86
Author: Cong Wang <amwang@redhat.com>
Date:   Tue Jan 22 21:29:39 2013 +0000

    netpoll: add RCU annotation to npinfo field

Cheers,
-- 
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 0/2] netpoll: Use RCU primitives for npinfo pointer access

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 18 Nov 2024 03:15:16 -0800 you wrote:
> The net_device->npinfo pointer is marked with __rcu, indicating it requires
> proper RCU access primitives:
> 
>   struct net_device {
> 	...
> 	struct netpoll_info __rcu *npinfo;
> 	...
>   };
> 
> [...]

Here is the summary with links:
  - [net,1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup
    https://git.kernel.org/netdev/net/c/c69c5e10adb9
  - [net,2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock
    https://git.kernel.org/netdev/net/c/a57d5a72f8de

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock

[Herbert Xu]

On Mon, Nov 18, 2024 at 03:15:18AM -0800, Breno Leitao wrote:
>
> diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
> index cd4e28db0cbd77572a579aff2067b5864d1a904a..959a4daacea1f2f76536e309d198bc14407942a4 100644
> --- a/include/linux/netpoll.h
> +++ b/include/linux/netpoll.h
> @@ -72,7 +72,7 @@ static inline void *netpoll_poll_lock(struct napi_struct *napi)
>  {
>  	struct net_device *dev = napi->dev;
>  
> -	if (dev && dev->npinfo) {
> +	if (dev && rcu_access_pointer(dev->npinfo)) {

This function is only ever called in a BH context and as such
the correct primitive would be rcu_dereference.  Indeed calling
this outside of BH/RCU context would be silly.

Cheers,
-- 
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 0/2] netpoll: Use RCU primitives for npinfo pointer access

[Herbert Xu]

On Tue, Nov 19, 2024 at 03:40:31AM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This series was applied to netdev/net.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
> 
> On Mon, 18 Nov 2024 03:15:16 -0800 you wrote:
> > The net_device->npinfo pointer is marked with __rcu, indicating it requires
> > proper RCU access primitives:
> > 
> >   struct net_device {
> > 	...
> > 	struct netpoll_info __rcu *npinfo;
> > 	...
> >   };
> > 
> > [...]
> 
> Here is the summary with links:
>   - [net,1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup
>     https://git.kernel.org/netdev/net/c/c69c5e10adb9
>   - [net,2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock
>     https://git.kernel.org/netdev/net/c/a57d5a72f8de

These are not bug fixes.  They should not be going through during
a merge window, especially with such a short period of review.

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 1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup

[Breno Leitao]

Hello Herbet,

On Tue, Nov 19, 2024 at 11:28:33AM +0800, Herbert Xu wrote:
> On Mon, Nov 18, 2024 at 03:15:17AM -0800, Breno Leitao wrote:
> > The ndev->npinfo pointer in __netpoll_setup() is RCU-protected but is being
> > accessed directly for a NULL check. While no RCU read lock is held in this
> > context, we should still use proper RCU primitives for consistency and
> > correctness.
> > 
> > Replace the direct NULL check with rcu_access_pointer(), which is the
> > appropriate primitive when only checking for NULL without dereferencing
> > the pointer. This function provides the necessary ordering guarantees
> > without requiring RCU read-side protection.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Fixes: 8fdd95ec162a ("netpoll: Allow netpoll_setup/cleanup recursion")
> > ---
> >  net/core/netpoll.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> > index aa49b92e9194babab17b2e039daf092a524c5b88..45fb60bc4803958eb07d4038028269fc0c19622e 100644
> > --- a/net/core/netpoll.c
> > +++ b/net/core/netpoll.c
> > @@ -626,7 +626,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
> >  		goto out;
> >  	}
> >  
> > -	if (!ndev->npinfo) {
> > +	if (!rcu_access_pointer(ndev->npinfo)) {
> >  		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
> >  		if (!npinfo) {
> >  			err = -ENOMEM;
> 
> This is completely bogus.  Think about it, we are setting ndev->npinfo,
> meaning that we must have some form of synchronisation over this that
> guarantees us to be the only writer.

Correct. __netpoll_setup() should have the RTNL lock held. In the most
common case, it is done through:

	netpoll_setup() {
		rtnl_lock();
		...
		__netpoll_setup()
		...
		rtnl_unlock();
	}

> So why does it need RCU protection for reading?

Good question, I understand this bring explicit calls to RCU pointers. In
fact, the same function that this patch changes (__netpoll_setup), later
does use rtnl_dereference(), and it is inside the same RTNL lock.

More over, looking at the RCU documentation, there is an explicit example
about this, at Documentation/RCU/Design/Requirements/Requirements.rst in
the "Performance and Scalability" section. I says:

      spin_lock(&gp_lock);
      p = rcu_access_pointer(gp);
      if (!p) {
		spin_unlock(&gp_lock);
		return false;
      }

> Assuming that this code isn't completely bonkers, then the correct
> primitive to use should be rcu_dereference_protected.

I looked about rcu_dereference_protected() as well, and I though it is
used when you are de-referencing the pointer, which is a more expensive
approach.  In the code above, the code basically need to check if the
pointer is assigned or not. Looking at the code, it seems that having
rcu_access_pointer() inside the update lock seems a common pattern, than
that is what I chose.

On the other side, I understand we want to call an RCU primitive with
the _protected() context, so, I looked for a possible
`rcu_access_pointer_protected()`, but this best does not exist. Anyway,
I am happy to change it, if it is the correct API.

> Fixes header should be set to the commit that introduced the broken
> RCU marking:
> 
> commit 5fbee843c32e5de2d8af68ba0bdd113bb0af9d86
> Author: Cong Wang <amwang@redhat.com>
> Date:   Tue Jan 22 21:29:39 2013 +0000
> 
>     netpoll: add RCU annotation to npinfo field

When 8fdd95ec162a was created, npinfo was an RCU pointer, although
without the RCU annotation that came later (5fbee843c).  That is
reason I chose to fix 8fdd95ec162a.

For instance, checking out 8fdd95ec162a, at the end of
__netpoll_setup(), I see, the RCU annotation, indicating that
ndev->npinfo was a RCU protected pointer.

          /* last thing to do is link it to the net device structure */
          rcu_assign_pointer(ndev->npinfo, npinfo);

Thanks for feedback and the good pointers
--breno

Re: [PATCH net 0/2] netpoll: Use RCU primitives for npinfo pointer access

[Jakub Kicinski]

On Tue, 19 Nov 2024 11:53:49 +0800 Herbert Xu wrote:
> > Here is the summary with links:
> >   - [net,1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup
> >     https://git.kernel.org/netdev/net/c/c69c5e10adb9
> >   - [net,2/2] netpoll: Use rcu_access_pointer() in netpoll_poll_lock
> >     https://git.kernel.org/netdev/net/c/a57d5a72f8de  
> 
> These are not bug fixes.  They should not be going through during
> a merge window, especially with such a short period of review.

Sorry, I do agree with your assessment, especially on patch 1.
But it's good to silence the false positive, so I applied and
stripped the Fixes tag.

Re: [PATCH net 1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup

[Herbert Xu]

On Tue, Nov 19, 2024 at 02:22:06AM -0800, Breno Leitao wrote:
>
> I looked about rcu_dereference_protected() as well, and I though it is
> used when you are de-referencing the pointer, which is a more expensive
> approach.  In the code above, the code basically need to check if the
> pointer is assigned or not. Looking at the code, it seems that having
> rcu_access_pointer() inside the update lock seems a common pattern, than
> that is what I chose.

No, rcu_dereference_protected is actually cheaper than rcu_access_pointer:

#define __rcu_access_pointer(p, local, space) \
({ \
        typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
        rcu_check_sparse(p, space); \
        ((typeof(*p) __force __kernel *)(local)); \
})

#define __rcu_dereference_protected(p, local, c, space) \
({ \
        RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \
        rcu_check_sparse(p, space); \
        ((typeof(*p) __force __kernel *)(p)); \
})

> On the other side, I understand we want to call an RCU primitive with
> the _protected() context, so, I looked for a possible
> `rcu_access_pointer_protected()`, but this best does not exist. Anyway,
> I am happy to change it, if it is the correct API.

There is no need for rcu_access_pointer_protected because the
rcu_dereference_protected helper is already the cheapest.

> When 8fdd95ec162a was created, npinfo was an RCU pointer, although
> without the RCU annotation that came later (5fbee843c).  That is
> reason I chose to fix 8fdd95ec162a.

The code was correct as is without RCU markings.  The only reason
we need the RCU markings is because an __rcu tag was added to the
variable later, without also making the necessary changes in the
existing code using that variable.

Cheers,
-- 
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 1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup

[Herbert Xu]

On Wed, Nov 20, 2024 at 11:01:28AM +0800, Herbert Xu wrote:
> 
> No, rcu_dereference_protected is actually cheaper than rcu_access_pointer:

BTW, this code should just use rtnl_dereference which does the
right thing.

Cheers,
-- 
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 1/2] netpoll: Use rcu_access_pointer() in __netpoll_setup

[Breno Leitao]

hello Herbet,

On Wed, Nov 20, 2024 at 11:48:57AM +0800, Herbert Xu wrote:
> On Wed, Nov 20, 2024 at 11:01:28AM +0800, Herbert Xu wrote:
> > 
> > No, rcu_dereference_protected is actually cheaper than rcu_access_pointer:
> 
> BTW, this code should just use rtnl_dereference which does the
> right thing.

Sure, let me send a patch with rtnl_derefence then.

Thanks for the feedback,
Breno