nvme driver: possible missing `unregister_irq`

nvme driver: possible missing `unregister_irq`

[Dennis Dai]

Hi all,

I was inspecting the rust nvme driver [1] and would like know if the following
code contains a missing unregister or I missed anything

// nvme.rs:180, in NvmeDevice::setup_io_queues
        admin_queue.register_irq(pci_dev)?;
// nvme.rs:186, in NvmeDevice::setup_io_queues
        let q_depth = core::cmp::min(...).try_into()?;
// nvme.rs:190, in NvmeDevice::setup_io_queues
        let tagset = mq::TagSet::try_new(...)?; //TODO: 1 or 3 on
demand, depending on polling enabled

Line 186 and 190 could abort the execution of
NvmeDevice::setup_io_queues without calling `unregister_irq`.
In the end this could result in an `request_threaded_irq` without a
pairing `free_irq` on failure.
Or is the job done by Rust by auto dropping?

Thank you very much!

[1] https://github.com/metaspace/rust-linux/commit/d88c3744d6cbdf11767e08bad56cbfb67c4c96d0

Re: nvme driver: possible missing `unregister_irq`

[Miguel Ojeda]

On Tue, Nov 8, 2022 at 6:18 PM Dennis Dai <dzy.0424thu@gmail.com> wrote:
>
> I was inspecting the rust nvme driver [1] and would like know if the following
> code contains a missing unregister or I missed anything

(Cc'ing Andreas and fixing Wedson's email -- please do so in the
future, thanks!)

Cheers,
Miguel

Re: nvme driver: possible missing `unregister_irq`

[Andreas Hindborg]

Dennis Dai <dzy.0424thu@gmail.com> writes:

> I was inspecting the rust nvme driver [1] and would like know if the following
> code contains a missing unregister or I missed anything
>
> // nvme.rs:180, in NvmeDevice::setup_io_queues
>         admin_queue.register_irq(pci_dev)?;
> // nvme.rs:186, in NvmeDevice::setup_io_queues
>         let q_depth = core::cmp::min(...).try_into()?;
> // nvme.rs:190, in NvmeDevice::setup_io_queues
>         let tagset = mq::TagSet::try_new(...)?; //TODO: 1 or 3 on
> demand, depending on polling enabled
>
> Line 186 and 190 could abort the execution of
> NvmeDevice::setup_io_queues without calling `unregister_irq`.
> In the end this could result in an `request_threaded_irq` without a
> pairing `free_irq` on failure.
> Or is the job done by Rust by auto dropping?

In line with my reply to the other potential sleep bug you reported,
teardown is not properly implemented yet, and I did not review the
teardown code that is already in place.

But, if you look at the `register_irq()` and `unregister_irq()`
functions of `NvmeQueue` you can see that the registrations are stored
in an `Option` within the `NvmeQueue` structure. So when the `NvmeQueue`
struct is dropped, the registration will be dropped. Also, if we call
`register_irq()` twice and forget to unregister the first one, it will
be unregistered when we register the second one (because we call
Option::replace()).

So as long as the `NvmeQueue` structs are dropped, we will not leak
IRQs. In case of one of the lines you point to return an `Err`, the ref
count of the `kernel::device::Data` allocated in `probe()` would go to
zero and it would be dropped and thus the IRQs would be unregistered.

So yes, it is handled by destructors that run on drop.

Best regards,
Andreas

Re: nvme driver: possible missing `unregister_irq`

[Dennis Dai]

Sorry I missed that point.
Now I do got it!

Thank you very much for the detailed explanation!

Best regards,
Dennis Dai

On Wed, Nov 9, 2022 at 3:58 AM Andreas Hindborg
<andreas.hindborg@wdc.com> wrote:
>
>
> Dennis Dai <dzy.0424thu@gmail.com> writes:
>
> > I was inspecting the rust nvme driver [1] and would like know if the following
> > code contains a missing unregister or I missed anything
> >
> > // nvme.rs:180, in NvmeDevice::setup_io_queues
> >         admin_queue.register_irq(pci_dev)?;
> > // nvme.rs:186, in NvmeDevice::setup_io_queues
> >         let q_depth = core::cmp::min(...).try_into()?;
> > // nvme.rs:190, in NvmeDevice::setup_io_queues
> >         let tagset = mq::TagSet::try_new(...)?; //TODO: 1 or 3 on
> > demand, depending on polling enabled
> >
> > Line 186 and 190 could abort the execution of
> > NvmeDevice::setup_io_queues without calling `unregister_irq`.
> > In the end this could result in an `request_threaded_irq` without a
> > pairing `free_irq` on failure.
> > Or is the job done by Rust by auto dropping?
>
> In line with my reply to the other potential sleep bug you reported,
> teardown is not properly implemented yet, and I did not review the
> teardown code that is already in place.
>
> But, if you look at the `register_irq()` and `unregister_irq()`
> functions of `NvmeQueue` you can see that the registrations are stored
> in an `Option` within the `NvmeQueue` structure. So when the `NvmeQueue`
> struct is dropped, the registration will be dropped. Also, if we call
> `register_irq()` twice and forget to unregister the first one, it will
> be unregistered when we register the second one (because we call
> Option::replace()).
>
> So as long as the `NvmeQueue` structs are dropped, we will not leak
> IRQs. In case of one of the lines you point to return an `Err`, the ref
> count of the `kernel::device::Data` allocated in `probe()` would go to
> zero and it would be dropped and thus the IRQs would be unregistered.
>
> So yes, it is handled by destructors that run on drop.
>
> Best regards,
> Andreas