Message-Id: <DB9HMM5M24FE.1VR5ZE5YJ66CK@kernel.org>
Date: Fri, 11 Jul 2025 21:54:23 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Joel Fernandes" <joelagnelf@...dia.com>
Cc: <abdiel.janulgue@...il.com>, <daniel.almeida@...labora.com>,
 <robin.murphy@....com>, <a.hindborg@...nel.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <aliceryhl@...gle.com>,
 <tmgross@...ch.edu>, <bhelgaas@...gle.com>, <kwilczynski@...nel.org>,
 <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
 <rust-for-linux@...r.kernel.org>, <linux-pci@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

On Fri Jul 11, 2025 at 9:35 PM CEST, Joel Fernandes wrote:
> It looks good to me. A few high-level comments:
>
> 1. If we don't expect the concurrency issue for this in C code, why do we
> expect it to happen in rust? 

The race can happen in C as well, but people would probably argue that no one
ever calls the mask setter function concurrently to DMA allocation and mapping
primitives.

> 2. Since the Rust code is wrapping around the C code, the data race is
> happening entirely on the C side right? So can we just rely on KCSAN to catch
> concurrency issues instead of marking the callers as unsafe? I feel the
> unsafe { } really might make the driver code ugly.

If the function is declared safe in Rust it means that we have to guarantee that
any possible use won't lead to undefined behavior. This isn't the case here
unfortunately.

> 3. Maybe we could document this issue than enforce it via unsafe? My concern
> is wrapping unsafe { } makes the calling code ugly.

Not possible, that's exactly what unsafe is for. We can't violate the "safe
functions can't cause UB" guarantee.

> 4. Are there other kernel APIs where Rust wraps potentially racy C code as
> unsafe?

Sure, but we usually don't expose those to drivers. We always try to make things
safe for drivers.

This case is a bit special though. There is a way to avoid unsafe for the DMA
mask setter methods, but it would involve at least three new type states for
device structures and duplicated API interfaces for types that expect certain
type states.

The conclusion was to go with unsafe for now, since introducing this kind of
complexity is not worth for something that is (not entirely, but more of) a
formal problem.

In the long term I'd like to get those setters safe, e.g. by making the DMA mask
fields atomics.

> 5. In theory, all rust bindings wrappers are unsafe and we do mark it around
> the bindings call, right? But in this case, we're also making the calling
> code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust
> PoV but I am not sure we worry about the internal implementation-unsafety of
> the C code because then maybe most bindings wrappers would need to be unsafe,
> not only these DMA ones.

No, the key is to design Rust APIs in a way that the abstraction can't call C
functions in a way that potentially causes undefined behavor.

As an example, let's have a look at a PCI device function:

	/// Enable bus-mastering for this device.
	pub fn set_master(&self) {
	    // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
	    unsafe { bindings::pci_set_master(self.as_raw()) };
	}

pci_set_master() is unsafe because it takes a pointer value, so the caller can
easily cause UB by passing an invalid pointer.

pci::Device::set_master() is safe, because once you have a pci::Device instance, there is no way
(other than due to a bug) that you obtain an invalid pointer from it to
subsequently call pci_set_master().

This means for the caller there is no way to call pci::Device::set_master() such
that it can cause UB.

Powered by blists - more mailing lists