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