Re: [PATCH] samples: rust: add QEMU EDU sample driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] pci: unconditional IRQ_HANDLED in shared interrupt handler leads to interrupt stealing
- [High] pci: dma buffer use-after-free on timeout error path in dma_test
- [High] pci: unbounded wait for hardware interrupt in probe can cause kernel thread deadlock
- [High] pci: infinite wait in factorial_irq due to missing timeout
- [Medium] pci: resource leak of device enable count and master state
- [Low] rust: missing #[inline] annotation on abstraction function from_raw
--

commit 72599ec79fdfa3396a1908c456cf647dcc3a5105
Author: Lucas Costa <2000.costalucas@gmail.com>

samples: rust: add QEMU EDU sample driver

Add a sample PCI driver, written in Rust, for QEMU's educational
("edu") device. The driver exercises the Rust PCI, IRQ, and DMA
abstractions in a single self-contained driver.

> diff --git a/rust/kernel/pci/id.rs b/rust/kernel/pci/id.rs
> index dbaf301666e74..2747d92be91b6 100644
> --- a/rust/kernel/pci/id.rs
> +++ b/rust/kernel/pci/id.rs
> @@ -154,9 +154,13 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>  
>  /// Once constructed, a `Vendor` contains a valid PCI Vendor ID.
>  impl Vendor {
> -    /// Create a Vendor from a raw 16-bit vendor ID.
> +    /// Create a [`Vendor`] from a raw 16-bit vendor ID.
> +    ///
> +    /// Prefer one of the named [`Vendor`] constants where one exists. This is intended for
> +    /// vendor IDs that have no dedicated constant, e.g. test or emulated devices whose IDs
> +    /// are not shared between multiple drivers and thus do not belong in `pci_ids.h`.
>      #[inline]
> -    pub(super) fn from_raw(vendor_id: u16) -> Self {
> +    pub const fn from_raw(vendor_id: u16) -> Self {

[Severity: Low]
Is it intentional to remove the #[inline] annotation here?

The Rust subsystem guidelines state that small functions or functions
forwarding to a C binding call in abstractions should be annotated with
#[inline].

>          Self(vendor_id)
>      }

[ ... ]

> diff --git a/samples/rust/rust_driver_edu.rs b/samples/rust/rust_driver_edu.rs
> new file mode 100644
> index 0000000000000..e9e5593bb89a1
> --- /dev/null
> +++ b/samples/rust/rust_driver_edu.rs
[ ... ]
> +    fn factorial_irq(pdev: &pci::Device<Bound>, bar: &Bar0<'_>) -> Result {
> +        let operand: u32 = 6;
> +
> +        let msi = pci::IrqTypes::default().with(pci::IrqType::Msi);
> +        let vectors = pdev.alloc_irq_vectors(1, 1, msi)?;
> +
> +        let registration = KBox::pin_init(
> +            unsafe {
> +                Registration::new(
> +                    vectors.index(0)?.into(),
> +                    Flags::SHARED,
> +                    c"rust_driver_edu",
> +                    try_pin_init!(EduIrq {

[Severity: High]
Does this code handle shared interrupts correctly?

The driver requests a shared interrupt by passing Flags::SHARED for the MSI
interrupt here, but the handler implementation (EduIrq::handle) unconditionally
claims the interrupt.

If the kernel invokes the handler for an interrupt raised by another device
sharing the same IRQ line, could this handler erroneously claim it,
potentially preventing the legitimate driver from handling it?

> +                        done <- Completion::new(),
> +                    }? Error),
> +                )
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        // Arm the "raise an IRQ when done" bit, then start the computation.
> +        bar.write_reg(regs::STATUS::zeroed().with_irq_on_done(true));
> +        bar.write_reg(regs::FACTORIAL::zeroed().with_value(operand));
> +
> +        // Sleep until the handler calls `complete_all()` from IRQ context.
> +        registration.handler().done.wait_for_completion();

[Severity: High]
Could this unbounded wait cause a kernel thread deadlock during device probe?

The driver puts the current kernel thread into an uninterruptible sleep state
with no timeout mechanism.

If the hardware fails to raise the interrupt or the interrupt is lost, the
wait will never complete. Could this permanently hang the kernel worker
thread running the device initialization?

> +
> +        // Acknowledge whatever the device raised so it lowers the interrupt.
> +        let pending = bar.read(regs::INTERRUPT_STATUS).status().get();
> +        bar.write_reg(regs::INTERRUPT_ACK::zeroed().with_ack(pending));
> +
> +        let result = bar.read(regs::FACTORIAL).value().get();

[ ... ]

> +    fn dma_test(pdev: &pci::Device<Core<'_>>, bar: &Bar0<'_>) -> Result {
> +        const DEV_BUF: u32 = 0x40_000;
> +        const LEN: usize = 512;
> +        const CMD_START: u32 = 1 << 0;
> +        const CMD_TO_RAM: u32 = 1 << 1;
> +
> +        unsafe { pdev.dma_set_mask_and_coherent(DmaMask::new::<28>())? };
> +
> +        let mut src: CoherentBox<[u8]> = CoherentBox::zeroed_slice(pdev.as_ref(), LEN, GFP_KERNEL)?;
> +        src.fill(0xa5);
> +
> +        let src: Coherent<[u8]> = src.into();
> +
> +        let dst: Coherent<[u8]> =
> +            CoherentBox::<[u8]>::zeroed_slice(pdev.as_ref(), LEN, GFP_KERNEL)?.into();
> +
> +        let src_dma = src.dma_address() as u32;
> +        let dst_dma = dst.dma_address() as u32;
> +
> +        let wait_dma = || {
> +            read_poll_timeout(
> +                || Ok(bar.read(regs::DMA_CMD).cmd().get()),
> +                |cmd| *cmd & CMD_START == 0,
> +                Delta::from_millis(10),
> +                Delta::from_secs(1),
> +            )
> +        };
> +
> +        // 1) RAM -> device internal buffer. CMD is written last: it triggers the transfer.
> +        bar.write_reg(regs::DMA_SRC_ADDR::zeroed().with_src(src_dma));
> +        bar.write_reg(regs::DMA_DST_ADDR::zeroed().with_dst(DEV_BUF));
> +        bar.write_reg(regs::DMA_CNT::zeroed().with_size(LEN as u32));
> +        bar.write_reg(regs::DMA_CMD::zeroed().with_cmd(CMD_START));
> +        wait_dma()?;

[Severity: High]
Can this code cause a use-after-free if the DMA transfer times out?

If wait_dma() times out, it immediately propagates the error, which causes
the local variables src and dst to go out of scope and free the underlying
physical memory pages.

Because the hardware DMA engine was not explicitly halted before returning,
could the device continue to asynchronously read from or write to the newly
freed memory pages?

> +
> +        // 2) device internal buffer -> RAM, landing in `dst`.
> +        bar.write_reg(regs::DMA_SRC_ADDR::zeroed().with_src(DEV_BUF));
> +        bar.write_reg(regs::DMA_DST_ADDR::zeroed().with_dst(dst_dma));
> +        bar.write_reg(regs::DMA_CNT::zeroed().with_size(LEN as u32));
> +        bar.write_reg(regs::DMA_CMD::zeroed().with_cmd(CMD_START | CMD_TO_RAM));
> +        wait_dma()?;

[Severity: High]
Similarly, can this timeout error path also result in a use-after-free?

If wait_dma() times out here, the function returns and frees dst while the
device may still be actively writing to the dst_dma address.

> +
> +        // Transfer completed, check dst is the expected value
> +        let out = unsafe { dst.as_ref() };

[ ... ]

> +    fn probe<'bound>(
> +        pdev: &'bound pci::Device<Core<'_>>,
> +        _info: Option<&'bound Self::IdInfo>,
> +    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> +        dev_dbg!(
> +            pdev,
> +            "Probe Rust QEMU EDU driver sample (PCI ID: {}, 0x{:x}).\n",
> +            pdev.vendor_id(),
> +            pdev.device_id()
> +        );
> +
> +        pdev.enable_device_mem()?;
> +        pdev.set_master();

[Severity: Medium]
Does this code leak the PCI device enable count and bus master state?

The driver calls pdev.enable_device_mem() and pdev.set_master() during
probe(), but neither the error paths in probe() nor the driver remove path
(SampleDriverData::drop) call pci_disable_device() or pci_clear_master().

Could this permanently leak these resources on driver unload or if an error
occurs later in the probe function?

> +
> +        let bar = pdev.iomap_region_sized::<{ regs::END }>(0, c"rust_driver_edu")?;

[ ... ]

> +impl Drop for SampleDriverData<'_> {
> +    fn drop(&mut self) {
> +        dev_dbg!(self.pdev, "Remove Rust QEMU EDU driver sample.\n");
> +    }
> +}
> +
> +/// Per-interrupt state shared between process context and IRQ context.
> +///
> +/// The factorial computation signals `done` from the IRQ handler; the process
> +/// context that started it waits on the same [`Completion`].
> +#[pin_data]
> +struct EduIrq {
> +    #[pin]
> +    done: Completion,
> +}
> +
> +impl irq::Handler for EduIrq {
> +    // Runs in IRQ context when the EDU device raises its interrupt.
> +    fn handle(&self) -> irq::IrqReturn {
> +        self.done.complete_all();
> +        irq::IrqReturn::Handled
> +    }
> +}

[Severity: High]
Could claiming a spurious or shared interrupt prematurely complete the done
variable?

If the handler claims an interrupt not belonging to this device, it would
unblock wait_for_completion() before the data is actually ready, which could
cause the driver probe to read incomplete data from the FACTORIAL register
and fail.

> +
> +// Helper functions

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