| Message ID | 20250303-netpoll_rcu_v2-v1-1-6b34d8a01fa2@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] netpoll: guard __netpoll_send_skb() with RCU read lock | expand |
On Mon, 03 Mar 2025 03:44:12 -0800 Breno Leitao wrote:
> + guard(rcu)();
Scoped guards if you have to.
Preferably just lock/unlock like a normal person..
Hello Jakub, On Tue, Mar 04, 2025 at 05:47:32PM -0800, Jakub Kicinski wrote: > On Mon, 03 Mar 2025 03:44:12 -0800 Breno Leitao wrote: > > + guard(rcu)(); > > Scoped guards if you have to. > Preferably just lock/unlock like a normal person.. Sure, I thought that we would be moving to scoped guards all across the board, at least that was my reading for a similar patch I sent a while ago: https://lore.kernel.org/all/20250224123016.GA17456@noisy.programming.kicks-ass.net/ Anyway, in which case should I use scoped guard instead of just being like a normal person? Thanks for the review, --breno
On Wed, 5 Mar 2025 01:09:49 -0800 Breno Leitao wrote: > On Tue, Mar 04, 2025 at 05:47:32PM -0800, Jakub Kicinski wrote: > > On Mon, 03 Mar 2025 03:44:12 -0800 Breno Leitao wrote: > > > + guard(rcu)(); > > > > Scoped guards if you have to. > > Preferably just lock/unlock like a normal person.. > > Sure, I thought that we would be moving to scoped guards all across the > board, at least that was my reading for a similar patch I sent a while > ago: > > https://lore.kernel.org/all/20250224123016.GA17456@noisy.programming.kicks-ass.net/ > > Anyway, in which case should I use scoped guard instead We are certainly not moving to guards in networking. Too C++-sy. Just lock / unlock please, correctly around the variable you actually intend to protect. Quoting documentation: Using device-managed and cleanup.h constructs ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Netdev remains skeptical about promises of all "auto-cleanup" APIs, including even ``devm_`` helpers, historically. They are not the preferred style of implementation, merely an acceptable one. Use of ``guard()`` is discouraged within any function longer than 20 lines, ``scoped_guard()`` is considered more readable. Using normal lock/unlock is still (weakly) preferred. Low level cleanup constructs (such as ``__free()``) can be used when building APIs and helpers, especially scoped iterators. However, direct use of ``__free()`` within networking core and drivers is discouraged. Similar guidance applies to declaring variables mid-function. See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
On Wed, Mar 05, 2025 at 01:09:49AM -0800, Breno Leitao wrote: > Hello Jakub, > > On Tue, Mar 04, 2025 at 05:47:32PM -0800, Jakub Kicinski wrote: > > On Mon, 03 Mar 2025 03:44:12 -0800 Breno Leitao wrote: > > > + guard(rcu)(); > > > > Scoped guards if you have to. > > Preferably just lock/unlock like a normal person.. > > Sure, I thought that we would be moving to scoped guards all across the > board, at least that was my reading for a similar patch I sent a while > ago: > > https://lore.kernel.org/all/20250224123016.GA17456@noisy.programming.kicks-ass.net/ > > Anyway, in which case should I use scoped guard instead of just being > like a normal person? https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html Section 1.6.5: Using device-managed and cleanup.h constructs Netdev remains skeptical about promises of all “auto-cleanup” APIs, including even devm_ helpers, historically. They are not the preferred style of implementation, merely an acceptable one. Use of guard() is discouraged within any function longer than 20 lines, scoped_guard() is considered more readable. Using normal lock/unlock is still (weakly) preferred. Low level cleanup constructs (such as __free()) can be used when building APIs and helpers, especially scoped iterators. However, direct use of __free() within networking core and drivers is discouraged. Similar guidance applies to declaring variables mid-function. So you need to spend time to find out what each subsystems view is on various APIs. Andrew
On Wed, Mar 05, 2025 at 05:09:14PM +0100, Andrew Lunn wrote: > On Wed, Mar 05, 2025 at 01:09:49AM -0800, Breno Leitao wrote: > > Hello Jakub, > > > > On Tue, Mar 04, 2025 at 05:47:32PM -0800, Jakub Kicinski wrote: > > > On Mon, 03 Mar 2025 03:44:12 -0800 Breno Leitao wrote: > > > > + guard(rcu)(); > > > > > > Scoped guards if you have to. > > > Preferably just lock/unlock like a normal person.. > > > > Sure, I thought that we would be moving to scoped guards all across the > > board, at least that was my reading for a similar patch I sent a while > > ago: > > > > https://lore.kernel.org/all/20250224123016.GA17456@noisy.programming.kicks-ass.net/ > > > > Anyway, in which case should I use scoped guard instead of just being > > like a normal person? > > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html > > Section 1.6.5: Using device-managed and cleanup.h constructs > > Netdev remains skeptical about promises of all “auto-cleanup” APIs, > including even devm_ helpers, historically. They are not the > preferred style of implementation, merely an acceptable one. > > Use of guard() is discouraged within any function longer than 20 > lines, scoped_guard() is considered more readable. Using normal > lock/unlock is still (weakly) preferred. > > Low level cleanup constructs (such as __free()) can be used when > building APIs and helpers, especially scoped iterators. However, > direct use of __free() within networking core and drivers is > discouraged. Similar guidance applies to declaring variables > mid-function. > > So you need to spend time to find out what each subsystems view is on > various APIs. That is clear. thanks for the heads-up! --breno
diff --git a/net/core/netpoll.c b/net/core/netpoll.c index 62b4041aae1ae..cac389105e2d1 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -326,6 +326,7 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) lockdep_assert_irqs_disabled(); + guard(rcu)(); dev = np->dev; npinfo = rcu_dereference_bh(dev->npinfo);
The function __netpoll_send_skb() is being invoked without holding the RCU read lock. This oversight triggers a warning message when CONFIG_PROVE_RCU_LIST is enabled: net/core/netpoll.c:330 suspicious rcu_dereference_check() usage! netpoll_send_skb netpoll_send_udp write_ext_msg console_flush_all console_unlock vprintk_emit To prevent npinfo from disappearing unexpectedly, ensure that __netpoll_send_skb() is protected with the RCU read lock. Fixes: 2899656b494dcd1 ("netpoll: take rcu_read_lock_bh() in netpoll_send_skb_on_dev()") Signed-off-by: Breno Leitao <leitao@debian.org> --- net/core/netpoll.c | 1 + 1 file changed, 1 insertion(+) --- base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6 change-id: 20250303-netpoll_rcu_v2-fed72eb0cb83 Best regards,