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

[Simon Horman <horms@kernel.org>]

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