| Message ID | 20250306-netpoll_rcu_v2-v2-1-bc4f5c51742a@debian.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 505ead7ab77f289f12d8a68ac83da068e4d4408b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] netpoll: hold rcu read lock in __netpoll_send_skb() | expand |
On Thu, Mar 06, 2025 at 05:16:18AM -0800, Breno Leitao wrote: > 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> > --- > Changes in v2: > - Use rcu_read_lock() instead of guard() as normal people do (Jakub). > - Link to v1: https://lore.kernel.org/r/20250303-netpoll_rcu_v2-v1-1-6b34d8a01fa2@debian.org Nice that we can be normal :) Reviewed-by: Simon Horman <horms@kernel.org> > --- > net/core/netpoll.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > index 62b4041aae1ae..0ab722d95a2df 100644 > --- a/net/core/netpoll.c > +++ b/net/core/netpoll.c > @@ -319,6 +319,7 @@ static int netpoll_owner_active(struct net_device *dev) > static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) > { > netdev_tx_t status = NETDEV_TX_BUSY; > + netdev_tx_t ret = NET_XMIT_DROP; > struct net_device *dev; > unsigned long tries; > /* It is up to the caller to keep npinfo alive. */ > @@ -327,11 +328,12 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) > lockdep_assert_irqs_disabled(); > > dev = np->dev; > + rcu_read_lock(); > npinfo = rcu_dereference_bh(dev->npinfo); > > if (!npinfo || !netif_running(dev) || !netif_device_present(dev)) { > dev_kfree_skb_irq(skb); > - return NET_XMIT_DROP; nit: I would have set ret here rather than as part of it's declaration, to avoid it being set twice in the non-error case. But as this function is doing quite a lot, and moreover the compiler probably has it's own ideas, I don' think this is a big deal. > + goto out; > } > > /* don't get messages out of order, and no recursion */ > @@ -370,7 +372,10 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) > skb_queue_tail(&npinfo->txq, skb); > schedule_delayed_work(&npinfo->tx_work,0); > } > - return NETDEV_TX_OK; > + ret = NETDEV_TX_OK; > +out: > + rcu_read_unlock(); > + return ret; > } > > netdev_tx_t netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) > > --- > base-commit: 848e076317446f9c663771ddec142d7c2eb4cb43 > change-id: 20250303-netpoll_rcu_v2-fed72eb0cb83 > > Best regards, > -- > Breno Leitao <leitao@debian.org> >
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 06 Mar 2025 05:16:18 -0800 you wrote: > 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 > > [...] Here is the summary with links: - [net,v2] netpoll: hold rcu read lock in __netpoll_send_skb() https://git.kernel.org/netdev/net/c/505ead7ab77f You are awesome, thank you!
diff --git a/net/core/netpoll.c b/net/core/netpoll.c index 62b4041aae1ae..0ab722d95a2df 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -319,6 +319,7 @@ static int netpoll_owner_active(struct net_device *dev) static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) { netdev_tx_t status = NETDEV_TX_BUSY; + netdev_tx_t ret = NET_XMIT_DROP; struct net_device *dev; unsigned long tries; /* It is up to the caller to keep npinfo alive. */ @@ -327,11 +328,12 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) lockdep_assert_irqs_disabled(); dev = np->dev; + rcu_read_lock(); npinfo = rcu_dereference_bh(dev->npinfo); if (!npinfo || !netif_running(dev) || !netif_device_present(dev)) { dev_kfree_skb_irq(skb); - return NET_XMIT_DROP; + goto out; } /* don't get messages out of order, and no recursion */ @@ -370,7 +372,10 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb) skb_queue_tail(&npinfo->txq, skb); schedule_delayed_work(&npinfo->tx_work,0); } - return NETDEV_TX_OK; + ret = NETDEV_TX_OK; +out: + rcu_read_unlock(); + return ret; } netdev_tx_t netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
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> --- Changes in v2: - Use rcu_read_lock() instead of guard() as normal people do (Jakub). - Link to v1: https://lore.kernel.org/r/20250303-netpoll_rcu_v2-v1-1-6b34d8a01fa2@debian.org --- net/core/netpoll.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) --- base-commit: 848e076317446f9c663771ddec142d7c2eb4cb43 change-id: 20250303-netpoll_rcu_v2-fed72eb0cb83 Best regards,