Re: [PATCH net] nfp: clean mc addresses in application firmware when driver exits

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 6/28/2023 2:32 AM, Louis Peens wrote:
> From: Yinjun Zhang <yinjun.zhang@xxxxxxxxxxxx>
> 
> The configured mc addresses are not removed from application firmware
> when driver exits. This will cause resource leak when repeatedly
> creating and destroying VFs.
> 
> Now use list to track configured mc addresses and remove them when
> corresponding driver exits.
> 
> Fixes: e20aa071cd95 ("nfp: fix schedule in atomic context when sync mc address")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Yinjun Zhang <yinjun.zhang@xxxxxxxxxxxx>
> Acked-by: Simon Horman <simon.horman@xxxxxxxxxxxx>
> Signed-off-by: Louis Peens <louis.peens@xxxxxxxxxxxx>
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_net.h  |  8 +++
>  .../ethernet/netronome/nfp/nfp_net_common.c   | 66 +++++++++++++++++--
>  2 files changed, 67 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h
> index 939cfce15830..b079b7a92a1d 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
> @@ -621,6 +621,7 @@ struct nfp_net_dp {
>   * @mbox_amsg.lock:	Protect message list
>   * @mbox_amsg.list:	List of message to process
>   * @mbox_amsg.work:	Work to process message asynchronously
> + * @mc_list:		List of multicast mac address
>   * @app_priv:		APP private data for this vNIC
>   */
>  struct nfp_net {
> @@ -728,6 +729,8 @@ struct nfp_net {
>  		struct work_struct work;
>  	} mbox_amsg;
>  
> +	struct list_head mc_list;
> +
>  	void *app_priv;
>  };
>  
> @@ -738,6 +741,11 @@ struct nfp_mbox_amsg_entry {
>  	char msg[];
>  };
>  
> +struct nfp_mc_entry {
> +	struct list_head list;
> +	u8 addr[ETH_ALEN];
> +};
> +
>  int nfp_net_sched_mbox_amsg_work(struct nfp_net *nn, u32 cmd, const void *data, size_t len,
>  				 int (*cb)(struct nfp_net *, struct nfp_mbox_amsg_entry *));
>  
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> index 49f2f081ebb5..ccc49b330b51 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> @@ -1380,9 +1380,8 @@ static void nfp_net_mbox_amsg_work(struct work_struct *work)
>  	}
>  }
>  
> -static int nfp_net_mc_cfg(struct nfp_net *nn, struct nfp_mbox_amsg_entry *entry)
> +static int _nfp_net_mc_cfg(struct nfp_net *nn, unsigned char *addr, u32 cmd)
>  {
> -	unsigned char *addr = entry->msg;
>  	int ret;
>  
>  	ret = nfp_net_mbox_lock(nn, NFP_NET_CFG_MULTICAST_SZ);
> @@ -1394,12 +1393,30 @@ static int nfp_net_mc_cfg(struct nfp_net *nn, struct nfp_mbox_amsg_entry *entry)
>  	nn_writew(nn, nn->tlv_caps.mbox_off + NFP_NET_CFG_MULTICAST_MAC_LO,
>  		  get_unaligned_be16(addr + 4));
>  
> -	return nfp_net_mbox_reconfig_and_unlock(nn, entry->cmd);
> +	return nfp_net_mbox_reconfig_and_unlock(nn, cmd);
> +}

Ok so the motivation here is to allow separating the command from the
entry so that you can call it during the npf_net_mc_clean().


> +
> +static void nfp_net_mc_clean(struct nfp_net *nn)
> +{
> +	struct nfp_mc_entry *entry, *tmp;
> +
> +	list_for_each_entry_safe(entry, tmp, &nn->mc_list, list) {
> +		_nfp_net_mc_cfg(nn, entry->addr, NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL);
> +		list_del(&entry->list);
> +		kfree(entry);
> +	}
> +}
> +
> +static int nfp_net_mc_cfg(struct nfp_net *nn, struct nfp_mbox_amsg_entry *entry)
> +{
> +	return _nfp_net_mc_cfg(nn, entry->msg, entry->cmd);
>  }
>  
>  static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char *addr)
>  {
>  	struct nfp_net *nn = netdev_priv(netdev);
> +	struct nfp_mc_entry *entry, *tmp;
> +	int err;
>  
>  	if (netdev_mc_count(netdev) > NFP_NET_CFG_MAC_MC_MAX) {
>  		nn_err(nn, "Requested number of MC addresses (%d) exceeds maximum (%d).\n",
> @@ -1407,16 +1424,48 @@ static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char *addr)
>  		return -EINVAL;
>  	}
>  
> -	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
> -					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
> +	list_for_each_entry_safe(entry, tmp, &nn->mc_list, list) {
> +		if (ether_addr_equal(entry->addr, addr)) /* already existed */
> +			return 0;
> +	}
> +
> +	entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	err = nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
> +					   NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
> +	if (!err) {
> +		ether_addr_copy(entry->addr, addr);
> +		list_add_tail(&entry->list, &nn->mc_list);
> +	} else {
> +		kfree(entry);
> +	}
> +
> +	return err;
>  }
>  
>  static int nfp_net_mc_unsync(struct net_device *netdev, const unsigned char *addr)
>  {
>  	struct nfp_net *nn = netdev_priv(netdev);
> +	struct nfp_mc_entry *entry, *tmp;
> +	int err;
> +
> +	list_for_each_entry_safe(entry, tmp, &nn->mc_list, list) {
> +		if (ether_addr_equal(entry->addr, addr)) {
> +			err = nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL,
> +							   addr, NFP_NET_CFG_MULTICAST_SZ,
> +							   nfp_net_mc_cfg);
> +			if (!err) {
> +				list_del(&entry->list);
> +				kfree(entry);
> +			}
> +
> +			return err;
> +		}
> +	}
>  
> -	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL, addr,
> -					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
> +	return -ENOENT;
>  }
>  
>  static void nfp_net_set_rx_mode(struct net_device *netdev)
> @@ -2687,6 +2736,8 @@ int nfp_net_init(struct nfp_net *nn)
>  			goto err_clean_mbox;
>  
>  		nfp_net_ipsec_init(nn);
> +
> +		INIT_LIST_HEAD(&nn->mc_list);
>  	}
>  
>  	nfp_net_vecs_init(nn);
> @@ -2718,5 +2769,6 @@ void nfp_net_clean(struct nfp_net *nn)
>  	nfp_net_ipsec_clean(nn);
>  	nfp_ccm_mbox_clean(nn);
>  	flush_work(&nn->mbox_amsg.work);
> +	nfp_net_mc_clean(nn);

Is there no way to just ask the kernel what addresses you already have
and avoid the need for a separate copy maintained in the driver? Or
maybe thats something that could be added since this doesn't seem like a
unique problem.

In fact, we absolutely can:

__dev_mc_unsync which is the opposite of __dev_mc_sync.

You can just call that during tear down with an unsync function and you
shouldn't need to bother maintaining your own list at all.

>  	nfp_net_reconfig_wait_posted(nn);
>  }



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux