Re: [net,v3,3/4] bnxt_en: Fix deleting of Ntuple filters

[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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
bnxt_en: Fix deleting of Ntuple filters

This commit fixes an issue where ntuple filters deleted when the
interface is down would cause FW errors. It saves a vnic pointer in the
filter's base structure to check if the VNIC is already freed.

Fixes: 8336a974f37d ("bnxt_en: Save user configured filters in a lookup list")

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 8fc15a8353fc..1f05ecf367fe 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -6240,6 +6244,9 @@ int bnxt_hwrm_cfa_ntuple_filter_free(struct bnxt *bp,
>  	int rc;
>
>  	set_bit(BNXT_FLTR_FW_DELETED, &fltr->base.state);
> +	if (fltr->base.vnic->fw_vnic_id == INVALID_HW_RING_ID)
> +		return 0;
> +

Does this code dereference fltr->base.vnic without checking if it's NULL?

For ntuple drop filters created with ring_cookie == RX_CLS_FLOW_DISC, the
code sets the BNXT_ACT_DROP flag. Looking at bnxt_hwrm_cfa_ntuple_filter_alloc:

int bnxt_hwrm_cfa_ntuple_filter_alloc(struct bnxt *bp,
                                      struct bnxt_ntuple_filter *fltr)
{
	...
	if (fltr->base.flags & BNXT_ACT_DROP) {
		req->flags =
			cpu_to_le32(CFA_NTUPLE_FILTER_ALLOC_REQ_FLAGS_DROP);
	} else if (bp->fw_cap & BNXT_FW_CAP_CFA_RFS_RING_TBL_IDX_V2) {
		bnxt_cfg_rfs_ring_tbl_idx(bp, req, fltr);
	} else {
		vnic = &bp->vnic_info[fltr->base.rxq + 1];
		fltr->base.vnic = vnic;
		req->dst_id = cpu_to_le16(vnic->fw_vnic_id);
	}

The BNXT_ACT_DROP case sets the drop flag but doesn't initialize
fltr->base.vnic. When that filter is later freed, this code dereferences
the NULL vnic pointer.

[ ... ]

> @@ -6339,6 +6348,7 @@ int bnxt_hwrm_cfa_ntuple_filter_alloc(struct bnxt *bp,
>  		bnxt_cfg_rfs_ring_tbl_idx(bp, req, fltr);
>  	} else {
>  		vnic = &bp->vnic_info[fltr->base.rxq + 1];
> +		fltr->base.vnic = vnic;
>  		req->dst_id = cpu_to_le16(vnic->fw_vnic_id);
>  	}

Looking at the if-else structure here:

	if (fltr->base.flags & BNXT_ACT_DROP) {
		req->flags =
			cpu_to_le32(CFA_NTUPLE_FILTER_ALLOC_REQ_FLAGS_DROP);
	} else if (bp->fw_cap & BNXT_FW_CAP_CFA_RFS_RING_TBL_IDX_V2) {
		bnxt_cfg_rfs_ring_tbl_idx(bp, req, fltr);
	} else {
		vnic = &bp->vnic_info[fltr->base.rxq + 1];
		fltr->base.vnic = vnic;
		req->dst_id = cpu_to_le16(vnic->fw_vnic_id);
	}

Should the BNXT_ACT_DROP case also set fltr->base.vnic?

Drop filters don't have a destination vnic, so the code doesn't set one.
But the free path in bnxt_hwrm_cfa_ntuple_filter_free now dereferences
fltr->base.vnic without a NULL check, which would crash when freeing drop
filters.

Either the drop case should explicitly set fltr->base.vnic = NULL and the
free path should check for NULL, or drop filters should be assigned a dummy
vnic. Currently the code leaves vnic uninitialized and then dereferences it.

[ ... ]