linux-kernel - Re: [PATCH v3 5/7] rust: percpu: Support non-zeroable types for DynamicPerCpu

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: <68b9f3d6.050a0220.174510.28d9@mx.google.com>
Date: Thu, 4 Sep 2025 13:17:23 -0700
From: Mitchell Levy <levymitchell0@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: 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>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dennis Zhou <dennis@...nel.org>, Tejun Heo <tj@...nel.org>,
	Christoph Lameter <cl@...ux.com>,
	Danilo Krummrich <dakr@...nel.org>,
	Benno Lossin <lossin@...nel.org>, Yury Norov <yury.norov@...il.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Tyler Hicks <code@...icks.com>, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v3 5/7] rust: percpu: Support non-zeroable types for
 DynamicPerCpu

On Thu, Sep 04, 2025 at 01:05:36AM +0200, Miguel Ojeda wrote:
> On Thu, Aug 28, 2025 at 9:01 PM Mitchell Levy <levymitchell0@...il.com> wrote:
> >
> > +    /// Get a `*mut MaybeUninit<T>` to the per-CPU variable on the CPU represented by `cpu`. Note
> > +    /// that without some kind of synchronization, use of the returned pointer may cause a data
> > +    /// race. It is the caller's responsibility to use the returned pointer in a reasonable way.
> 
> Please try to make the first paragraph ("short description" / title) smaller.

Will do.

> Does "reasonable" mean anything different than any other raw pointer?

This will depend very heavily on `T`, there's a detailed discussion
here: https://docs.kernel.org/core-api/this_cpu_ops.html#remote-access-to-per-cpu-data

In general, remote accesses are strongly discouraged, and my intention
was mostly just wanting to reflect that in this documentation.

> > +    /// # Safety
> 
> Newline after section headers (also elsewhere).

Will do.

> > +    /// - The returned pointer is valid only if `self` is (that is, it points to a live allocation
> > +    ///   correctly sized and aligned to hold a `T`)
> > +    /// - The returned pointer is valid only if the bit corresponding to `cpu` is set in
> > +    ///   `Cpumask::possible()`.
> 
> It sounds like the returned pointer can be invalid without triggering
> UB -- could you please clarify why the method is `unsafe`?

Yes, this is true; strictly speaking, calling this function without
dereferencing the returned pointer is always safe. I suppose I find it
clearer that, when a function has preconditions that are necessary for
it to return a valid pointer, the "safe-ness" has more to do with the
functions preconditions than the act of dereferencing the pointer.

In this case, the pointer shouldn't be going very far, but I think this
logic applies especially well in cases where pointers might be getting
stored away for later (and so the validity of the dereference later on
might rely on non-local conditions).

All that said, I'm alright with having this be a safe function, with the
documentation noting these requirements for the returned pointer to be
valid.

> In addition, please use intra-doc links wherever possible (e.g. there
> a was also an `Arc` elsewhere).

Will do.

> > +        // SAFETY: The requirements of this function ensure this call is safe.
> > +        unsafe { bindings::per_cpu_ptr(self.0.cast(), cpu.as_u32()) }.cast()
> 
> Please try to explain why, not just that it is. It isn't clear how the
> safety preconditions, which only seem to talk about the returned
> pointer, make this OK.

This flows from the first requirement (that `self` is a live allocation,
which is necessary for `per_cpu_ptr` to return a valid pointer). Though,
as above, simply possessing this pointer isn't UB, so it's arguable that
any call to `per_cpu_ptr` (on x86 at least, I'm not sure how it's
implemented on other arches) is always safe. Regardless, I agree this
comment should be more clear (even if the function is safe, it's
probably worth noting why the pointer returned is valid when the
function preconditions are met); will fix.

Thanks,
Mitchell

> Thanks!
> 
> Cheers,
> Miguel