Re: [PATCH 5/6] pds_core: add host backed memory support for firmware

[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.
Full review at: https://sashiko.dev
---
--- Patch [5]: [PATCH 5/6] pds_core: add host backed memory support for firmware ---
commit 0970ce1c8fa02a37c722e99472916638286b6ad9
Author: Nikhil P. Rao <nikhil.rao@amd.com>

pds_core: add host backed memory support for firmware

Some newer AMD/Pensando cards have minimal memory and there are cases
where components, specifically in the control plane, need more memory.
This series adds support for host backed DMA memory that can be used
by the firmware for the previously mentioned cases.

> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
> index a7a0bcf98ed3f..356a69c895b63 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.c
> +++ b/drivers/net/ethernet/amd/pds_core/core.c

[ ... ]

> @@ -673,3 +675,161 @@ void pdsc_health_thread(struct work_struct *work)

[ ... ]

> +static int pdsc_host_mem_add_one(struct pdsc *pdsc, int index)
> +{
> +	struct pdsc_host_mem *hm = &pdsc->host_mem_reqs[index];
> +	union pds_core_dev_comp comp = {};
> +	union pds_core_dev_cmd cmd = {};
> +	int err;

[ ... ]

> +	/* Track this allocation so pdsc_host_mem_free() can clean it up */
> +	pdsc->num_host_mem_reqs++;
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	memset(&comp, 0, sizeof(comp));
> +	cmd.host_mem.opcode = PDS_CORE_CMD_HOST_MEM;
> +	cmd.host_mem.oper = PDS_CORE_HOST_MEM_ADD;
> +	cmd.host_mem.tag = cpu_to_le16(hm->tag);
> +	cmd.host_mem.size = cpu_to_le32(hm->size);
> +	cmd.host_mem.buf_pa = cpu_to_le64(hm->pa);
> +
> +	dev_dbg(pdsc->dev, "Sending devcmd for mem add tag %d size %d pa %pad\n",
> +		hm->tag, hm->size, &hm->pa);
> +	err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
> +	if (err || comp.status != PDS_RC_SUCCESS) {
> +		dev_err(pdsc->dev, "mem add failed err %d status %d for tag %d\n",
> +			err, comp.status, hm->tag);
> +		err = err ? err : -EIO;
> +		goto err_del;
> +	}
> +	dev_dbg(pdsc->dev, "mem add completed for tag %d\n", hm->tag);
> +
> +	return 0;
> +
> +err_del:
> +	/* After MEM_QUERY succeeds, firmware expects MEM_ADD or MEM_DEL */
> +	pdsc_host_mem_del_one(pdsc, hm->tag, PDS_RC_ENOMEM);
> +	return err;
> +}

[Severity: Medium]
When the MEM_ADD device command fails, does this error path leave the memory
allocated and DMA mapped?

Since pdsc->num_host_mem_reqs was incremented before the command, the memory
will eventually be freed when pdsc_host_mem_free() runs at driver teardown.
However, it appears up to 4MB of memory per failed request could be held
uselessly for the entire operational runtime of the driver. 

Should the driver explicitly unmap and free the memory in this error path?

[ ... ]

> +void pdsc_host_mem_free(struct pdsc *pdsc)
> +{
> +	int i;
> +
> +	if (!pdsc->host_mem_reqs)
> +		return;
> +
> +	for (i = 0; i < pdsc->num_host_mem_reqs; i++) {
> +		dma_unmap_page(pdsc->dev, pdsc->host_mem_reqs[i].pa,
> +			       pdsc->host_mem_reqs[i].size,
> +			       DMA_BIDIRECTIONAL);
> +		__free_pages(pdsc->host_mem_reqs[i].pg,
> +			     pdsc->host_mem_reqs[i].order);
> +	}
> +
> +	kfree(pdsc->host_mem_reqs);
> +	pdsc->host_mem_reqs = NULL;
> +	pdsc->num_host_mem_reqs = 0;
> +}

[Severity: High]
Does this code risk leaving the firmware with dangling DMA addresses?

The loop unmaps and frees the provisioned host memory pages, but it doesn't
appear to send a corresponding PDS_CORE_HOST_MEM_DEL device command to notify
the firmware that the memory has been revoked.

While pdsc_teardown() sends an explicit reset command for Physical Functions,
it skips this step for Virtual Functions. If a VF frees memory without sending
a MEM_DEL command, could the firmware still attempt to access the freed memory,
resulting in IOMMU faults or use-after-free corruption?