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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rust: pci: clear_master() and disable_device() exposed on Sync Device<Normal>
- [Medium] rust: sync: bad timeout conversion in wait_for_completion_timeout()
--

commit 49001463072fd010a27e67166e414638fec06345
Author: Lucas Costa <2000.costalucas@gmail.com>

samples: rust: add QEMU EDU sample driver

This commit adds a Rust PCI sample driver for QEMU's educational ("edu")
device along with PCI teardown helpers and completion timeout support.

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 3ec897709e890..810703d7a2fb5 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -445,6 +445,20 @@ pub fn pci_class(&self) -> Class {
>          // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>          Class::from_raw(unsafe { (*self.as_raw()).class })
>      }
> +
> +    /// Disable bus-mastering for this device.
> +    #[inline]
> +    pub fn clear_master(&self) {
> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        unsafe { bindings::pci_clear_master(self.as_raw()) };
> +    }
> +
> +    /// Disable the device.
> +    #[inline]
> +    pub fn disable_device(&self) {
> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        unsafe { bindings::pci_disable_device(self.as_raw()) };
> +    }
>  }
>  
>  impl<'a> Device<device::Core<'a>> {

[Severity: High]
Should clear_master() and disable_device() be restricted to
Device<device::Core<'_>> alongside enable_device_mem() and set_master()
rather than exposed on impl Device (Device<device::Normal>)?

Device<device::Normal> and Device<device::Bound> implement Sync under the
safety invariant that all methods on Device<device::Normal> are thread-safe,
and Device<device::Normal> implements AlwaysRefCounted so ARef<pci::Device>
can be shared across threads and outlive driver unbind:

rust/kernel/pci.rs:
    // SAFETY: `Device` can be shared among threads because all methods of `Device`
    // (i.e. `Device<Normal>) are thread safe.
    unsafe impl Sync for Device {}

However, pci_clear_master() (via __pci_set_master()) and
pci_disable_device() perform unsynchronized read-modify-write sequences on
PCI_COMMAND in configuration space and non-atomic writes to
dev->is_busmaster, which is a 1-bit C bitfield sharing a storage word with
msi_enabled, msix_enabled, block_cfg_access, is_managed, and state_saved in
struct pci_dev.

Furthermore, in samples/rust/rust_driver_edu.rs, SampleDriverData::drop()
calls self.pdev.clear_master() and self.pdev.disable_device():

samples/rust/rust_driver_edu.rs:
    impl Drop for SampleDriverData<'_> {
        fn drop(&mut self) {
            dev_dbg!(self.pdev, "Remove Rust QEMU EDU driver sample.\n");

            self.pdev.clear_master();
            self.pdev.disable_device();
        }
    }

During unbind, Registration::post_unbind_callback() drops SampleDriverData
in device_unbind_cleanup() only after pci_device_remove() has already
dropped the runtime PM reference and cleared pci_dev->driver and after
pci_dma_cleanup() has detached the IOMMU default domain. In addition,
SampleDriverData::drop() runs before its bar field (Bar0) is dropped, so
pci_disable_device() executes before Bar::drop() calls pci_iounmap() and
pci_release_region().

Would moving clear_master() and disable_device() to
impl Device<device::Core<'_>> and performing teardown in
pci::Driver::unbind() (or managing device enablement via devres) avoid both
the Sync soundness issue and the out-of-phase teardown?

[ ... ]

> diff --git a/rust/kernel/sync/completion.rs b/rust/kernel/sync/completion.rs
> index 35ff049ff0783..83a22d25b61dd 100644
> --- a/rust/kernel/sync/completion.rs
> +++ b/rust/kernel/sync/completion.rs
> @@ -111,4 +116,17 @@ pub fn wait_for_completion(&self) {
>          // SAFETY: `self.as_raw()` is a pointer to a valid `struct completion`.
>          unsafe { bindings::wait_for_completion(self.as_raw()) };
>      }
> +
> +    /// Wait for completion of a task, with a timeout.
> +    ///
> +    /// Returns `true` if the task completed before `timeout` elapsed, and `false` otherwise.
> +    pub fn wait_for_completion_timeout(&self, timeout: Delta) -> bool {
> +        let timeout = timeout.as_millis().clamp(0, u32::MAX as i64) as u32;
> +        let jiffies = crate::time::msecs_to_jiffies(timeout);
> +
> +        // SAFETY: `self.as_raw()` is a pointer to a valid `struct completion`.
> +        let remaining = unsafe { bindings::wait_for_completion_timeout(self.as_raw(), jiffies) };
> +
> +        remaining != 0
> +    }
>  }

[Severity: Medium]
Can this timeout conversion in Completion::wait_for_completion_timeout()
mishandle both sub-millisecond and large Delta values?

First, timeout.as_millis() truncates towards zero rather than rounding up
with timeout.as_millis_ceil(), so any positive Delta under 1 ms (such as
Delta::from_micros(500)) is converted to 0 jiffies and times out
immediately without sleeping if the completion is not already done.

Second, clamping the upper bound to u32::MAX passes values in
(i32::MAX as u32 + 1)..=u32::MAX to __msecs_to_jiffies(), which treats any
value where (int)m < 0 as a negative timeout and returns MAX_JIFFY_OFFSET
(an infinite timeout on 64-bit systems):

kernel/time/time.c:__msecs_to_jiffies()
    if ((int)m < 0)
        return MAX_JIFFY_OFFSET;
    return _msecs_to_jiffies(m);

Should this use timeout.as_millis_ceil().clamp(0, i32::MAX as i64) as u32
(and also be marked #[inline] like the other methods on Completion)?

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