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

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); }