Re: [PATCH v4 net-next 07/14] net: enetc: add support for "Add" and "Delete" operations to IPFT

[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.
---
net: enetc: add support for "Add" and "Delete" operations to IPFT

This commit adds support for the "Add" and "Delete" operations to the
ingress port filter table (IPFT). These interfaces will be used by both
the ENETC driver and the NETC switch driver.

> diff --git a/drivers/net/ethernet/freescale/enetc/ntmp.c b/drivers/net/ethernet/freescale/enetc/ntmp.c
> index 3c59b355c142..960d5be8ec42 100644
> --- a/drivers/net/ethernet/freescale/enetc/ntmp.c
> +++ b/drivers/net/ethernet/freescale/enetc/ntmp.c

[ ... ]

> @@ -468,6 +471,79 @@ int ntmp_rsst_query_entry(struct ntmp_user *user, u32 *table, int count)
>  }
>  EXPORT_SYMBOL_GPL(ntmp_rsst_query_entry);
>  
> +/**
> + * ntmp_ipft_add_entry - add an entry into the ingress port filter table
> + * @user: target ntmp_user struct
> + * @entry: the entry data, entry->cfge (configuration element data) and
> + * entry->keye (key element data) are used as input. Since the entry ID
> + * is assigned by the hardware, so entry->entry_id is a returned value
> + * for the driver to use, the driver can update/delete/query the entry
> + * based on the entry_id.
> + *
> + * Return: 0 on success, otherwise a negative error code
> + */
> +int ntmp_ipft_add_entry(struct ntmp_user *user,
> +			struct ipft_entry_data *entry)
> +{
[ ... ]
> +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> +	if (err)
> +		return err;
> +
> +	ntmp_fill_crd(&req->crd, user->tbl.ipft_ver, NTMP_QA_ENTRY_ID,
> +		      NTMP_GEN_UA_CFGEU | NTMP_GEN_UA_STSEU);
> +	req->ak.keye = entry->keye;
> +	req->cfge = entry->cfge;

The request sets NTMP_GEN_UA_STSEU to indicate statistics data is present,
but struct ipft_req_ua does not appear to contain a match_count or STSE data
field before cfge.

Since the hardware expects STSE data before CFGE data (as seen in
struct ipft_resp_query), will this cause the hardware to misinterpret the
first 8 bytes of cfge as the match_count, and then read the actual filter
configuration from out-of-bounds or uninitialized memory?

> diff --git a/drivers/net/ethernet/freescale/enetc/ntmp_private.h b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> index 7d50af7745c7..6d519d59a433 100644
> --- a/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> +++ b/drivers/net/ethernet/freescale/enetc/ntmp_private.h

[ ... ]

> +/* Ingress Port Filter Table Request Data Buffer Format of Update and
> + * Add actions
> + */
> +struct ipft_req_ua {
> +	struct ntmp_cmn_req_data crd;
> +	union ipft_access_key ak;
> +	struct ipft_cfge_data cfge;
> +};

Does this struct need an STSE data field before cfge to match the request
header flags?

> diff --git a/include/linux/fsl/ntmp.h b/include/linux/fsl/ntmp.h
> index a54945dcdc61..efda35e5e22c 100644
> --- a/include/linux/fsl/ntmp.h
> +++ b/include/linux/fsl/ntmp.h

[ ... ]

> @@ -66,6 +68,94 @@ struct maft_entry_data {
>  	struct maft_cfge_data cfge;
>  };
>  
> +struct ipft_pld_byte {
> +	u8 data;
> +	u8 mask;
> +};
> +
> +struct ipft_keye_data {
> +	__le16 precedence;
> +	__le16 resv0[3];
> +	__le16 frm_attr_flags;
> +#define IPFT_FAF_OVLAN		BIT(2)
> +#define IPFT_FAF_IVLAN		BIT(3)
> +#define IPFT_FAF_IP_HDR		BIT(7)
> +#define IPFT_FAF_IP_VER6	BIT(8)
> +#define IPFT_FAF_L4_CODE	GENMASK(11, 10)
> +#define  IPFT_FAF_TCP_HDR	1
> +#define  IPFT_FAF_UDP_HDR	2
> +#define  IPFT_FAF_SCTP_HDR	3
> +#define IPFT_FAF_WOL_MAGIC	BIT(12)
> +	__le16 frm_attr_flags_mask;
> +	__le16 dscp;
> +#define IPFT_DSCP		GENMASK(5, 0)
> +#define IPFT_DSCP_MASK		GENMASK(11, 0)

Does IPFT_DSCP_MASK completely overlap with IPFT_DSCP?

If it is defined as GENMASK(11, 0) instead of GENMASK(11, 6), using
FIELD_PREP with this mask will not shift the value into the upper bits,
which might overwrite the actual DSCP value in bits 0-5.