Re: [PATCH net 1/2] net: disable netpoll on fresh napis

[Eric Dumazet <eric.dumazet@gmail.com>]

On 8/26/20 12:40 PM, Jakub Kicinski wrote:
> napi_disable() makes sure to set the NAPI_STATE_NPSVC bit to prevent
> netpoll from accessing rings before init is complete. However, the
> same is not done for fresh napi instances in netif_napi_add(),
> even though we expect NAPI instances to be added as disabled.
> 
> This causes crashes during driver reconfiguration (enabling XDP,
> changing the channel count) - if there is any printk() after
> netif_napi_add() but before napi_enable().
> 
> To ensure memory ordering is correct we need to use RCU accessors.
> 
> Reported-by: Rob Sherwood <rsher@fb.com>
> Fixes: 2d8bff12699a ("netpoll: Close race condition between poll_one_napi and napi_disable")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/dev.c     | 3 ++-
>  net/core/netpoll.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d42c9ea0c3c0..95ac7568f693 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6612,12 +6612,13 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
>  		netdev_err_once(dev, "%s() called with weight %d\n", __func__,
>  				weight);
>  	napi->weight = weight;
> -	list_add(&napi->dev_list, &dev->napi_list);
>  	napi->dev = dev;
>  #ifdef CONFIG_NETPOLL
>  	napi->poll_owner = -1;
>  #endif
>  	set_bit(NAPI_STATE_SCHED, &napi->state);
> +	set_bit(NAPI_STATE_NPSVC, &napi->state);
> +	list_add_rcu(&napi->dev_list, &dev->napi_list);
>  	napi_hash_add(napi);
>  }
>  EXPORT_SYMBOL(netif_napi_add);
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 093e90e52bc2..2338753e936b 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -162,7 +162,7 @@ static void poll_napi(struct net_device *dev)
>  	struct napi_struct *napi;
>  	int cpu = smp_processor_id();
>  
> -	list_for_each_entry(napi, &dev->napi_list, dev_list) {
> +	list_for_each_entry_rcu(napi, &dev->napi_list, dev_list) {
>  		if (cmpxchg(&napi->poll_owner, -1, cpu) == -1) {
>  			poll_one_napi(napi);
>  			smp_store_release(&napi->poll_owner, -1);
> 

You added rcu in this patch (without anything in the changelog).

netpoll_poll_dev() uses rcu_dereference_bh(), suggesting you might need list_for_each_entry_rcu_bh()