Re: [PATCH 4/6] pds_core: add PLDM component info display

[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 [4]: [PATCH 4/6] pds_core: add PLDM component info display ---
commit 4d0cc8aef76c7e2e30f98535495312200941b646
Author: Brett Creeley <brett.creeley@amd.com>

pds_core: add PLDM component info display
    
Add detailed component information display via devlink info. This
allows users to see individual firmware components and their versions.
Components are reported as fixed, running, or stored based on their
firmware-provided flags.

> diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
> index 3b763ee1715ee..63fe45e91f710 100644
> --- a/drivers/net/ethernet/amd/pds_core/devlink.c
> +++ b/drivers/net/ethernet/amd/pds_core/devlink.c

[ ... ]

> +static int pdsc_dl_component_info_get(struct devlink *dl,
> +				      struct devlink_info_req *req,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct pdsc *pdsc = devlink_priv(dl);
> +	u8 num_components;
> +	int err;
> +	int i;
> +
> +	/* Pairs with WRITE_ONCE in pdsc_fw_components_invalidate().
> +	 * Use READ_ONCE to get a consistent snapshot of num_components.
> +	 * pdsc_fw_components_invalidate() can zero it concurrently during
> +	 * firmware recovery; using the local copy avoids iterating zero
> +	 * times when we already decided the cache was valid.
> +	 */
> +	num_components = READ_ONCE(pdsc->fw_components.num_components);
> +	if (!num_components) {
> +		err = pdsc_get_component_info(pdsc);

[Severity: High]
Does this code expose uninitialized memory to concurrent readers?

If two threads execute this concurrently after the firmware component list
has been invalidated, one thread could enter pdsc_get_component_info(),
which updates the count before the array is populated:

drivers/net/ethernet/amd/pds_core/fw.c:pdsc_get_component_info() {
    ...
	pdsc->fw_components.num_components = num_components;
	for (i = 0; i < num_components; i++) {
		struct pds_core_fw_component_info *info =
			&pdsc->fw_components.info[i];

		memcpy(info, &list_info->info[i], sizeof(*info));
    ...
}

A second thread could concurrently execute pdsc_dl_component_info_get(), see
num_components > 0 via READ_ONCE(), and proceed to iterate over the array
while the first thread is still executing memcpy() into the array.

Could this be addressed by moving the update of num_components to after the
array population is complete, perhaps using smp_store_release() and
smp_load_acquire() to ensure proper ordering?

> +		if (err)
> +			return pdsc_dl_report_fw_ver(req,
> +						    pdsc->dev_info.fw_version);
> +		num_components = READ_ONCE(pdsc->fw_components.num_components);
> +		if (!num_components)
> +			return pdsc_dl_report_fw_ver(req,
> +						    pdsc->dev_info.fw_version);
> +	}
> +
> +	num_components = min_t(u16, num_components,
> +			       le16_to_cpu(pdsc->dev_ident.max_fw_slots));
> +	for (i = 0; i < num_components; i++) {
> +		err = pdsc_dl_report_component(req,
> +					       &pdsc->fw_components.info[i]);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}