Re: [PATCH v4 net-next 06/14] net: enetc: add support for the "Update" operation to buffer pool table

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: enetc: add support for the "Update" operation to buffer pool table

This patch adds the ntmp_bpt_update_entry() helper function to support
updating a specified entry in the buffer pool table. It provides the
ability to update buffer pool configuration data.

> diff --git a/drivers/net/ethernet/freescale/enetc/ntmp_private.h b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> --- a/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> +++ b/drivers/net/ethernet/freescale/enetc/ntmp_private.h

[ ... ]

> +/* Buffer Pool Table Request Data Buffer Format of Update action */
> +struct bpt_req_update {
> +	struct ntmp_req_by_eid rbe;
> +	struct bpt_cfge_data cfge;
> +};
> +

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/ntmp.c b/drivers/net/ethernet/freescale/enetc/ntmp.c
> --- a/drivers/net/ethernet/freescale/enetc/ntmp.c
> +++ b/drivers/net/ethernet/freescale/enetc/ntmp.c

[ ... ]

> @@ -702,5 +708,38 @@ int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
>  }
>  EXPORT_SYMBOL_GPL(ntmp_vft_add_entry);
>  
> +int ntmp_bpt_update_entry(struct ntmp_user *user, u32 entry_id,
> +			  const struct bpt_cfge_data *cfge)
> +{
> +	struct ntmp_dma_buf data = {
> +		.dev = user->dev,
> +		.size = sizeof(struct bpt_req_update),
> +	};
> +	struct bpt_req_update *req;
> +	union netc_cbd cbd;
> +	int err;
> +
> +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> +	if (err)
> +		return err;
> +
> +	ntmp_fill_crd_eid(&req->rbe, user->tbl.bpt_ver, 0,
> +			  NTMP_GEN_UA_CFGEU | BPT_UA_BPSEU, entry_id);

Does this update action mask match the size of the request buffer?

The mask includes both NTMP_GEN_UA_CFGEU and BPT_UA_BPSEU, which instructs
the hardware to update both the configuration entry and the status entry.
However, struct bpt_req_update only contains the configuration data,
omitting the status data.

If the hardware attempts to read the missing BPSE data, could it reject
the command due to an undersized request length, or read beyond the
intended payload into zero-initialized DMA padding? This might overwrite
critical buffer pool tracking variables like amount_used_hwm.

If only the configuration is being updated, should the BPT_UA_BPSEU flag
be removed?

> +	req->cfge = *cfge;
> +	ntmp_fill_request_hdr(&cbd, data.dma, NTMP_LEN(data.size, 0),
> +			      NTMP_BPT_ID, NTMP_CMD_UPDATE, NTMP_AM_ENTRY_ID);
> +
> +	err = netc_xmit_ntmp_cmd(user, &cbd);
> +	if (err)
> +		dev_err(user->dev,
> +			"Failed to update %s entry 0x%x, err: %pe\n",
> +			ntmp_table_name(NTMP_BPT_ID), entry_id, ERR_PTR(err));
> +
> +	ntmp_free_data_mem(&data);

Can this result in a DMA use-after-free if the hardware command times out?

If netc_xmit_ntmp_cmd() times out waiting for the hardware to process the
command, it returns -ETIMEDOUT without resetting the hardware ring or
explicitly cancelling the command.

The descriptor remains active in the ring, and ntmp_free_data_mem()
unconditionally frees the DMA-mapped buffer. If the hardware eventually
processes the descriptor, it will read from the now-freed DMA memory.

Could this lead to an IOMMU fault or cause the hardware to read garbage
data if the page has been reallocated?

> +
> +	return err;
> +}