Re: [PATCH RFC 1/3] misc: add edu_srpoc surprise removal POC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The shared IRQ handler does not correctly handle the `0xffffffff` register read value on surprise removal, leading to spurious `IRQ_HANDLED` returns and false completions.
- [Medium] Missing explicit `cancel_work_sync()` for `disconnect_work` in `remove()` leads to a Use-After-Free of the `edu` structure due to a TOCTOU race.
- [Low] The driver references a non-existent `disconnect_work` member in `struct pci_dev` and calls non-existent PCI functions, causing immediate build failures.
--

--- Patch [1]: [PATCH RFC 1/3] misc: add edu_srpoc surprise removal POC driver ---
commit 1e01da4d29945cd7e9e2a5dc7d46075c3af6e84b
Author: Abhin Parekadan Jose <abhin@poc.local>

misc: add edu_srpoc surprise removal POC driver

This patch adds a test driver for the QEMU edu device to reproduce a surprise
removal hang. It registers a PCI driver, requests regions, and sets up
interrupts and a work queue.

> diff --git a/drivers/misc/edu_srpoc.c b/drivers/misc/edu_srpoc.c
> new file mode 100644
> index 0000000000000..ce2c254e3362f
> --- /dev/null
> +++ b/drivers/misc/edu_srpoc.c
[ ... ]
> +static irqreturn_t edu_irq_handler(int irq, void *data)
> +{
> +	struct edu_dev *edu = data;
> +	u32 status;
> +
> +	status = ioread32(edu->regs + EDU_REG_IRQ_STATUS);
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	iowrite32(status, edu->regs + EDU_REG_IRQ_LOWER);
> +
> +	if (status & (EDU_FACT_IRQ | EDU_DELAY_IRQ))
> +	{

[Severity: High]
When the PCI device is surprise-removed, MMIO reads (like ioread32())
typically return all-ones (0xffffffff). 

Since this is a shared interrupt handler, 0xffffffff will pass the !status
check and the status & (EDU_FACT_IRQ | EDU_DELAY_IRQ) check. 

Will this cause the handler to spuriously complete the wait and return
IRQ_HANDLED, stealing interrupts from other devices sharing the line?

> +		pr_info("complete(&edu->irq_done)\n");
> +		complete(&edu->irq_done);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void edu_disconnect(struct work_struct *work)
> +{
> +	struct pci_dev *pdev = container_of(work, struct pci_dev,
> +					    disconnect_work);

[Severity: Low]
Is disconnect_work a valid member of struct pci_dev? 

It appears this member does not exist in the PCI subsystem, which would cause
the module to fail to compile.

> +	struct edu_dev *edu = pci_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev, "edu_disconnect()\n");
> +	if (!pci_test_and_clear_disconnect_enable(pdev))
> +		return;
> +
> +	if (!edu)
> +		return;
> +
> +	dev_info(&pdev->dev, "disconnect_work fired — unblocking remove()\n");
> +	complete(&edu->irq_done);

[Severity: Medium]
If the work function executes concurrently with edu_remove(), could it read
the edu pointer, pass the if (!edu) check, and get preempted before calling
complete()? 

Since edu_remove() does not synchronously cancel the work (e.g. using 
cancel_work_sync()), and edu is allocated with devm_kzalloc(), edu could be 
freed by devres when remove() returns.

Could this lead to a use-after-free on edu->irq_done here?

> +}
> +
> +static int edu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
[ ... ]
> +	INIT_WORK(&pdev->disconnect_work, edu_disconnect);
> +	pci_set_disconnect_work(pdev);

[Severity: Low]
Does pci_set_disconnect_work() exist in the kernel? 

Calling this undefined function will cause build errors.

> +
> +	dev_info(&pdev->dev, "edu_srpoc probed\n");
> +	return 0;
[ ... ]
> +static void edu_remove(struct pci_dev *pdev)
> +{
> +	struct edu_dev *edu = pci_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev, "remove(): starting factorial — blocking for IRQ\n");
> +
> +	iowrite32(EDU_STATUS_IRQFACT, edu->regs + EDU_REG_STATUS);
> +
> +	iowrite32(600000, edu->regs + EDU_REG_DELAYED_IRQ);
> +
> +	wait_for_completion(&edu->irq_done);
> +
> +	dev_info(&pdev->dev, "remove(): unblocked, cleaning up\n");
> +
> +	pci_clear_disconnect_work(pdev);

[Severity: Low]
Does pci_clear_disconnect_work() exist in the kernel? 

Calling this undefined function will cause build errors.

> +	free_irq(pci_irq_vector(pdev, 0), edu);
> +	pci_free_irq_vectors(pdev);
> +	pci_iounmap(pdev, edu->regs);
> +	pci_release_regions(pdev);
> +	pci_disable_device(pdev);
> +}

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