Re: [PATCH RFC 3/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] Unconditional compilation of test driver causes kernel bloat and hangs on normal unbind
- [High] Missing surprise removal check (0xFFFFFFFF) in shared IRQ handler causes interrupt storms
- [Medium] Missing pci_clear_master() in cleanup and error paths violates PCI resource management rules
--

commit bf44d9ecb886ad9012988c5c1b317cd2fd7cfe83
Author: Abhin Parekadan Jose <abhinjoses@gmail.com>

misc: Add edu_srpoc surprise removal POC driver
    
This commit introduces a test driver for the QEMU edu device that reproduces
a surprise removal hang. It writes to a hacked register in remove() to trigger
a delayed IRQ and blocks waiting for it to simulate del_gendisk() behavior.

> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e8d8d5d88c0df..1479bf19c6467 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
[ ... ]
> @@ -9,6 +9,7 @@ obj-$(CONFIG_AD525X_DPOT_I2C)	+= ad525x_dpot-i2c.o
>  obj-$(CONFIG_AD525X_DPOT_SPI)	+= ad525x_dpot-spi.o
>  obj-$(CONFIG_ATMEL_SSC)		+= atmel-ssc.o
>  obj-$(CONFIG_DUMMY_IRQ)		+= dummy-irq.o
> +obj-y				+= edu_srpoc.o

[Severity: High]
Is it intentional to unconditionally compile this test driver into the kernel
image? Using obj-y forces the module to be built-in for any configuration with
misc devices enabled, which causes unnecessary kernel bloat.

>  obj-$(CONFIG_ICS932S401)	+= ics932s401.o
>  obj-$(CONFIG_LKDTM)		+= lkdtm/
>  obj-$(CONFIG_TI_FPC202)		+= ti_fpc202.o
> diff --git a/drivers/misc/edu_srpoc.c b/drivers/misc/edu_srpoc.c
> new file mode 100644
> index 0000000000000..f536bc4aa253d
> --- /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;

[Severity: High]
What happens to this check during a surprise removal where MMIO reads typically
return all 1s (0xFFFFFFFF)?

Since 0xFFFFFFFF evaluates as non-zero, it appears this will bypass the
return IRQ_NONE. The handler would then incorrectly complete edu->irq_done
and return IRQ_HANDLED. Because the IRQ is registered as IRQF_SHARED, falsely
claiming the interrupt without clearing it on the actual hardware could defeat
the kernel's spurious interrupt detector, leading to an infinite interrupt
storm.

> +
> +	iowrite32(status, edu->regs + EDU_REG_IRQ_LOWER);
> +
> +	if (status & (EDU_FACT_IRQ | EDU_DELAY_IRQ)) {
> +		complete(&edu->irq_done);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static int edu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
[ ... ]
> +	pci_set_master(pdev);
> +
> +	err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI | PCI_IRQ_INTX);
> +	if (err < 0)
> +		goto err_iounmap;
> +
> +	err = request_irq(pci_irq_vector(pdev, 0), edu_irq_handler,
> +			  IRQF_SHARED, "edu_srpoc", edu);
> +	if (err)
> +		goto err_free_vectors;
> +
> +	pci_set_drvdata(pdev, edu);
> +
> +	INIT_WORK(&pdev->disconnect_work, edu_disconnect);
> +	pci_set_disconnect_work(pdev);
> +
> +	dev_info(&pdev->dev, "edu_srpoc probed\n");
> +	return 0;
> +
> +err_free_vectors:
> +	pci_free_irq_vectors(pdev);
> +err_iounmap:
> +	pci_iounmap(pdev, edu->regs);

[Severity: Medium]
Should pci_clear_master() be called in the error paths here to match the
earlier pci_set_master() call? Failing to clear the bus master bit before
releasing resources and disabling the device could theoretically allow rogue
DMA operations if the hardware acts unexpectedly.

> +err_release:
> +	pci_release_regions(pdev);
> +err_disable:
> +	pci_disable_device(pdev);
> +	return err;
> +}
> +
> +static void edu_remove(struct pci_dev *pdev)
> +{
> +	struct edu_dev *edu = pci_get_drvdata(pdev);
> +
> +	iowrite32(EDU_STATUS_IRQFACT, edu->regs + EDU_REG_STATUS);
> +	iowrite32(600000, edu->regs + EDU_REG_DELAYED_IRQ);
> +
> +	dev_info(&pdev->dev, "Waiting for IRQ in remove()\n");
> +	wait_for_completion(&edu->irq_done);

[Severity: High]
If this driver is bound to a standard QEMU edu device (which lacks the custom
EDU_REG_DELAYED_IRQ functionality), will this wait forever? A normal device
unbind via sysfs or system shutdown seems like it would hang indefinitely here
since the delayed hardware interrupt will never fire and disconnect_work is
only triggered on surprise removal.

> +	dev_info(&pdev->dev, "Unblocked, cleaning up\n");
> +
> +	pci_clear_disconnect_work(pdev);
> +	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);

[Severity: Medium]
Does this cleanup path also need a call to pci_clear_master() before
pci_disable_device() to ensure bus mastering is properly disabled upon
removal?

> +}

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