linux-kernel - Re: nvme driver: possible missing `unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 9 Nov 2022 16:34:26 +0800
From:   Dennis Dai <dzy.0424thu@...il.com>
To:     Andreas Hindborg <andreas.hindborg@....com>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
        "baijiaju1990@...il.com" <baijiaju1990@...il.com>
Subject: Re: nvme driver: possible missing `unregister_irq`

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@....com> wrote:
>
>
> Dennis Dai <dzy.0424thu@...il.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