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: <Zz1RCAT9Ao5PsAAK@gondor.apana.org.au>
Date: Wed, 20 Nov 2024 11:01:28 +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 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@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt