Re: [PATCH net] octeontx2-pf: mcs: Fix mcs resources free on PF shutdown

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

Simon says:

There is also an AI-generated review available at https://sashiko.dev
However, I do not believe it raises anything that needs to
be addressed in the context of this patch that is not already 
raised below.

---
Subject: octeontx2-pf: mcs: Fix mcs resources free on PF shutdown

This patch gates the MACSEC resource cleanup in cn10k_mcs_free() on
the txsc_list being non-empty, with the intent of skipping the
mailbox messages when no MCS resources were ever allocated to the
PF. It also sets clear_req->all in cn10k_mcs_free_rsrc().

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> index 2cc1bdfd9b2e..f25809c9d393 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> @@ -182,6 +182,7 @@ static void cn10k_mcs_free_rsrc(struct otx2_nic *pfvf, enum mcs_direction dir,
>  	clear_req->id = hw_rsrc_id;
>  	clear_req->type = type;
>  	clear_req->dir = dir;
> +	clear_req->all = all;

[Low]
Could the commit message mention this change?  Before the patch,
clear_req->all was never assigned and the mcs_clear_stats mailbox
always asked the AF to clear stats for the single resource named by
id.  After the patch, calls from cn10k_mcs_free() pass all=true and
the AF will now be asked to clear all PF-mapped resource stats.

That looks consistent with the pre-existing req->all = 1 on the
matching mcs_free_resources message, but it is an additional fix to
the AF mailbox protocol that the changelog does not describe and
that may want its own Fixes: tag for backporting.

>  
>  	req = otx2_mbox_alloc_msg_mcs_free_resources(mbox);
>  	if (!req)
> @@ -1776,9 +1777,14 @@ int cn10k_mcs_init(struct otx2_nic *pfvf)
>  
>  void cn10k_mcs_free(struct otx2_nic *pfvf)
>  {
> +	struct cn10k_mcs_cfg *cfg = pfvf->macsec_cfg;
> +
>  	if (!test_bit(CN10K_HW_MACSEC, &pfvf->hw.cap_flag))
>  		return;
>  
> +	if (list_empty(&cfg->txsc_list))
> +		return;
> +

[High]
Does this early return leak pfvf->macsec_cfg?

cn10k_mcs_init() unconditionally allocates cfg whenever
CN10K_HW_MACSEC is set, and initializes both lists empty:

	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
	...
	INIT_LIST_HEAD(&cfg->txsc_list);
	INIT_LIST_HEAD(&cfg->rxsc_list);
	pfvf->macsec_cfg = cfg;

txsc_list only becomes non-empty once the user creates a SecY via
cn10k_mdo_add_secy().  On any MACSEC-capable PF where the user never
configures MACSEC offload, txsc_list is still empty at otx2_remove()
time, and the new early return skips the trailing kfree:

>  	cn10k_mcs_free_rsrc(pfvf, MCS_TX, MCS_RSRC_TYPE_SECY, 0, true);
>  	cn10k_mcs_free_rsrc(pfvf, MCS_RX, MCS_RSRC_TYPE_SECY, 0, true);
>  	kfree(pfvf->macsec_cfg);
>  	pfvf->macsec_cfg = NULL;
>  }

The same path runs from the err_mcs_free probe error label in
otx2_probe() when cn10k_ipsec_init() fails, so the leak also fires
there.

Would gating only the two cn10k_mcs_free_rsrc() mailbox calls on
the empty-list condition, and keeping the kfree unconditional, work
better?  Something like:

	if (!list_empty(&cfg->txsc_list)) {
		cn10k_mcs_free_rsrc(pfvf, MCS_TX, MCS_RSRC_TYPE_SECY, 0, true);
		cn10k_mcs_free_rsrc(pfvf, MCS_RX, MCS_RSRC_TYPE_SECY, 0, true);
	}
	kfree(pfvf->macsec_cfg);
	pfvf->macsec_cfg = NULL;