[PATCH net v2] netpoll: hold rcu read lock in __netpoll_send_skb()

[PATCH net v2] netpoll: hold rcu read lock in __netpoll_send_skb()

[Breno Leitao]

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(-)

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)

---
base-commit: 848e076317446f9c663771ddec142d7c2eb4cb43
change-id: 20250303-netpoll_rcu_v2-fed72eb0cb83

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

Re: [PATCH net v2] netpoll: hold rcu read lock in __netpoll_send_skb()

[Simon Horman]

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>
> 

Re: [PATCH net v2] netpoll: hold rcu read lock in __netpoll_send_skb()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html