Re: [PATCH net v3 3/3] pds_core: check info_regs in the identity debugfs reader

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

- [Medium] The newly added guard in `identity_show()` is an
  unsynchronized check-then-use of an `__iomem` pointer that a…

Pre-existing issues:
- [High] NULL-pointer dereference in the sibling debugfs reader
  `viftype_show()` (drivers/net/ethernet/amd/pds_core/debugfs.c:81-96).

--

pds_core: check info_regs in the identity debugfs reader

This adds a NULL test around the fw_heartbeat read in identity_show() so
the identity debugfs file does not dereference pdsc->info_regs after
pdsc_unmap_bars() has cleared it. The identity file is deliberately kept
across a reset, so the reader can run while the BARs are unmapped.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804235946.177762-1-nikhil.rao%40amd.com

[Severity: Medium]
Is the guard enough when the unmap happens concurrently with the read?
The test and the use are two separate plain loads of pdsc->info_regs,
and identity_show() holds none of the driver's locks:

	if (pdsc->info_regs)
		seq_printf(seq, "fw_heartbeat:     0x%x\n",
			   ioread32(&pdsc->info_regs->fw_heartbeat));

Meanwhile pdsc_unmap_bars() in main.c clears the pointer and then drops
the mapping:

	pdsc->info_regs = NULL;
	pdsc->cmd_regs = NULL;
	...
		if (bars[i].vaddr)
			pci_iounmap(pdsc->pdev, bars[i].vaddr);

So can the second load still observe NULL and make ioread32() fault at
offsetof(struct pds_core_dev_info_regs, fw_heartbeat)?  And even if the
compiler keeps the first value, can pci_iounmap() have already torn the
mapping down, leaving the ioread32() touching an unmapped ioremap range?

The other info_regs readers appear to be serialized against the unmap:
pdsc_check_pci_health() and pdsc_is_fw_running() run from the health work
under config_lock, which pdsc_reset_prepare() stops first, and the adminq
readers are gated by PDSC_S_FW_DEAD plus the adminq_refcnt drain in
pdsc_fw_down().  pdsc_reset_prepare() serializes the unmap only with
devcmd_lock:

	mutex_lock(&pdsc->devcmd_lock);
	pdsc_unmap_bars(pdsc);
	mutex_unlock(&pdsc->devcmd_lock);
	pci_release_regions(pdev);

which identity_show() never takes.  In pdsc_remove() the ordering looks
worse, since the lock is gone before the unmap and the debugfs consumer of
the mapping is only removed afterwards:

	mutex_destroy(&pdsc->devcmd_lock);
	...
	pdsc_unmap_bars(pdsc);
	pci_release_regions(pdev);
	...
	pdsc_debugfs_del_dev(pdsc);

Would snapshotting the pointer into a local plus serializing against
map/unmap (or removing the debugfs entries before unmapping the BARs) close
this properly?

[Severity: High]
This isn't a bug introduced by this patch, but the sibling reader in the
same file, viftype_show(), has the same shape of problem and no NULL test
at all:

	for (vt = 0; vt < PDS_DEV_TYPE_MAX; vt++) {
		if (!pdsc->viftype_status[vt].name)
			continue;

pdsc_teardown() with PDSC_TEARDOWN_REMOVING frees and clears the base
pointer:

	if (removing) {
		kfree(pdsc->viftype_status);
		pdsc->viftype_status = NULL;
	}

In pdsc_remove() that teardown runs well before pdsc_debugfs_del_dev(),
with destroy_workqueue(), two mutex_destroy() calls, pdsc_unmap_bars(),
pci_release_regions(), pci_disable_device(), pdsc_deferred_dma_free() and
ida_free() in between, so can a read of the still-registered viftypes file
dereference a NULL pdsc->viftype_status?  The same window looks present on
the pdsc_init_pf() error path, where err_out_teardown runs long before
pdsc_debugfs_del_dev().

The other readers of that member treat NULL as reachable, for example
pdsc_dl_find_viftype_by_id() in devlink.c:

	if (!pdsc->viftype_status)
		return NULL;

and host_mem_show() in this same file NULL-checks pdsc->host_mem_reqs.
Should viftype_show() get the same treatment while this area is being
touched?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901044219.1361466-1-nikhil.rao%40amd.com