Re: [net-next,v7,11/13] octeontx2-pf: cn20k: Add TC rules support

[Jakub Kicinski <kuba@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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
octeontx2-pf: cn20k: Add TC rules support

This patch adds dynamic MCAM entry allocation for TC rules on CN20K,
with X2/X4 key width support and entry shifting based on priorities.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c

[ ... ]

> +int cn20k_tc_update_mcam_table_add_req(struct otx2_nic *nic,
> +				       struct otx2_flow_config *flow_cfg,
> +				       struct otx2_tc_flow *node)
> +{
> +	struct otx2_tc_flow *tmp;
> +	u16 cntr_val = 0;
> +	int list_idx, i;
> +	int entry, prev;
> +
> +	list_idx = otx2_tc_add_to_flow_list(flow_cfg, node);
> +	entry = node->entry;
> +	if (!cn20k_tc_check_entry_shiftable(nic, flow_cfg, node,
> +					    list_idx, true)) {
> +		/* Due to mix of X2 and X4, entries cannot be shifted.
> +		 * In this case free the entry allocated for this rule.
> +		 */
> +		if (cn20k_tc_free_mcam_entry(nic, entry))
> +			netdev_err(nic->netdev,
> +				   "Freeing entry %d to AF failed\n", entry);
> +		return -EINVAL;
                ^^^^^^^^^^^^^^

When this returns -EINVAL, the caller otx2_tc_add_flow() will then jump
to free_leaf and call cn20k_tc_free_mcam_entry() again with the same
entry value (stored in new_node->entry). Is this a double-free of the
MCAM entry?

> +	}

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c

[ ... ]

> +	if (is_cn20k(nic->pdev)) {
> +		rc = cn20k_tc_alloc_entry(nic, tc_flow_cmd, new_node, &dummy);
> +		if (rc) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "MCAM rule allocation failed");
> +			kfree_rcu(new_node, rcu);
> +			return rc;
> +		}
> +	}
> +
> +	entry = otx2_tc_update_mcam_table(nic, flow_cfg, new_node, true);
> +	if (entry < 0) {
> +		NL_SET_ERR_MSG_MOD(extack, "Adding rule failed");
> +		rc = entry;
> +		goto free_leaf;
> +	}
> +
>  	mutex_lock(&nic->mbox.lock);
>  	req = otx2_mbox_alloc_msg_npc_install_flow(&nic->mbox);
>  	if (!req) {
>  		mutex_unlock(&nic->mbox.lock);
>  		rc = -ENOMEM;
>  		goto free_leaf;
>  	}

[ ... ]

>  free_leaf:
> +	cn20k_tc_free_mcam_entry(nic, new_node->entry);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This function is called unconditionally in the error path, but on
non-CN20K devices new_node->entry was never initialized (it would be 0
from kzalloc). Would this send a mbox message to free MCAM entry 0 on
older silicon? Should this call be guarded with is_cn20k()?

Additionally, when cn20k_tc_update_mcam_table_add_req() fails due to
X2/X4 mix, it already frees the entry before returning -EINVAL. Reaching
this point would then attempt to free the same entry again.