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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 08 Nov 2022 20:30:52 +0100
From:   Andreas Hindborg <andreas.hindborg@....com>
To:     Dennis Dai <dzy.0424thu@...il.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`

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