* Re: [PATCH net v2] pds_core: fix cmd_regs access racing BAR unmap on reset
2026-08-04 23:59 [PATCH net v2] pds_core: fix cmd_regs access racing BAR unmap on reset Nikhil P. Rao
@ 2026-08-07 23:47 ` Jakub Kicinski
0 siblings, 0 replies; 2+ messages in thread
From: Jakub Kicinski @ 2026-08-07 23:47 UTC (permalink / raw)
To: nikhil.rao
Cc: Jakub Kicinski, netdev, brett.creeley, eric.joyner, andrew+netdev,
davem, edumazet, pabeni
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
pds_core: fix cmd_regs access racing BAR unmap on reset
This takes devcmd_lock across the BAR unmap in pdsc_reset_prepare() and
the remap in pdsc_reset_done(), skips the unmap entirely for VFs, and adds
cmd_regs NULL checks in pdsc_core_init(), pdsc_identify() and the
pdsc_firmware_update() download loop.
Closes: https://sashiko.dev/#/patchset/20260708212222.296202-1-nikhil.rao%40amd.com?part=3
A few questions below.
> pdsc_unmap_bars() also clears info_regs, intr_status, intr_ctrl and
> db_pages.
[Severity: Low]
Is this part of the changelog accurate? pdsc_unmap_bars() clears exactly
four pointers:
pdsc->info_regs = NULL;
pdsc->cmd_regs = NULL;
pdsc->intr_status = NULL;
pdsc->intr_ctrl = NULL;
It never touches pdsc->db_pages or pdsc->phy_db_pages. Grepping the
driver, db_pages is assigned only in pdsc_map_bars():
pdsc->db_pages = bar->vaddr;
pdsc->phy_db_pages = bar->bus_addr;
and bar there is &bars[1], whose vaddr was forced to NULL by the earlier
"only map the whole bar 0" branch:
/* only map the whole bar 0 */
if (j > 0) {
bars[j].vaddr = NULL;
So db_pages is always NULL and never a live mapping. Would "db_pages is
never mapped" be the correct justification here, rather than saying the
unmap clears it?
> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
> index 04ec2569c61c0..c3c27a7c63d0e 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.c
> +++ b/drivers/net/ethernet/amd/pds_core/core.c
> @@ -364,6 +364,14 @@ static int pdsc_core_init(struct pdsc *pdsc)
>
> mutex_lock(&pdsc->devcmd_lock);
>
> + if (!pdsc->cmd_regs) {
> + mutex_unlock(&pdsc->devcmd_lock);
> + err = -ENXIO;
> + dev_err(pdsc->dev, "Device init command failed: %pe\n",
> + ERR_PTR(err));
> + goto err_out_uninit;
> + }
> +
[Severity: Medium]
Does this new bail-out actually avoid the unmapped-MMIO access it is meant
to avoid? The err_out_uninit path leads to interrupt-control register
writes through the same BAR0 mapping:
pdsc_core_init() -> err_out_uninit -> pdsc_core_uninit() -> pdsc_qcq_free()
-> pdsc_qcq_intr_free() -> pdsc_intr_free()
and pdsc_intr_free() does:
pds_core_intr_mask(&pdsc->intr_ctrl[index], PDS_CORE_INTR_MASK_SET);
pds_core_intr_clean(&pdsc->intr_ctrl[index]);
free_irq(intr_info->vector, intr_info->data);
pdsc_unmap_bars() clears cmd_regs and intr_ctrl in the same four
statements, so cmd_regs == NULL implies intr_ctrl == NULL. The adminq QCQ
was just allocated in this function with PDS_CORE_QCQ_F_INTR, so
intr_info->vector is non-zero and the early return in pdsc_intr_free() is
not taken. Does that turn the memcpy_toio() to a NULL cmd_regs into an
iowrite32() at &((struct pds_core_intr *)NULL)[index].mask, and also skip
the intended mask/credit-clean before free_irq() drops the handler?
There is a related asymmetry earlier in the same call sequence:
pdsc_qcq_alloc() -> pdsc_intr_alloc() dereferences pdsc->intr_ctrl in
pds_core_intr_clean_flags(&pdsc->intr_ctrl[index],
PDS_CORE_INTR_CRED_RESET_COALESCE);
with no NULL check, and pdsc_start() does the same with
pds_core_intr_mask(&pdsc->intr_ctrl[pdsc->adminqcq.intx],
PDS_CORE_INTR_MASK_CLEAR);
Is the window this check covers reachable at all? If the BARs cannot
vanish inside pdsc_core_init(), this is dead code; if they can, the
intr_ctrl users around it look equally exposed.
[Severity: Low]
This isn't a bug, but the three new checks for the same condition report it
three different ways.
Here pdsc_core_init() logs "Device init command failed: -ENXIO" on a path
that unlocks and jumps to err_out_uninit before pdsc_devcmd_locked() is
ever called, and the string is identical to the one emitted just below for
a real devcmd failure:
err = pdsc_devcmd_locked(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
...
if (err) {
dev_err(pdsc->dev, "Device init command failed: %pe\n",
ERR_PTR(err));
Would something like "BARs unmapped, skipping device init" make the two
distinguishable in dmesg?
pdsc_identify() returns -ENXIO with no log line at all, and
pdsc_firmware_update() now has two checks of the same condition on the same
devlink flash_update path: the pre-existing entry check
if (!pdsc->cmd_regs)
return -ENXIO;
returns without an extack, while the new in-loop check sets "Device reset
during flash". Should userspace see the same extack regardless of which
one fires?
> diff --git a/drivers/net/ethernet/amd/pds_core/dev.c b/drivers/net/ethernet/amd/pds_core/dev.c
> index bded6b33289ce..ec96f67dc67b9 100644
> --- a/drivers/net/ethernet/amd/pds_core/dev.c
> +++ b/drivers/net/ethernet/amd/pds_core/dev.c
> @@ -294,6 +294,11 @@ static int pdsc_identify(struct pdsc *pdsc)
> */
> mutex_lock(&pdsc->devcmd_lock);
>
> + if (!pdsc->cmd_regs) {
> + mutex_unlock(&pdsc->devcmd_lock);
> + return -ENXIO;
> + }
> +
> sz = min_t(size_t, sizeof(drv), sizeof(pdsc->cmd_regs->data));
> memcpy_toio(&pdsc->cmd_regs->data, &drv, sz);
>
> diff --git a/drivers/net/ethernet/amd/pds_core/fw.c b/drivers/net/ethernet/amd/pds_core/fw.c
> index fa626719e68d1..cd7616ed9ef32 100644
> --- a/drivers/net/ethernet/amd/pds_core/fw.c
> +++ b/drivers/net/ethernet/amd/pds_core/fw.c
> @@ -134,6 +134,12 @@ int pdsc_firmware_update(struct pdsc *pdsc, const struct firmware *fw,
>
> copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
> mutex_lock(&pdsc->devcmd_lock);
> + if (!pdsc->cmd_regs) {
> + mutex_unlock(&pdsc->devcmd_lock);
> + err = -ENXIO;
> + NL_SET_ERR_MSG_MOD(extack, "Device reset during flash");
> + goto err_out;
> + }
> memcpy_toio(&pdsc->cmd_regs->data, fw->data + offset, copy_sz);
> err = pdsc_devcmd_fw_download_locked(pdsc, data_addr,
> offset, copy_sz);
> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index 9a2c64198d03b..1b960139de4e4 100644
> --- a/drivers/net/ethernet/amd/pds_core/main.c
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -507,7 +507,11 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
> pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
> }
>
> - pdsc_unmap_bars(pdsc);
> + if (!pdev->is_virtfn) {
> + mutex_lock(&pdsc->devcmd_lock);
> + pdsc_unmap_bars(pdsc);
> + mutex_unlock(&pdsc->devcmd_lock);
> + }
[Severity: High]
This isn't a bug introduced by this patch, and the changelog already calls
the debugfs readers out of scope, but the info_regs case looks like it can
oops without any race at all, so it seems worth mentioning here.
identity_show() reads the heartbeat register with no NULL check:
seq_printf(seq, "fw_heartbeat: 0x%x\n",
ioread32(&pdsc->info_regs->fw_heartbeat));
Every other reader of that same register tests the pointer first, for
example pdsc_is_fw_running():
if (!pdsc->info_regs)
return false;
pdsc_unmap_bars() sets pdsc->info_regs = NULL, and the "identity" file
deliberately survives a reset, per pdsc_debugfs_add_ident():
/* This file will already exist in the reset flow */
dentry = debugfs_lookup("identity", pdsc->dentry);
So besides the FLR window, if pdsc_reset_done() bails out early at
pci_enable_device(), pci_request_regions() or pdsc_map_bars(), does
info_regs stay NULL permanently while the file stays registered, making
every subsequent read of /sys/kernel/debug/pds_core/<pci-name>/identity
dereference NULL plus the fw_heartbeat offset?
pdsc_remove() also calls pdsc_unmap_bars() before pdsc_debugfs_del_dev(),
which leaves the same window during unbind.
Note that the qcq intr_ctrl regset in debugfs does not have this problem:
pdsc_debugfs_del_qcq() runs from pdsc_qcq_free() via pdsc_fw_down() ->
pdsc_teardown() -> pdsc_core_uninit(), which is before the unmap.
> pci_release_regions(pdev);
[Severity: Low]
This is a pre-existing asymmetry rather than something this patch
introduces, but the new is_virtfn guard right above it makes it visible:
pci_release_regions() still runs for VFs, and the VF path never requests
regions. pdsc_init_vf() has no pci_request_regions() call, and the
counterpart in pdsc_reset_done() is PF-only:
if (!pdev->is_virtfn) {
...
err = pci_request_regions(pdsc->pdev, PDS_CORE_DRV_NAME);
For a VF this reaches pci_release_region() -> release_mem_region() on a
range that was never reserved, so __release_region() prints "Trying to free
nonexistent resource" per memory BAR on every VF FLR. Should
pci_release_regions() move inside the same !pdev->is_virtfn block?
> if (pci_is_enabled(pdev))
> pci_disable_device(pdev);
[ ... ]
--
pw-bot: cr
^ permalink raw reply [flat|nested] 2+ messages in thread