Message-ID: <CAJ-ks9ko3FGtpCnTwhuAb+KHwGczN_H8tM1=SQjRc3M-LgZrqA@mail.gmail.com>
Date: Mon, 17 Mar 2025 19:56:09 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nicolas Schier <nicolas@...sle.eu>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Luis Chamberlain <mcgrof@...nel.org>, 
	Russ Weight <russ.weight@...ux.dev>, Rob Herring <robh@...nel.org>, 
	Saravana Kannan <saravanak@...gle.com>, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, 
	linux-pci@...r.kernel.org, linux-block@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v5 6/6] rust: use strict provenance APIs

On Mon, Mar 17, 2025 at 5:36 PM Boqun Feng <boqun.feng@...il.com> wrote:
>
> On Mon, Mar 17, 2025 at 04:53:18PM -0400, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 4:46 PM Boqun Feng <boqun.feng@...il.com> wrote:
> > >
> > > On Mon, Mar 17, 2025 at 04:35:42PM -0400, Tamir Duberstein wrote:
> > > > On Mon, Mar 17, 2025 at 4:28 PM Boqun Feng <boqun.feng@...il.com> wrote:
> > > > >
> > > > > On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote:
> > > > > > On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@...il.com> wrote:
> > > > > > >
> > > > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@...il.com> wrote:
> > > > > > > >
> > > > > > > > Then we should fix clippy or how we set msrv rather adding the stub.
> > > > > > > > @Miguel?
> > > > > > >
> > > > > > > I filed https://github.com/rust-lang/rust-clippy/issues/14425.
> > > > > >
> > > > > > I don't think we can wait for that to be fixed, though. Usually clippy
> > > > > > is distributed with rustc via rustup, so even if this is eventually
> > > > > > fixed, all versions between 1.84.0 and the fix will need this
> > > > > > workaround until MSRV is >= 1.84.0.
> > > > >
> > > > > We need to take one step back to evalute this "workaround".
> > > > >
> > > > > First, expose_provenance() and with_exposed_provenance{,_mut}() API are
> > > > > clearly defined as equavilent to `as` operation [1]. Therefore, the
> > > > > changes in this patch doing the conversion with expose_provenance() and
> > > > > with_exposed_provenance{,_mut}() don't change anything related to
> > > > > provenance in practice.
> > > > >
> > > > > I do agree we want to use the explicit provenance API, but I don't think
> > > > > we want to introduce some API that we know we will change them latter
> > > > > when we bump the rustc minimal version. So the question is: are these
> > > > > stubs what we want even though in the future our minimal rustc version
> > > > > stablizes provenance API? If not, then the cost of this patch cannot
> > > > > justify its benefits IMO.
> > > > >
> > > > > Now let's also look into why we choose a msrv for clippy, I would guess
> > > > > it's because we need to support all the versions of rustc starting at
> > > > > 1.78 and we want clippy to report a problem based on 1.78 even though
> > > > > we're using a higher version of rustc. But for this particular case, we
> > > > > use a feature that has already been stablized in a higher version of
> > > > > rustc, which means the problem reported by clippy doesn't help us, nor
> > > > > does it provide better code. Frankly speaking, I think we have other
> > > > > ways to ensure the support of all rustc versions without a msrv for
> > > > > clippy. If I was to choose, I would simply drop the msrv. But maybe I'm
> > > > > missing something.
> > > > >
> > > > > The point is tools should help us to write good and maintainable code,
> > > > > we shouldn't introduce complicated structure of code just because some
> > > > > tools fail to do its job.
> > > > >
> > > > > [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html
> > > >
> > > > Even if we globally disable this clippy lint, we still need stubs
> > > > because exposed_provenance was added in 1.79.0. Did your suggestion
> > > > address this? Perhaps I missed it.
> > >
> > > No, I didn't.
> > >
> > > That's a separate topic though, because I can see the argument that:
> > > because with_exposed_provenance() is a function rather than a method, it
> > > won't be very benefical to use ptr::with_exposed_provenance() instead of
> > > kernel::with_exposed_provenance(), therefor these stubs of
> > > exposed_provenance make sense to exist. But I don't think the same
> > > argument works for ptr::{with_,map_,}addr().
> >
> > What about `pointer::expose_provenance`? It's a method that was added in 1.79.0.
> >
>
> We have a few options:
>
> 1) we can decide to use funtion-version of expose_provenance() (i.e. the
>    stub), if we feel the symmetry with with_exposed_provenance() is
>    a strong rationale. This also means we won't likely use
>    pointer::expose_provenance() in the future. That is, although kernel
>    doesn't have stable internal API, but in the foreseeable future, we
>    decide to use funtion-version of expose_provenance().

I don't think we want these stubs forever.

> 2) we can introduce a PtrExt trait for <1.79
>
>    pub trait PtrExt<T> {
>        fn expose_provenance(self) -> usize;
>    }
>
>    and
>
>    impl<T> PtrExt<T> for *const T {
>         ...
>    }
>
>    and `PtrExt` in kernel::prelude.
>
>    (we need to #[allow(unstable_name_collisions)] to make that work)

I like this idea, but I can't get it to work. When both inherent and
trait methods are available, the compiler seems to prefer the inherent
method.

>    We can also make with_exposed_provenance() use the same *Ext trick,
>    and remove it when we bump the minimal rustc version.

This part I don't understand. What would we impl the Ext on, given
that `with_exposed_provenance` is a free function?

Option 3) take this series without the last commit, and revisit when
MSRV >= 1.79.0 or >= 1.84.0?

Powered by blists - more mailing lists