Re: [PATCH v3 10/10] samples: rust: add Rust SR-IOV VF driver sample

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: missing pci_disable_device() leaks enable_cnt and bus master state
--

commit 775047d7dfd13478598c3e7650d421010c6a0cef
Author: Peter Colberg <peter@colberg.org>

samples: rust: add Rust SR-IOV VF driver sample

Add a new SR-IOV driver sample that demonstrates how to enable and
disable the Single Root I/O Virtualization capability for a PCI device.

Link: https://www.qemu.org/docs/master/system/devices/igb.html

> diff --git a/samples/rust/rust_driver_sriov.rs b/samples/rust/rust_driver_sriov.rs
> new file mode 100644
> --- /dev/null
> +++ b/samples/rust/rust_driver_sriov.rs

[ ... ]

> +    fn probe<'bound>(
> +        pdev: &'bound pci::Device<Core<'_>>,
> +        _info: Option<&'bound Self::IdInfo>,
> +    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> +        pin_init::pin_init_scope(move || {
> +            dev_info!(
> +                pdev,
> +                "Probe Rust SR-IOV PF sample (PCI ID: {}, 0x{:x}).\n",
> +                pdev.vendor_id(),
> +                pdev.device_id()
> +            );
> +
> +            pdev.enable_device_mem()?;
> +            pdev.set_master();
> +
> +            Ok(try_pin_init!(PfDriverData {

[Severity: Medium]
Does this code leak the enable_cnt and leave bus mastering active if the
subsequent try_pin_init!() block fails?

pdev.enable_device_mem() calls pci_enable_device_mem(), incrementing the
enable_cnt of the PCI device, and pdev.set_master() enables bus mastering.
Returning an error without calling pci_disable_device() or
pci_clear_master() would leave the device in an unbalanced state.

> +                // SAFETY:
> +                // - probe has exclusive access to this PF before SR-IOV is enabled;
> +                // - the registration is pinned in the PF driver data and dropped before `pdev`;
> +                // - no other registration is created for this PF; and
> +                // - VFs are enabled only after probe by `sriov_enable`.
> +                _registration <- unsafe {
> +                    pci::VfRegistration::new(

[ ... ]

> +    fn probe<'bound>(
> +        pdev: &'bound pci::Device<Core<'_>>,
> +        _info: Option<&'bound Self::IdInfo>,
> +    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> +        pin_init::pin_init_scope(move || {
> +            dev_info!(
> +                pdev,
> +                "Probe Rust SR-IOV VF sample (PCI ID: {}, 0x{:x}).\n",
> +                pdev.vendor_id(),
> +                pdev.device_id()
> +            );
> +
> +            let pdev_bound: &'bound pci::Device<Bound> = pdev;
> +            let pf_api = pdev_bound.vf_registration_data::<PfApiForLt>()?;
> +
> +            pdev.enable_device_mem()?;
> +            pdev.set_master();
> +
> +            let request = pf_api.submit(pdev)?;

[Severity: Medium]
Similarly, does this code leak the enable_cnt and leave bus mastering
active if pf_api.submit(pdev)? fails and propagates an error?

> +            dev_info!(pdev, "Submitted request {} through PF data.\n", request);
> +
> +            Ok(try_pin_init!(VfDriverData { pdev: pdev.into() }))
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for PfDriverData<'_> {
> +    fn drop(self: Pin<&mut Self>) {
> +        dev_info!(self.pdev, "Remove Rust SR-IOV PF sample.\n");
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for VfDriverData {
> +    fn drop(self: Pin<&mut Self>) {
> +        dev_info!(self.pdev, "Remove Rust SR-IOV VF sample.\n");
> +    }
> +}

[Severity: Medium]
During driver teardown, does the PinnedDrop implementation successfully
execute but fail to disable the device memory and clear the master bit?

Leaving these resources enabled leads to an unbalanced enable_cnt which
prevents correct hot-unplug behavior and resource releasing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790705494.git.zhiw@nvidia.com?part=10