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

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

[Subbaraya Sundeep]

From: Geetha sowjanya <gakula@marvell.com>

On PF shutdown, the current driver free mcs hardware
resources though mcs resources are not allocated to it.
This patch checks the mcs resources status and if resources
are allocated then only sends mailbox message to free them.

Fixes: c54ffc73601c ("octeontx2-pf: mcs: Introduce MACSEC hardware offloading")
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
---
 drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c | 6 ++++++
 1 file changed, 6 insertions(+)

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;
 
 	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;
+
 	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);
-- 
2.48.1

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

[Simon Horman]

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;