Re: [net-next,PatchV2,2/2] Octeontx2-pf: Add support for DMAC_FILTER trap

[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: Add support for DMAC_FILTER trap

This patch adds DMAC_FILTER trap support to the octeontx2 driver, allowing
devlink trap statistics to be queried for DMAC filtered packets.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> index a72694219df4..9f1b78286695 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c

[ ... ]

> +int otx2_devlink_traps_register(struct otx2_nic *pf)
> +{
> +	const u32 groups_count = ARRAY_SIZE(otx2_trap_groups_arr);
> +	const u32 traps_count = ARRAY_SIZE(otx2_trap_items_arr);
> +	struct devlink *devlink = priv_to_devlink(pf->dl);
> +	struct otx2_trap_data *trap_data;
> +	struct otx2_trap *otx2_trap;
> +	int err, i;
> +
> +	trap_data = kzalloc(sizeof(*trap_data), GFP_KERNEL);
> +	if (!trap_data)
> +		return -ENOMEM;
> +
> +	trap_data->trap_items_arr = kcalloc(traps_count,
> +					    sizeof(struct otx2_trap_item),
> +					    GFP_KERNEL);
> +	if (!trap_data->trap_items_arr) {
> +		err = -ENOMEM;
> +		goto err_trap_items_alloc;
> +	}
> +
> +	trap_data->dl = pf->dl;
> +	trap_data->traps_count = traps_count;
> +	pf->dl->trap_data = trap_data;
> +
> +	err = devlink_trap_groups_register(devlink, otx2_trap_groups_arr,
> +					   groups_count);
> +	if (err)
> +		goto err_groups_register;

[ ... ]

> +err_groups_register:
> +	kfree(trap_data->trap_items_arr);
> +err_trap_items_alloc:
> +	kfree(trap_data);
> +	return err;
> +}

When this function fails after setting pf->dl->trap_data but before returning,
the error paths free trap_data while pf->dl->trap_data still references the
freed memory. Similarly, if kzalloc or kcalloc fails, pf->dl->trap_data remains
NULL. In both cases, the probe error path will call otx2_devlink_traps_unregister()
which dereferences pf->dl->trap_data unconditionally.

> +void otx2_devlink_traps_unregister(struct otx2_nic *pf)
> +{
> +	struct otx2_trap_data *trap_data = pf->dl->trap_data;
                                            ^^^^^^^^^^^^^^^^
If otx2_devlink_traps_register() failed due to allocation failure, trap_data
will be NULL here, causing a NULL pointer dereference below. If registration
failed after pf->dl->trap_data was set, this points to already-freed memory.

> +	struct devlink *devlink = priv_to_devlink(pf->dl);
> +	const struct devlink_trap *trap;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(otx2_trap_items_arr); ++i) {
> +		trap = &otx2_trap_items_arr[i].trap;
> +		devlink_traps_unregister(devlink, trap, 1);
> +	}
> +
> +	devlink_trap_groups_unregister(devlink, otx2_trap_groups_arr,
> +				       ARRAY_SIZE(otx2_trap_groups_arr));
> +	kfree(trap_data->trap_items_arr);
> +	kfree(trap_data);
> +}

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> index a7feb4c392b3..5da1605a1a90 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -3282,6 +3282,10 @@ static int otx2_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (err)
>  		goto err_mcam_flow_del;
>
> +	err = otx2_devlink_traps_register(pf);
> +	if (err)
> +		goto err_traps_unregister;
                     ^^^^^^^^^^^^^^^^^^^^
When otx2_devlink_traps_register() fails, is it correct to jump to a label
that calls otx2_devlink_traps_unregister()? The registration function already
cleaned up its allocations on failure, so calling unregister here would operate
on invalid or NULL trap_data.

Should the error path skip the unregister call, perhaps by jumping directly to
err_mcam_flow_del instead?

> +
>  	/* Initialize SR-IOV resources */
>  	err = otx2_sriov_vfcfg_init(pf);
>  	if (err)

[ ... ]

> @@ -3314,6 +3318,8 @@ static int otx2_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	otx2_sriov_vfcfg_cleanup(pf);
>  err_pf_sriov_init:
>  	otx2_shutdown_tc(pf);
> +err_traps_unregister:
> +	otx2_devlink_traps_unregister(pf);
>  err_mcam_flow_del:
>  	otx2_mcam_flow_del(pf);