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

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

[Louis Peens]

From: Yinjun Zhang <yinjun.zhang@corigine.com>

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@vger.kernel.org
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Acked-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 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);
+}
+
+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);
 	nfp_net_reconfig_wait_posted(nn);
 }
-- 
2.34.1

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

[Jacob Keller]

On 6/28/2023 2:32 AM, Louis Peens wrote:
> From: Yinjun Zhang <yinjun.zhang@corigine.com>
> 
> 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@vger.kernel.org
> Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> Acked-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>
> ---
>  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);
>  }

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

[Jakub Kicinski]

On Wed, 28 Jun 2023 11:32:28 +0200 Louis Peens wrote:
> The configured mc addresses are not removed from application firmware
> when driver exits. This will cause resource leak when repeatedly
> creating and destroying VFs.

I think the justification is somewhat questionable, too.
VF is by definition not trusted. Does FLR not clean up the resources?

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

[Yinjun Zhang]

On Thursday, June 29, 2023 2:21 AM, Jacob Keller wrote:
> 
> 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.

Yes, you're right, I'll use _unsync. Thank you.

> 
> >       nfp_net_reconfig_wait_posted(nn);
> >  }

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

[Yinjun Zhang]

On Thursday, June 29, 2023 4:59 AM, Jakub Kicinski wrote:
> On Wed, 28 Jun 2023 11:32:28 +0200 Louis Peens wrote:
> > The configured mc addresses are not removed from application firmware
> > when driver exits. This will cause resource leak when repeatedly
> > creating and destroying VFs.
> 
> I think the justification is somewhat questionable, too.
> VF is by definition not trusted. Does FLR not clean up the resources?

Sorry, it doesn't. And I also find that moving netdev from one namespace
to another causes the same problem. So this fix is not for the VF case only.