netdev - Re: [PATCH net 1/2] netpoll: Use rcu_access_pointer() in __netpoll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZzwF4QdNch_UzMlV@gondor.apana.org.au>
Date: Tue, 19 Nov 2024 11:28:33 +0800
From: Herbert Xu <herbert@...dor.apana.org.au>
To: Breno Leitao <leitao@...ian.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Stephen Hemminger <stephen@...workplumber.org>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	paulmck@...nel.org, Cong Wang <cong.wang@...edance.com>
Subject: Re: [PATCH net 1/2] netpoll: Use rcu_access_pointer() in
 __netpoll_setup

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@...ian.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@...hat.com>
Date:   Tue Jan 22 21:29:39 2013 +0000

    netpoll: add RCU annotation to npinfo field

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