Re: [PATCH RFC v2 1/5] PCI: Report surprise removal event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: workqueue list corruption via TOCTTOU in pci_dev_set_disconnected
--

commit 95014e29e88ff3b1020861e3c7351ac0913e2410
Author: Michael S. Tsirkin <mst@redhat.com>

PCI: Report surprise removal event

This commit introduces a mechanism to notify drivers about surprise device
disconnects via a work struct. It allows drivers to perform necessary cleanup 
knowing the device is inactive, preventing the remove callback from hanging 
forever when waiting for interrupts.

Link: https://lore.kernel.org/all/fba3d235e38c1c6fcef2a30ed083ad9e25b20fa3.1752094439.git.mst@redhat.com/

> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ba3c3fddddc23..23b1605e783a3 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -805,6 +805,12 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
>  	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
>  	pci_doe_disconnected(dev);
>  
> +	if (READ_ONCE(dev->disconnect_work_enable)) {
> +		/* Make sure work is up to date. */
> +		smp_rmb();
> +		schedule_work(&dev->disconnect_work);
> +	}

[Severity: High]
Can this lockless check cause a race condition if the CPU is preempted right
after the READ_ONCE(), but before schedule_work() executes?

If preemption occurs here, the driver could be concurrently unbound, calling
pci_clear_disconnect_work().

If a new driver then binds and initializes the work item via 
INIT_WORK(&dev->disconnect_work, ...) during its probe, the delayed
schedule_work() from the preempted CPU might execute concurrently with the
new initialization.

[ ... ]

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d31a8d107b1ef..f4c0240b62dc8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
[ ... ]
> @@ -2123,6 +2126,49 @@ pci_release_mem_regions(struct pci_dev *pdev)
[ ... ]
> +static inline void pci_clear_disconnect_work(struct pci_dev *pdev)
> +{
> +	WRITE_ONCE(pdev->disconnect_work_enable, 0x0);
> +
> +	/* Make sure to stop using work from now on. */
> +	smp_wmb();
> +
> +	/* Leaves the work disabled, so a racing schedule_work() is a no-op. */
> +	disable_work_sync(&pdev->disconnect_work);
> +}

[Severity: High]
Does this function provide enough protection against the delayed
schedule_work() scenario mentioned above?

While disable_work_sync() prevents immediate races by setting disable bits on
the work item, a subsequent driver probe calling INIT_WORK() will
non-atomically reset work->data.

This reset clears all flags, including the WORK_OFFQ_DISABLE_BITS. If
INIT_WORK() executes INIT_LIST_HEAD() concurrently with the delayed
schedule_work() insertion, couldn't this overwrite the next and prev pointers
of the queued work item and corrupt the workqueue linked list?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927165459.829900-1-abhinjoses@gmail.com?part=1