Re: [PATCH v3 1/7] rust: percpu: introduce a rust API for per-CPU variables

[Yury Norov <yury.norov@gmail.com>]

On Thu, Sep 04, 2025 at 12:53:59PM -0700, Mitchell Levy wrote:
> On Wed, Sep 03, 2025 at 05:42:08PM -0400, Yury Norov wrote:
> > On Thu, Aug 28, 2025 at 12:00:08PM -0700, Mitchell Levy wrote:

...

> > > +    /// Get a `&mut MaybeUninit<T>` to the per-CPU variable on the current CPU represented by `&self`
> > > +    ///
> > > +    /// # Safety
> > > +    /// The returned `&mut T` must follow Rust's aliasing rules. That is, no other `&(mut) T` may
> > > +    /// exist that points to the same location in memory. In practice, this means that `get_(mut_)ref`
> > 
> > How long is this line?
> 
> 102 chars, or 103 if you include the newline. `rustfmt` doesn't break
> the line, so I left it as-is for this patch. Happy to change it if it
> poses a problem, though.

Then don't use that tool - it's broken. In kernel we used to have 80-chars
limit for the lines, recently relaxed to 100.
 
> > > +    /// must not be called on another `PerCpuPtr<T>` that is a copy/clone of `&self` for as long as
> > > +    /// the returned reference lives.
> > > +    ///
> > > +    /// CPU preemption must be disabled before calling this function and for the lifetime of the
> > > +    /// returned reference. Otherwise, the returned reference might end up being a reference to a
> > > +    /// different CPU's per-CPU area, causing the potential for a data race.
> > > +    #[allow(clippy::mut_from_ref)] // Safety requirements prevent aliasing issues
> > > +    pub unsafe fn get_mut_ref(&self) -> &mut MaybeUninit<T> {
> > > +        // SAFETY: `self.get_ptr()` returns a valid pointer to a `MaybeUninit<T>` by its contract,
> > > +        // and the safety requirements of this function ensure that the returned reference is
> > > +        // exclusive.
> > > +        unsafe { &mut *(self.get_ptr()) }
> > > +    }
> > 
> > Here and everywhere: would it make sense to enforce it by testing
> > the CPU with preemptible() before returning a reference? 
> 
> The only thing we could do would be to panic, which I don't 100% love.
> Another alternative would be to take a &'a CpuGuard and bound the
> lifetime of the returned reference to 'a, and then we don't need to do
> any run-time checking at all.
> 
> Originally, I had left this to the caller because it might make sense
> down the line for some complex behavior based on per-CPU (e.g., per-CPU
> refcount) to do all its own management of per-CPU variables using
> `PerCpuPtr` as a core primitive. In these cases, I believe there are
> some times where being non-preemptible wouldn't actually be required
> (that said, my thoughts on this aren't well reflected in the safety
> comment, since I said it must be disabled... gah). But, the more I think
> about it, the more I think these use cases would be better served by
> just using `get_ptr` --- conjuring `&mut` references seems like it would
> be a big footgun. And the safety comment already actually reflects these
> thoughts somewhat :)

If you think that in future there will be a user who will not need to
disable preemption before dereferencing a percpu pointer, then you can
add another less restricted flavor of the helper.

> For v4 I will probably have this function take a &'a CpuGuard and use
> that to bound the liftetime of the returned reference, unless there are
> other thoughts on this point.

I don't want you to panic just because of invlid user call, but
whatever you call in comment must be enforced in code, right?


You can use the guard, if it guarantees the preemption disabled; or 
you can return None; you can create CONFIG_RUST_PERCPU_HARDENED for
panics.

Please refer the recent bitmap API wrapper, and how erroneous request
is handled there.

https://lore.kernel.org/all/20250904165015.3791895-4-bqe@google.com/