Re: [net-next,v5,10/13] octeontx2-af: npc: cn20k: Allocate MCAM entry for flow installation

[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-af: npc: cn20k: Allocate MCAM entry for flow installation

This patch extends the install_flow mailbox request message with additional
fields that allow the AF driver to determine the required NPC MCAM entry
type, allocate the MCAM entry, and complete the flow installation in a
single step for CN20K.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> index 052d989f2d9a..efd994c65b32 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c

[ ... ]

> +static int otx2_get_kw_type(struct otx2_nic *pfvf,
> +			    struct npc_install_flow_req *fl_req,
> +			    u8 *kw_type)
> +{
> +	struct npc_get_num_kws_req *req;
> +	struct npc_get_num_kws_rsp *rsp;
> +	u8 *src, *dst;
> +	int off, err;
> +	int kw_bits;
> +
> +	off = offsetof(struct npc_install_flow_req, packet);
> +
> +	mutex_lock(&pfvf->mbox.lock);
> +
> +	req = otx2_mbox_alloc_msg_npc_get_num_kws(&pfvf->mbox);
> +
> +	dst = (u8 *)&req->fl + off;
                  ^^^

Can this dereference a NULL pointer? otx2_mbox_alloc_msg_* functions can
return NULL on allocation failure, and this line accesses req->fl without
first checking if req is NULL.

Other similar functions in this file (e.g., otx2_mcam_pfl_info_get(),
otx2_get_dft_rl_idx()) properly check for NULL after the mailbox
allocation before using the returned pointer.

> +	src = (u8 *)fl_req + off;
> +
> +	memcpy(dst, src, sizeof(struct npc_install_flow_req) - off);

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> index 963d12ecd328..5fe077935c97 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c

[ ... ]

> @@ -1759,8 +1857,11 @@ int rvu_mbox_handler_npc_install_flow(struct rvu *rvu,
>  		enable = true;
>
>  	/* Do not allow requests from uninitialized VFs */
> -	if (from_vf && !enable)
> +	if (from_vf && !enable) {
> +		rvu_npc_free_entry_for_flow_install(rvu, req->hdr.pcifunc,
> +						    allocated, req->entry);
>  		return NPC_FLOW_VF_NOT_INIT;
> +	}
>
>  	/* PF sets VF mac & VF NIXLF is not attached, update the mac addr */
>  	if (pf_set_vfs_mac && !enable) {
>  		ether_addr_copy(pfvf->default_mac, req->packet.dmac);
>  		ether_addr_copy(pfvf->mac_addr, req->packet.dmac);
>  		set_bit(PF_SET_VF_MAC, &pfvf->flags);
>  		return 0;
                ^^^^^^^^^^

Does this path leak the allocated MCAM entry? When rvu_npc_alloc_entry_for_
flow_install() allocates an entry (setting allocated=true), but then this
early return path is taken, rvu_npc_free_entry_for_flow_install() is not
called. All other early return paths in this function properly free the
entry, but this one does not.

>  	}
>
>  	mutex_lock(&rswitch->switch_lock);