linux-kernel - Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <DB6YTN5P23X3.2S0NH4YECP1CP@kernel.org>
Date: Tue, 08 Jul 2025 22:44:53 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alistair Popple" <apopple@...dia.com>
Cc: <rust-for-linux@...r.kernel.org>, "Bjorn Helgaas" <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>, "Miguel
 Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun
 Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>, "John Hubbard" <jhubbard@...dia.com>, "Alexandre
 Courbot" <acourbot@...dia.com>, <linux-pci@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] rust: Add dma_set_mask() and
 dma_set_coherent_mask() bindings

On Tue Jul 8, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
> On Tue Jul 8, 2025 at 10:40 AM CEST, Danilo Krummrich wrote:
>> On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
>>> Add bindings to allow setting the DMA masks for both a generic device
>>> and a PCI device.
>>
>> Nice coincidence, I was about to get back to this. I already implemented this in
>> a previous patch [1], but didn't apply it yet.
>>
>> I think the approach below is thought a bit too simple:
>>
>>   (1) We want the DMA mask methods to be implemented by a trait in dma.rs.
>>       Subsequently, the trait should only be implemented by bus devices where
>>       the bus actually supports DMA. Allowing to set the DMA mask on any device
>>       doesn't make sense.
>
> Forgot to mention, another reason for a trait is that we can also use it as a
> trait bound on dma::CoherentAllocation::new(), such that people can't pass
> arbitrary devices to dma::CoherentAllocation::new(), but only those that
> actually sit on a DMA capable bus.
>
>>
>>   (2) We need to consider that with this we do no prevent
>>       dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not
>>       even if we'd add a new `Probe` device context).
>>
>> (2) is the main reason why I didn't follow up yet. So far I haven't found a nice
>>     solution for a sound API that doesn't need unsafe.
>>
>> One thing I did consider was to have some kind of per device table (similar to
>> the device ID table) for drivers to specify the DMA mask already at compile
>> time. However, I'm pretty sure there are cases where the DMA mask has to derived
>> dynamically from probe().
>>
>> I think I have to think a bit more about it.

Ok, there are multiple things to consider in the context of (2) above.

  (a) We have to ensure that the dev->dma_mask pointer is properly initialized,
      which happens when the corresponding bus device is initialized. This is
      definitely the case when probe() is called, i.e. when the device is bound.

      So the solutions here is simple, we just implement the dma::Device trait
      (which implements dma_set_mask() and dma_set_coherent_mask()) for
      &Device<Bound>.

  (b) When dma_set_mask() or dma_set_coherent_mask() are called concurrently
      with e.g. dma_alloc_coherent(), there is a data race with dev->dma_mask,
      dev->coherent_dma_mask and dev->dma_skip_sync (also set by
      dma_set_mask()).

      However, AFAICT, this does not necessarily make the Rust API unsafe in the
      sense of Rust's requirements. I.e. a potential data race does not lead to
      undefined behavior on the CPU side of things, but may result into a not
      properly functioning device.

      It would be possible to declare dma_set_mask() and dma_set_coherent_mask()
      Rust accessors as safe with the caveat that the device may not be able to
      use the memory concurrently allocated with e.g.
      dma::CoherentAllocation::new() properly.

      The alternative would be to make dma_set_mask() and
      dma_set_coherent_mask() unsafe to begin with.

      I don't think there's a reasonable alternative given that the mask may be
      derived on runtime in probe() by probing the device itself.

      I guess we could do something with type states and cookie values etc., but
      that's unreasonable overhead for something that is clearly more a
      theoretical than a practical concern.

      My conclusion is that we should just declare dma_set_mask() and
      dma_set_coherent_mask() as safe functions (with proper documentation on
      the pitfalls), given that the device is equally malfunctioning if they're
      not called at all.

@Alistair: If that is fine for you I'll pick up my old patches ([1] and related
ones) and re-send them.

If there is more discussion on (b) I'm happy to follow up either here or in the
mentioned patches once I re-visited and re-sent them.

>> [1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@gmail.com/