| Message ID | 20250409-ptr-as-ptr-v8-6-3738061534ef@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: reduce `as` casts, enable related lints | expand |
On Wed Apr 9, 2025 at 4:47 PM CEST, Tamir Duberstein wrote: > In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]: > >> Using `as` casts may result in silently changing mutability or type. > > While this doesn't eliminate unchecked `as` conversions, it makes such > conversions easier to scrutinize. It also has the slight benefit of > removing a degree of freedom on which to bikeshed. Thus apply the > changes and enable the lint -- no functional change intended. > > Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] > Suggested-by: Benno Lossin <benno.lossin@proton.me> > Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ > Signed-off-by: Tamir Duberstein <tamird@gmail.com> Reviewed-by: Benno Lossin <benno.lossin@proton.me> --- Cheers, Benno > --- > Makefile | 1 + > rust/bindings/lib.rs | 1 + > rust/kernel/device_id.rs | 3 ++- > rust/kernel/fs/file.rs | 3 ++- > rust/kernel/str.rs | 6 ++++-- > rust/kernel/uaccess.rs | 10 ++++------ > rust/uapi/lib.rs | 1 + > 7 files changed, 15 insertions(+), 10 deletions(-)
On Wed, Apr 09, 2025 at 10:47:23AM -0400, Tamir Duberstein wrote: > In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]: > > > Using `as` casts may result in silently changing mutability or type. > > While this doesn't eliminate unchecked `as` conversions, it makes such > conversions easier to scrutinize. It also has the slight benefit of > removing a degree of freedom on which to bikeshed. Thus apply the > changes and enable the lint -- no functional change intended. > > Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] > Suggested-by: Benno Lossin <benno.lossin@proton.me> > Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > --- > Makefile | 1 + > rust/bindings/lib.rs | 1 + > rust/kernel/device_id.rs | 3 ++- > rust/kernel/fs/file.rs | 3 ++- > rust/kernel/str.rs | 6 ++++-- > rust/kernel/uaccess.rs | 10 ++++------ > rust/uapi/lib.rs | 1 + > 7 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/Makefile b/Makefile > index eb5a942241a2..2a16e02f26db 100644 > --- a/Makefile > +++ b/Makefile > @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ > -Wclippy::no_mangle_with_rust_abi \ > -Wclippy::ptr_as_ptr \ > -Wclippy::ptr_cast_constness \ > + -Wclippy::ref_as_ptr \ > -Wclippy::undocumented_unsafe_blocks \ > -Wclippy::unnecessary_safety_comment \ > -Wclippy::unnecessary_safety_doc \ > diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs > index b105a0d899cc..2b69016070c6 100644 > --- a/rust/bindings/lib.rs > +++ b/rust/bindings/lib.rs > @@ -27,6 +27,7 @@ > #[allow(dead_code)] > #[allow(clippy::cast_lossless)] > #[allow(clippy::ptr_as_ptr)] > +#[allow(clippy::ref_as_ptr)] > #[allow(clippy::undocumented_unsafe_blocks)] > mod bindings_raw { > // Manual definition for blocklisted types. > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > index 4063f09d76d9..37cc03d1df4c 100644 > --- a/rust/kernel/device_id.rs > +++ b/rust/kernel/device_id.rs > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > fn as_ptr(&self) -> *const T::RawType { > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > // to access the sentinel. > - (self as *const Self).cast() > + let this: *const Self = self; Hmm.. so this lint usually just requires to use a let statement instead of as expression when casting a reference to a pointer? Not 100% convinced this results into better code TBH.. > + this.cast() > } > > fn id(&self, index: usize) -> &T::RawType { > diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs > index 791f493ada10..559a4bfa123f 100644 > --- a/rust/kernel/fs/file.rs > +++ b/rust/kernel/fs/file.rs > @@ -359,12 +359,13 @@ impl core::ops::Deref for File { > type Target = LocalFile; > #[inline] > fn deref(&self) -> &LocalFile { > + let this: *const Self = self; > // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a > // valid file for the desired duration. > // > // By the type invariants, there are no `fdget_pos` calls that did not take the > // `f_pos_lock` mutex. > - unsafe { LocalFile::from_raw_file((self as *const Self).cast()) } > + unsafe { LocalFile::from_raw_file(this.cast()) } > } > } > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > index 40034f77fc2f..75b4a18c67c4 100644 > --- a/rust/kernel/str.rs > +++ b/rust/kernel/str.rs > @@ -28,8 +28,9 @@ pub const fn is_empty(&self) -> bool { > /// Creates a [`BStr`] from a `[u8]`. > #[inline] > pub const fn from_bytes(bytes: &[u8]) -> &Self { > + let bytes: *const [u8] = bytes; > // SAFETY: `BStr` is transparent to `[u8]`. > - unsafe { &*(bytes as *const [u8] as *const BStr) } > + unsafe { &*(bytes as *const BStr) } unsafe { &*(bytes.cast::<BStr>()) } ? I'm curious why this dodged the other lint (ptr_as_ptr). > } > > /// Strip a prefix from `self`. Delegates to [`slice::strip_prefix`]. > @@ -289,8 +290,9 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError > /// `NUL` byte (or the string will be truncated). > #[inline] > pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { > + let bytes: *mut [u8] = bytes; > // SAFETY: Properties of `bytes` guaranteed by the safety precondition. > - unsafe { &mut *(bytes as *mut [u8] as *mut CStr) } > + unsafe { &mut *(bytes as *mut CStr) } Ditto. > } > > /// Returns a C pointer to the string. > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs > index 80a9782b1c6e..7a6fc78fc314 100644 > --- a/rust/kernel/uaccess.rs > +++ b/rust/kernel/uaccess.rs > @@ -240,9 +240,10 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { > /// Fails with [`EFAULT`] if the read happens on a bad address, or if the read goes out of > /// bounds of this [`UserSliceReader`]. This call may modify `out` even if it returns an error. > pub fn read_slice(&mut self, out: &mut [u8]) -> Result { > + let out: *mut [u8] = out; > // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to > // `out`. > - let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) }; > + let out = unsafe { &mut *(out as *mut [MaybeUninit<u8>]) }; Ditto. Regards, Boqun > self.read_raw(out) > } > > @@ -348,6 +349,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { > if len > self.length { > return Err(EFAULT); > } > + let value: *const T = value; > // SAFETY: The reference points to a value of type `T`, so it is valid for reading > // `size_of::<T>()` bytes. > // > @@ -355,11 +357,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { > // kernel pointer. This mirrors the logic on the C side that skips the check when the length > // is a compile-time constant. > let res = unsafe { > - bindings::_copy_to_user( > - self.ptr as *mut c_void, > - (value as *const T).cast::<c_void>(), > - len, > - ) > + bindings::_copy_to_user(self.ptr as *mut c_void, value.cast::<c_void>(), len) > }; > if res != 0 { > return Err(EFAULT); > diff --git a/rust/uapi/lib.rs b/rust/uapi/lib.rs > index d5dab4dfabec..6230ba48201d 100644 > --- a/rust/uapi/lib.rs > +++ b/rust/uapi/lib.rs > @@ -16,6 +16,7 @@ > clippy::all, > clippy::cast_lossless, > clippy::ptr_as_ptr, > + clippy::ref_as_ptr, > clippy::undocumented_unsafe_blocks, > dead_code, > missing_docs, > > -- > 2.49.0 > >
Hi Boqun, thanks for having a look! On Tue, Apr 15, 2025 at 1:37 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Wed, Apr 09, 2025 at 10:47:23AM -0400, Tamir Duberstein wrote: > > In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]: > > > > > Using `as` casts may result in silently changing mutability or type. > > > > While this doesn't eliminate unchecked `as` conversions, it makes such > > conversions easier to scrutinize. It also has the slight benefit of > > removing a degree of freedom on which to bikeshed. Thus apply the > > changes and enable the lint -- no functional change intended. > > > > Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > --- > > Makefile | 1 + > > rust/bindings/lib.rs | 1 + > > rust/kernel/device_id.rs | 3 ++- > > rust/kernel/fs/file.rs | 3 ++- > > rust/kernel/str.rs | 6 ++++-- > > rust/kernel/uaccess.rs | 10 ++++------ > > rust/uapi/lib.rs | 1 + > > 7 files changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index eb5a942241a2..2a16e02f26db 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ > > -Wclippy::no_mangle_with_rust_abi \ > > -Wclippy::ptr_as_ptr \ > > -Wclippy::ptr_cast_constness \ > > + -Wclippy::ref_as_ptr \ > > -Wclippy::undocumented_unsafe_blocks \ > > -Wclippy::unnecessary_safety_comment \ > > -Wclippy::unnecessary_safety_doc \ > > diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs > > index b105a0d899cc..2b69016070c6 100644 > > --- a/rust/bindings/lib.rs > > +++ b/rust/bindings/lib.rs > > @@ -27,6 +27,7 @@ > > #[allow(dead_code)] > > #[allow(clippy::cast_lossless)] > > #[allow(clippy::ptr_as_ptr)] > > +#[allow(clippy::ref_as_ptr)] > > #[allow(clippy::undocumented_unsafe_blocks)] > > mod bindings_raw { > > // Manual definition for blocklisted types. > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > index 4063f09d76d9..37cc03d1df4c 100644 > > --- a/rust/kernel/device_id.rs > > +++ b/rust/kernel/device_id.rs > > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > > fn as_ptr(&self) -> *const T::RawType { > > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > > // to access the sentinel. > > - (self as *const Self).cast() > > + let this: *const Self = self; > > Hmm.. so this lint usually just requires to use a let statement instead > of as expression when casting a reference to a pointer? Not 100% > convinced this results into better code TBH.. The rationale is in the lint description and quoted in the commit message: "Using `as` casts may result in silently changing mutability or type.". > > > + this.cast() > > } > > > > fn id(&self, index: usize) -> &T::RawType { > > diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs > > index 791f493ada10..559a4bfa123f 100644 > > --- a/rust/kernel/fs/file.rs > > +++ b/rust/kernel/fs/file.rs > > @@ -359,12 +359,13 @@ impl core::ops::Deref for File { > > type Target = LocalFile; > > #[inline] > > fn deref(&self) -> &LocalFile { > > + let this: *const Self = self; > > // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a > > // valid file for the desired duration. > > // > > // By the type invariants, there are no `fdget_pos` calls that did not take the > > // `f_pos_lock` mutex. > > - unsafe { LocalFile::from_raw_file((self as *const Self).cast()) } > > + unsafe { LocalFile::from_raw_file(this.cast()) } > > } > > } > > > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > > index 40034f77fc2f..75b4a18c67c4 100644 > > --- a/rust/kernel/str.rs > > +++ b/rust/kernel/str.rs > > @@ -28,8 +28,9 @@ pub const fn is_empty(&self) -> bool { > > /// Creates a [`BStr`] from a `[u8]`. > > #[inline] > > pub const fn from_bytes(bytes: &[u8]) -> &Self { > > + let bytes: *const [u8] = bytes; > > // SAFETY: `BStr` is transparent to `[u8]`. > > - unsafe { &*(bytes as *const [u8] as *const BStr) } > > + unsafe { &*(bytes as *const BStr) } > > unsafe { &*(bytes.cast::<BStr>()) } > > ? I'm curious why this dodged the other lint (ptr_as_ptr). The reason it has to be written this way is that BStr is !Sized, and `pointer::cast` has an implicit Sized bound. Perhaps the lint is smart enough to avoid the suggestion in that case? Seems like yes: https://github.com/rust-lang/rust-clippy/blob/d3267e9230940757fde2fcb608605bf8dbfd85e1/clippy_lints/src/casts/ptr_as_ptr.rs#L36. > > > } > > > > /// Strip a prefix from `self`. Delegates to [`slice::strip_prefix`]. > > @@ -289,8 +290,9 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError > > /// `NUL` byte (or the string will be truncated). > > #[inline] > > pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { > > + let bytes: *mut [u8] = bytes; > > // SAFETY: Properties of `bytes` guaranteed by the safety precondition. > > - unsafe { &mut *(bytes as *mut [u8] as *mut CStr) } > > + unsafe { &mut *(bytes as *mut CStr) } > > Ditto. > > > } > > > > /// Returns a C pointer to the string. > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs > > index 80a9782b1c6e..7a6fc78fc314 100644 > > --- a/rust/kernel/uaccess.rs > > +++ b/rust/kernel/uaccess.rs > > @@ -240,9 +240,10 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { > > /// Fails with [`EFAULT`] if the read happens on a bad address, or if the read goes out of > > /// bounds of this [`UserSliceReader`]. This call may modify `out` even if it returns an error. > > pub fn read_slice(&mut self, out: &mut [u8]) -> Result { > > + let out: *mut [u8] = out; > > // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to > > // `out`. > > - let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) }; > > + let out = unsafe { &mut *(out as *mut [MaybeUninit<u8>]) }; > > Ditto. Same rationale here. Cheers. Tamir
On Tue, Apr 15, 2025 at 01:58:41PM -0400, Tamir Duberstein wrote: > Hi Boqun, thanks for having a look! > > On Tue, Apr 15, 2025 at 1:37 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > On Wed, Apr 09, 2025 at 10:47:23AM -0400, Tamir Duberstein wrote: > > > In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]: > > > > > > > Using `as` casts may result in silently changing mutability or type. > > > > > > While this doesn't eliminate unchecked `as` conversions, it makes such > > > conversions easier to scrutinize. It also has the slight benefit of > > > removing a degree of freedom on which to bikeshed. Thus apply the > > > changes and enable the lint -- no functional change intended. > > > > > > Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] > > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > > Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > --- > > > Makefile | 1 + > > > rust/bindings/lib.rs | 1 + > > > rust/kernel/device_id.rs | 3 ++- > > > rust/kernel/fs/file.rs | 3 ++- > > > rust/kernel/str.rs | 6 ++++-- > > > rust/kernel/uaccess.rs | 10 ++++------ > > > rust/uapi/lib.rs | 1 + > > > 7 files changed, 15 insertions(+), 10 deletions(-) > > > > > > diff --git a/Makefile b/Makefile > > > index eb5a942241a2..2a16e02f26db 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ > > > -Wclippy::no_mangle_with_rust_abi \ > > > -Wclippy::ptr_as_ptr \ > > > -Wclippy::ptr_cast_constness \ > > > + -Wclippy::ref_as_ptr \ > > > -Wclippy::undocumented_unsafe_blocks \ > > > -Wclippy::unnecessary_safety_comment \ > > > -Wclippy::unnecessary_safety_doc \ > > > diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs > > > index b105a0d899cc..2b69016070c6 100644 > > > --- a/rust/bindings/lib.rs > > > +++ b/rust/bindings/lib.rs > > > @@ -27,6 +27,7 @@ > > > #[allow(dead_code)] > > > #[allow(clippy::cast_lossless)] > > > #[allow(clippy::ptr_as_ptr)] > > > +#[allow(clippy::ref_as_ptr)] > > > #[allow(clippy::undocumented_unsafe_blocks)] > > > mod bindings_raw { > > > // Manual definition for blocklisted types. > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > > index 4063f09d76d9..37cc03d1df4c 100644 > > > --- a/rust/kernel/device_id.rs > > > +++ b/rust/kernel/device_id.rs > > > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > > > fn as_ptr(&self) -> *const T::RawType { > > > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > > > // to access the sentinel. > > > - (self as *const Self).cast() > > > + let this: *const Self = self; > > > > Hmm.. so this lint usually just requires to use a let statement instead > > of as expression when casting a reference to a pointer? Not 100% > > convinced this results into better code TBH.. > > The rationale is in the lint description and quoted in the commit > message: "Using `as` casts may result in silently changing mutability > or type.". > Could you show me how you can silently change the mutability or type? A simple try like below doesn't compile: let x = &42; let ptr = x as *mut i32; // <- error let another_ptr = x as *const i64; // <- error also from the link document you shared, looks like the suggestion is to use core::ptr::from_{ref,mut}(), was this ever considered? > > > > > + this.cast() > > > } > > > > > > fn id(&self, index: usize) -> &T::RawType { > > > diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs > > > index 791f493ada10..559a4bfa123f 100644 > > > --- a/rust/kernel/fs/file.rs > > > +++ b/rust/kernel/fs/file.rs > > > @@ -359,12 +359,13 @@ impl core::ops::Deref for File { > > > type Target = LocalFile; > > > #[inline] > > > fn deref(&self) -> &LocalFile { > > > + let this: *const Self = self; > > > // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a > > > // valid file for the desired duration. > > > // > > > // By the type invariants, there are no `fdget_pos` calls that did not take the > > > // `f_pos_lock` mutex. > > > - unsafe { LocalFile::from_raw_file((self as *const Self).cast()) } > > > + unsafe { LocalFile::from_raw_file(this.cast()) } > > > } > > > } > > > > > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > > > index 40034f77fc2f..75b4a18c67c4 100644 > > > --- a/rust/kernel/str.rs > > > +++ b/rust/kernel/str.rs > > > @@ -28,8 +28,9 @@ pub const fn is_empty(&self) -> bool { > > > /// Creates a [`BStr`] from a `[u8]`. > > > #[inline] > > > pub const fn from_bytes(bytes: &[u8]) -> &Self { > > > + let bytes: *const [u8] = bytes; > > > // SAFETY: `BStr` is transparent to `[u8]`. > > > - unsafe { &*(bytes as *const [u8] as *const BStr) } > > > + unsafe { &*(bytes as *const BStr) } > > > > unsafe { &*(bytes.cast::<BStr>()) } > > > > ? I'm curious why this dodged the other lint (ptr_as_ptr). > > The reason it has to be written this way is that BStr is !Sized, and > `pointer::cast` has an implicit Sized bound. > > Perhaps the lint is smart enough to avoid the suggestion in that case? > Seems like yes: > https://github.com/rust-lang/rust-clippy/blob/d3267e9230940757fde2fcb608605bf8dbfd85e1/clippy_lints/src/casts/ptr_as_ptr.rs#L36. > Oh, I see, so fat pointers get their way from this check? Hmm... however fat pointers also suffer the same problem that ptr_as_ptr prevents, right? How should we avoid that? Regards, Boqun
On Tue, Apr 15, 2025 at 2:18 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Tue, Apr 15, 2025 at 01:58:41PM -0400, Tamir Duberstein wrote: > > Hi Boqun, thanks for having a look! > > > > On Tue, Apr 15, 2025 at 1:37 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > On Wed, Apr 09, 2025 at 10:47:23AM -0400, Tamir Duberstein wrote: > > > > In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]: > > > > > > > > > Using `as` casts may result in silently changing mutability or type. > > > > > > > > While this doesn't eliminate unchecked `as` conversions, it makes such > > > > conversions easier to scrutinize. It also has the slight benefit of > > > > removing a degree of freedom on which to bikeshed. Thus apply the > > > > changes and enable the lint -- no functional change intended. > > > > > > > > Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] > > > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > > > Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ > > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > --- > > > > Makefile | 1 + > > > > rust/bindings/lib.rs | 1 + > > > > rust/kernel/device_id.rs | 3 ++- > > > > rust/kernel/fs/file.rs | 3 ++- > > > > rust/kernel/str.rs | 6 ++++-- > > > > rust/kernel/uaccess.rs | 10 ++++------ > > > > rust/uapi/lib.rs | 1 + > > > > 7 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/Makefile b/Makefile > > > > index eb5a942241a2..2a16e02f26db 100644 > > > > --- a/Makefile > > > > +++ b/Makefile > > > > @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ > > > > -Wclippy::no_mangle_with_rust_abi \ > > > > -Wclippy::ptr_as_ptr \ > > > > -Wclippy::ptr_cast_constness \ > > > > + -Wclippy::ref_as_ptr \ > > > > -Wclippy::undocumented_unsafe_blocks \ > > > > -Wclippy::unnecessary_safety_comment \ > > > > -Wclippy::unnecessary_safety_doc \ > > > > diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs > > > > index b105a0d899cc..2b69016070c6 100644 > > > > --- a/rust/bindings/lib.rs > > > > +++ b/rust/bindings/lib.rs > > > > @@ -27,6 +27,7 @@ > > > > #[allow(dead_code)] > > > > #[allow(clippy::cast_lossless)] > > > > #[allow(clippy::ptr_as_ptr)] > > > > +#[allow(clippy::ref_as_ptr)] > > > > #[allow(clippy::undocumented_unsafe_blocks)] > > > > mod bindings_raw { > > > > // Manual definition for blocklisted types. > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > > > index 4063f09d76d9..37cc03d1df4c 100644 > > > > --- a/rust/kernel/device_id.rs > > > > +++ b/rust/kernel/device_id.rs > > > > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > > > > fn as_ptr(&self) -> *const T::RawType { > > > > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > > > > // to access the sentinel. > > > > - (self as *const Self).cast() > > > > + let this: *const Self = self; > > > > > > Hmm.. so this lint usually just requires to use a let statement instead > > > of as expression when casting a reference to a pointer? Not 100% > > > convinced this results into better code TBH.. > > > > The rationale is in the lint description and quoted in the commit > > message: "Using `as` casts may result in silently changing mutability > > or type.". > > > > Could you show me how you can silently change the mutability or type? A > simple try like below doesn't compile: > > let x = &42; > let ptr = x as *mut i32; // <- error > let another_ptr = x as *const i64; // <- error I think the point is that the meaning of an `as` cast can change when the type of `x` changes, which can happen at a distance. The example shown in the clippy docs uses `as _`, which is where you get into real trouble. > also from the link document you shared, looks like the suggestion is to > use core::ptr::from_{ref,mut}(), was this ever considered? I considered it, but I thought it was ugly. We don't have a linter to enforce it, so I'd be surprised if people reached for it. > > > > > > > > + this.cast() > > > > } > > > > > > > > fn id(&self, index: usize) -> &T::RawType { > > > > diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs > > > > index 791f493ada10..559a4bfa123f 100644 > > > > --- a/rust/kernel/fs/file.rs > > > > +++ b/rust/kernel/fs/file.rs > > > > @@ -359,12 +359,13 @@ impl core::ops::Deref for File { > > > > type Target = LocalFile; > > > > #[inline] > > > > fn deref(&self) -> &LocalFile { > > > > + let this: *const Self = self; > > > > // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a > > > > // valid file for the desired duration. > > > > // > > > > // By the type invariants, there are no `fdget_pos` calls that did not take the > > > > // `f_pos_lock` mutex. > > > > - unsafe { LocalFile::from_raw_file((self as *const Self).cast()) } > > > > + unsafe { LocalFile::from_raw_file(this.cast()) } > > > > } > > > > } > > > > > > > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > > > > index 40034f77fc2f..75b4a18c67c4 100644 > > > > --- a/rust/kernel/str.rs > > > > +++ b/rust/kernel/str.rs > > > > @@ -28,8 +28,9 @@ pub const fn is_empty(&self) -> bool { > > > > /// Creates a [`BStr`] from a `[u8]`. > > > > #[inline] > > > > pub const fn from_bytes(bytes: &[u8]) -> &Self { > > > > + let bytes: *const [u8] = bytes; > > > > // SAFETY: `BStr` is transparent to `[u8]`. > > > > - unsafe { &*(bytes as *const [u8] as *const BStr) } > > > > + unsafe { &*(bytes as *const BStr) } > > > > > > unsafe { &*(bytes.cast::<BStr>()) } > > > > > > ? I'm curious why this dodged the other lint (ptr_as_ptr). > > > > The reason it has to be written this way is that BStr is !Sized, and > > `pointer::cast` has an implicit Sized bound. > > > > Perhaps the lint is smart enough to avoid the suggestion in that case? > > Seems like yes: > > https://github.com/rust-lang/rust-clippy/blob/d3267e9230940757fde2fcb608605bf8dbfd85e1/clippy_lints/src/casts/ptr_as_ptr.rs#L36. > > > > Oh, I see, so fat pointers get their way from this check? Hmm... however > fat pointers also suffer the same problem that ptr_as_ptr prevents, > right? How should we avoid that? Probably the proper solution is to remove the `Sized` bound from `pointer::cast`. Short of that, I'm not sure how -- but I don't think this deficiency should prevent us from the benefits of this change, even if they are a bit limited.
On Tue, Apr 15, 2025 at 04:10:01PM -0400, Tamir Duberstein wrote: > On Tue, Apr 15, 2025 at 2:18 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > On Tue, Apr 15, 2025 at 01:58:41PM -0400, Tamir Duberstein wrote: > > > Hi Boqun, thanks for having a look! > > > > > > On Tue, Apr 15, 2025 at 1:37 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > On Wed, Apr 09, 2025 at 10:47:23AM -0400, Tamir Duberstein wrote: > > > > > In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]: > > > > > > > > > > > Using `as` casts may result in silently changing mutability or type. > > > > > > > > > > While this doesn't eliminate unchecked `as` conversions, it makes such > > > > > conversions easier to scrutinize. It also has the slight benefit of > > > > > removing a degree of freedom on which to bikeshed. Thus apply the > > > > > changes and enable the lint -- no functional change intended. > > > > > > > > > > Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] > > > > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > > > > Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ > > > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > > --- > > > > > Makefile | 1 + > > > > > rust/bindings/lib.rs | 1 + > > > > > rust/kernel/device_id.rs | 3 ++- > > > > > rust/kernel/fs/file.rs | 3 ++- > > > > > rust/kernel/str.rs | 6 ++++-- > > > > > rust/kernel/uaccess.rs | 10 ++++------ > > > > > rust/uapi/lib.rs | 1 + > > > > > 7 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > index eb5a942241a2..2a16e02f26db 100644 > > > > > --- a/Makefile > > > > > +++ b/Makefile > > > > > @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ > > > > > -Wclippy::no_mangle_with_rust_abi \ > > > > > -Wclippy::ptr_as_ptr \ > > > > > -Wclippy::ptr_cast_constness \ > > > > > + -Wclippy::ref_as_ptr \ > > > > > -Wclippy::undocumented_unsafe_blocks \ > > > > > -Wclippy::unnecessary_safety_comment \ > > > > > -Wclippy::unnecessary_safety_doc \ > > > > > diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs > > > > > index b105a0d899cc..2b69016070c6 100644 > > > > > --- a/rust/bindings/lib.rs > > > > > +++ b/rust/bindings/lib.rs > > > > > @@ -27,6 +27,7 @@ > > > > > #[allow(dead_code)] > > > > > #[allow(clippy::cast_lossless)] > > > > > #[allow(clippy::ptr_as_ptr)] > > > > > +#[allow(clippy::ref_as_ptr)] > > > > > #[allow(clippy::undocumented_unsafe_blocks)] > > > > > mod bindings_raw { > > > > > // Manual definition for blocklisted types. > > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > > > > index 4063f09d76d9..37cc03d1df4c 100644 > > > > > --- a/rust/kernel/device_id.rs > > > > > +++ b/rust/kernel/device_id.rs > > > > > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > > > > > fn as_ptr(&self) -> *const T::RawType { > > > > > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > > > > > // to access the sentinel. > > > > > - (self as *const Self).cast() > > > > > + let this: *const Self = self; > > > > > > > > Hmm.. so this lint usually just requires to use a let statement instead > > > > of as expression when casting a reference to a pointer? Not 100% > > > > convinced this results into better code TBH.. > > > > > > The rationale is in the lint description and quoted in the commit > > > message: "Using `as` casts may result in silently changing mutability > > > or type.". > > > > > > > Could you show me how you can silently change the mutability or type? A > > simple try like below doesn't compile: > > > > let x = &42; > > let ptr = x as *mut i32; // <- error > > let another_ptr = x as *const i64; // <- error > > I think the point is that the meaning of an `as` cast can change when > the type of `x` changes, which can happen at a distance. The example So my example shows that you can only use `as` to convert a `&T` into a `*const T`, no matter how far it happens, and.. > shown in the clippy docs uses `as _`, which is where you get into real > trouble. > ... no matter whether `as _` is used or not. Of course once you have a `*const T`, using `as` can change it to a different type or mutability, but that's a different problem. Your argument still lacks convincing evidences or examples showing this is a real trouble. For example, if you have a `x` of type `&i32`, and do a `x as _` somewhere, you will have a compiler error once compilers infers a type that is not `*const i32` for `_`. If your argument being it's better do the reference-to-pointer conversion explicitly, then that makes some sense, but I still don't think we need to do it globablly. > > also from the link document you shared, looks like the suggestion is to > > use core::ptr::from_{ref,mut}(), was this ever considered? > > I considered it, but I thought it was ugly. We don't have a linter to > enforce it, so I'd be surprised if people reached for it. > I think avoiding the extra line of `let` is a win, also I don't get why you feel it's *ugly*: having the extra `let` line is ugly to me ;-) Regards, Boqun [...]
On Tue, Apr 15, 2025 at 4:51 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Tue, Apr 15, 2025 at 04:10:01PM -0400, Tamir Duberstein wrote: > > On Tue, Apr 15, 2025 at 2:18 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > On Tue, Apr 15, 2025 at 01:58:41PM -0400, Tamir Duberstein wrote: > > > > Hi Boqun, thanks for having a look! > > > > > > > > On Tue, Apr 15, 2025 at 1:37 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > On Wed, Apr 09, 2025 at 10:47:23AM -0400, Tamir Duberstein wrote: > > > > > > In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]: > > > > > > > > > > > > > Using `as` casts may result in silently changing mutability or type. > > > > > > > > > > > > While this doesn't eliminate unchecked `as` conversions, it makes such > > > > > > conversions easier to scrutinize. It also has the slight benefit of > > > > > > removing a degree of freedom on which to bikeshed. Thus apply the > > > > > > changes and enable the lint -- no functional change intended. > > > > > > > > > > > > Link: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr [1] > > > > > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > > > > > Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ > > > > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > > > --- > > > > > > Makefile | 1 + > > > > > > rust/bindings/lib.rs | 1 + > > > > > > rust/kernel/device_id.rs | 3 ++- > > > > > > rust/kernel/fs/file.rs | 3 ++- > > > > > > rust/kernel/str.rs | 6 ++++-- > > > > > > rust/kernel/uaccess.rs | 10 ++++------ > > > > > > rust/uapi/lib.rs | 1 + > > > > > > 7 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > > index eb5a942241a2..2a16e02f26db 100644 > > > > > > --- a/Makefile > > > > > > +++ b/Makefile > > > > > > @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ > > > > > > -Wclippy::no_mangle_with_rust_abi \ > > > > > > -Wclippy::ptr_as_ptr \ > > > > > > -Wclippy::ptr_cast_constness \ > > > > > > + -Wclippy::ref_as_ptr \ > > > > > > -Wclippy::undocumented_unsafe_blocks \ > > > > > > -Wclippy::unnecessary_safety_comment \ > > > > > > -Wclippy::unnecessary_safety_doc \ > > > > > > diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs > > > > > > index b105a0d899cc..2b69016070c6 100644 > > > > > > --- a/rust/bindings/lib.rs > > > > > > +++ b/rust/bindings/lib.rs > > > > > > @@ -27,6 +27,7 @@ > > > > > > #[allow(dead_code)] > > > > > > #[allow(clippy::cast_lossless)] > > > > > > #[allow(clippy::ptr_as_ptr)] > > > > > > +#[allow(clippy::ref_as_ptr)] > > > > > > #[allow(clippy::undocumented_unsafe_blocks)] > > > > > > mod bindings_raw { > > > > > > // Manual definition for blocklisted types. > > > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > > > > > index 4063f09d76d9..37cc03d1df4c 100644 > > > > > > --- a/rust/kernel/device_id.rs > > > > > > +++ b/rust/kernel/device_id.rs > > > > > > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > > > > > > fn as_ptr(&self) -> *const T::RawType { > > > > > > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > > > > > > // to access the sentinel. > > > > > > - (self as *const Self).cast() > > > > > > + let this: *const Self = self; > > > > > > > > > > Hmm.. so this lint usually just requires to use a let statement instead > > > > > of as expression when casting a reference to a pointer? Not 100% > > > > > convinced this results into better code TBH.. > > > > > > > > The rationale is in the lint description and quoted in the commit > > > > message: "Using `as` casts may result in silently changing mutability > > > > or type.". > > > > > > > > > > Could you show me how you can silently change the mutability or type? A > > > simple try like below doesn't compile: > > > > > > let x = &42; > > > let ptr = x as *mut i32; // <- error > > > let another_ptr = x as *const i64; // <- error > > > > I think the point is that the meaning of an `as` cast can change when > > the type of `x` changes, which can happen at a distance. The example > > So my example shows that you can only use `as` to convert a `&T` into a > `*const T`, no matter how far it happens, and.. I don't think you're engaging with the point I'm making here. Suppose the type is `&mut T` initially and `as _` is being used to convert it to `*mut T`; now if the type of `&mut T` changes to `*const T`, you have completely different semantics. > > > shown in the clippy docs uses `as _`, which is where you get into real > > trouble. > > > > ... no matter whether `as _` is used or not. Of course once you have a > `*const T`, using `as` can change it to a different type or mutability, > but that's a different problem. Your argument still lacks convincing > evidences or examples showing this is a real trouble. For example, if > you have a `x` of type `&i32`, and do a `x as _` somewhere, you will > have a compiler error once compilers infers a type that is not `*const > i32` for `_`. If your argument being it's better do the > reference-to-pointer conversion explicitly, then that makes some sense, > but I still don't think we need to do it globablly. Can you help me understand what it is I need to convince you of? There was prior discussion in https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/, where it was suggested to use this lint. I suppose in any discussion of a chance, we should also enumerate the costs -- you're taking the position that the status quo is preferable, yes? Can you help me understand why? > > > > also from the link document you shared, looks like the suggestion is to > > > use core::ptr::from_{ref,mut}(), was this ever considered? > > > > I considered it, but I thought it was ugly. We don't have a linter to > > enforce it, so I'd be surprised if people reached for it. > > > > I think avoiding the extra line of `let` is a win, also I don't get why > you feel it's *ugly*: having the extra `let` line is ugly to me ;-) I admit it's subjective, so I'm happy to change it. But I've never seen that syntax used, and we lack enforcement for either one, so I don't find much value in arguing over this.
On Tue, Apr 15, 2025 at 04:59:01PM -0400, Tamir Duberstein wrote: [...] > > > > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > > > > > > index 4063f09d76d9..37cc03d1df4c 100644 > > > > > > > --- a/rust/kernel/device_id.rs > > > > > > > +++ b/rust/kernel/device_id.rs > > > > > > > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > > > > > > > fn as_ptr(&self) -> *const T::RawType { > > > > > > > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > > > > > > > // to access the sentinel. > > > > > > > - (self as *const Self).cast() > > > > > > > + let this: *const Self = self; > > > > > > > > > > > > Hmm.. so this lint usually just requires to use a let statement instead > > > > > > of as expression when casting a reference to a pointer? Not 100% > > > > > > convinced this results into better code TBH.. > > > > > > > > > > The rationale is in the lint description and quoted in the commit > > > > > message: "Using `as` casts may result in silently changing mutability > > > > > or type.". > > > > > > > > > > > > > Could you show me how you can silently change the mutability or type? A > > > > simple try like below doesn't compile: > > > > > > > > let x = &42; > > > > let ptr = x as *mut i32; // <- error > > > > let another_ptr = x as *const i64; // <- error > > > > > > I think the point is that the meaning of an `as` cast can change when > > > the type of `x` changes, which can happen at a distance. The example > > > > So my example shows that you can only use `as` to convert a `&T` into a > > `*const T`, no matter how far it happens, and.. > > I don't think you're engaging with the point I'm making here. Suppose > the type is `&mut T` initially and `as _` is being used to convert it > to `*mut T`; now if the type of `&mut T` changes to `*const T`, you have > completely different semantics. > You're right, I had some misunderstanding, the "`_`" part of `as _` seems to be a red herring, the problematic code snippet you meant can be shown as (without a `as _`): f(x as *mut T); // f() takes a `*mut T`. where it compiles with `x` being either a `&mut T` or `*const T`, and `as` has different meanings in these cases. > > > > > shown in the clippy docs uses `as _`, which is where you get into real > > > trouble. > > > > > > > ... no matter whether `as _` is used or not. Of course once you have a > > `*const T`, using `as` can change it to a different type or mutability, > > but that's a different problem. Your argument still lacks convincing > > evidences or examples showing this is a real trouble. For example, if > > you have a `x` of type `&i32`, and do a `x as _` somewhere, you will > > have a compiler error once compilers infers a type that is not `*const > > i32` for `_`. If your argument being it's better do the > > reference-to-pointer conversion explicitly, then that makes some sense, > > but I still don't think we need to do it globablly. > > Can you help me understand what it is I need to convince you of? There > was prior discussion in > https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/, > where it was suggested to use this lint. > > I suppose in any discussion of a chance, we should also enumerate the > costs -- you're taking the position that the status quo is preferable, > yes? Can you help me understand why? > In this case the status quo is not having the lint, which allows users to convert a raw pointer from a reference with `as`. What you proposed in patch is to do the conversion with a stand-alone let statement, and that IMO doesn't suit all the cases: we are dealing with C code a lot, that means dealing raw pointers a lot, it's handy and logically tight if we have an expression that converts a Rust location into a raw pointer. And forcing let statements every time is not really reasonble because of this. Also I didn't get the problematic code the lint can prevent as well until very recent discussion in this thread. I would not say the status quo is preferable, more like your changes in this patch complicate some simple patterns which are reasonable to me. And it's also weird that we use a lint but don't use its suggestion. So in short, I'm not against this lint, but if we only use let-statement resolution, I need to understand why and as you said, we need to evaluate the cost. > > > > > > also from the link document you shared, looks like the suggestion is to > > > > use core::ptr::from_{ref,mut}(), was this ever considered? > > > > > > I considered it, but I thought it was ugly. We don't have a linter to > > > enforce it, so I'd be surprised if people reached for it. > > > > > > > I think avoiding the extra line of `let` is a win, also I don't get why > > you feel it's *ugly*: having the extra `let` line is ugly to me ;-) > > I admit it's subjective, so I'm happy to change it. But I've never > seen that syntax used, and we lack enforcement for either one, so I > don't find much value in arguing over this. > If the original code use "as" for conversion purposes, I think it's good to be consistent and using from_ref() or from_mut(): they are just bullet-proof version of conversions, and having a separate let statement looks like a distraction to me. If for new code, and the author has a reason for let statement, then it's fine. Regards, Boqun
On Tue, Apr 15, 2025 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Tue, Apr 15, 2025 at 04:59:01PM -0400, Tamir Duberstein wrote: > [...] > > > > > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > > > > > > > index 4063f09d76d9..37cc03d1df4c 100644 > > > > > > > > --- a/rust/kernel/device_id.rs > > > > > > > > +++ b/rust/kernel/device_id.rs > > > > > > > > @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { > > > > > > > > fn as_ptr(&self) -> *const T::RawType { > > > > > > > > // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance > > > > > > > > // to access the sentinel. > > > > > > > > - (self as *const Self).cast() > > > > > > > > + let this: *const Self = self; > > > > > > > > > > > > > > Hmm.. so this lint usually just requires to use a let statement instead > > > > > > > of as expression when casting a reference to a pointer? Not 100% > > > > > > > convinced this results into better code TBH.. > > > > > > > > > > > > The rationale is in the lint description and quoted in the commit > > > > > > message: "Using `as` casts may result in silently changing mutability > > > > > > or type.". > > > > > > > > > > > > > > > > Could you show me how you can silently change the mutability or type? A > > > > > simple try like below doesn't compile: > > > > > > > > > > let x = &42; > > > > > let ptr = x as *mut i32; // <- error > > > > > let another_ptr = x as *const i64; // <- error > > > > > > > > I think the point is that the meaning of an `as` cast can change when > > > > the type of `x` changes, which can happen at a distance. The example > > > > > > So my example shows that you can only use `as` to convert a `&T` into a > > > `*const T`, no matter how far it happens, and.. > > > > I don't think you're engaging with the point I'm making here. Suppose > > the type is `&mut T` initially and `as _` is being used to convert it > > to `*mut T`; now if the type of `&mut T` changes to `*const T`, you have > > completely different semantics. > > > > You're right, I had some misunderstanding, the "`_`" part of `as _` > seems to be a red herring, the problematic code snippet you meant can be > shown as (without a `as _`): > > f(x as *mut T); // f() takes a `*mut T`. > > where it compiles with `x` being either a `&mut T` or `*const T`, and > `as` has different meanings in these cases. > > > > > > > > shown in the clippy docs uses `as _`, which is where you get into real > > > > trouble. > > > > > > > > > > ... no matter whether `as _` is used or not. Of course once you have a > > > `*const T`, using `as` can change it to a different type or mutability, > > > but that's a different problem. Your argument still lacks convincing > > > evidences or examples showing this is a real trouble. For example, if > > > you have a `x` of type `&i32`, and do a `x as _` somewhere, you will > > > have a compiler error once compilers infers a type that is not `*const > > > i32` for `_`. If your argument being it's better do the > > > reference-to-pointer conversion explicitly, then that makes some sense, > > > but I still don't think we need to do it globablly. > > > > Can you help me understand what it is I need to convince you of? There > > was prior discussion in > > https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/, > > where it was suggested to use this lint. > > > > I suppose in any discussion of a chance, we should also enumerate the > > costs -- you're taking the position that the status quo is preferable, > > yes? Can you help me understand why? > > > > In this case the status quo is not having the lint, which allows users > to convert a raw pointer from a reference with `as`. What you proposed > in patch is to do the conversion with a stand-alone let statement, and > that IMO doesn't suit all the cases: we are dealing with C code a lot, > that means dealing raw pointers a lot, it's handy and logically tight if > we have an expression that converts a Rust location into a raw pointer. > And forcing let statements every time is not really reasonble because of > this. > > Also I didn't get the problematic code the lint can prevent as well > until very recent discussion in this thread. > > I would not say the status quo is preferable, more like your changes in > this patch complicate some simple patterns which are reasonable to me. > And it's also weird that we use a lint but don't use its suggestion. > > So in short, I'm not against this lint, but if we only use let-statement > resolution, I need to understand why and as you said, we need to > evaluate the cost. > > > > > > > > > also from the link document you shared, looks like the suggestion is to > > > > > use core::ptr::from_{ref,mut}(), was this ever considered? > > > > > > > > I considered it, but I thought it was ugly. We don't have a linter to > > > > enforce it, so I'd be surprised if people reached for it. > > > > > > > > > > I think avoiding the extra line of `let` is a win, also I don't get why > > > you feel it's *ugly*: having the extra `let` line is ugly to me ;-) > > > > I admit it's subjective, so I'm happy to change it. But I've never > > seen that syntax used, and we lack enforcement for either one, so I > > don't find much value in arguing over this. > > > > If the original code use "as" for conversion purposes, I think it's good > to be consistent and using from_ref() or from_mut(): they are just > bullet-proof version of conversions, and having a separate let statement > looks like a distraction to me. If for new code, and the author has a > reason for let statement, then it's fine. Fine by me. I'll change the let statements to those methods on the next spin. Thanks for your feedback. Tamir
On Tue, Apr 15, 2025 at 07:08:42PM -0400, Tamir Duberstein wrote: [...] > > > > > > > > > > also from the link document you shared, looks like the suggestion is to > > > > > > use core::ptr::from_{ref,mut}(), was this ever considered? > > > > > > > > > > I considered it, but I thought it was ugly. We don't have a linter to > > > > > enforce it, so I'd be surprised if people reached for it. > > > > > > > > > > > > > I think avoiding the extra line of `let` is a win, also I don't get why > > > > you feel it's *ugly*: having the extra `let` line is ugly to me ;-) > > > > > > I admit it's subjective, so I'm happy to change it. But I've never > > > seen that syntax used, and we lack enforcement for either one, so I > > > don't find much value in arguing over this. > > > > > > > If the original code use "as" for conversion purposes, I think it's good > > to be consistent and using from_ref() or from_mut(): they are just > > bullet-proof version of conversions, and having a separate let statement > > looks like a distraction to me. If for new code, and the author has a > > reason for let statement, then it's fine. > > Fine by me. I'll change the let statements to those methods on the next spin. > Thanks! There are a few instances in the early patches as well, appreciate it if you can change them as well. Regards, Boqun > Thanks for your feedback. > Tamir
diff --git a/Makefile b/Makefile index eb5a942241a2..2a16e02f26db 100644 --- a/Makefile +++ b/Makefile @@ -485,6 +485,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::no_mangle_with_rust_abi \ -Wclippy::ptr_as_ptr \ -Wclippy::ptr_cast_constness \ + -Wclippy::ref_as_ptr \ -Wclippy::undocumented_unsafe_blocks \ -Wclippy::unnecessary_safety_comment \ -Wclippy::unnecessary_safety_doc \ diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs index b105a0d899cc..2b69016070c6 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -27,6 +27,7 @@ #[allow(dead_code)] #[allow(clippy::cast_lossless)] #[allow(clippy::ptr_as_ptr)] +#[allow(clippy::ref_as_ptr)] #[allow(clippy::undocumented_unsafe_blocks)] mod bindings_raw { // Manual definition for blocklisted types. diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index 4063f09d76d9..37cc03d1df4c 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { fn as_ptr(&self) -> *const T::RawType { // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance // to access the sentinel. - (self as *const Self).cast() + let this: *const Self = self; + this.cast() } fn id(&self, index: usize) -> &T::RawType { diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs index 791f493ada10..559a4bfa123f 100644 --- a/rust/kernel/fs/file.rs +++ b/rust/kernel/fs/file.rs @@ -359,12 +359,13 @@ impl core::ops::Deref for File { type Target = LocalFile; #[inline] fn deref(&self) -> &LocalFile { + let this: *const Self = self; // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a // valid file for the desired duration. // // By the type invariants, there are no `fdget_pos` calls that did not take the // `f_pos_lock` mutex. - unsafe { LocalFile::from_raw_file((self as *const Self).cast()) } + unsafe { LocalFile::from_raw_file(this.cast()) } } } diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..75b4a18c67c4 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -28,8 +28,9 @@ pub const fn is_empty(&self) -> bool { /// Creates a [`BStr`] from a `[u8]`. #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { + let bytes: *const [u8] = bytes; // SAFETY: `BStr` is transparent to `[u8]`. - unsafe { &*(bytes as *const [u8] as *const BStr) } + unsafe { &*(bytes as *const BStr) } } /// Strip a prefix from `self`. Delegates to [`slice::strip_prefix`]. @@ -289,8 +290,9 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError /// `NUL` byte (or the string will be truncated). #[inline] pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { + let bytes: *mut [u8] = bytes; // SAFETY: Properties of `bytes` guaranteed by the safety precondition. - unsafe { &mut *(bytes as *mut [u8] as *mut CStr) } + unsafe { &mut *(bytes as *mut CStr) } } /// Returns a C pointer to the string. diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 80a9782b1c6e..7a6fc78fc314 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -240,9 +240,10 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { /// Fails with [`EFAULT`] if the read happens on a bad address, or if the read goes out of /// bounds of this [`UserSliceReader`]. This call may modify `out` even if it returns an error. pub fn read_slice(&mut self, out: &mut [u8]) -> Result { + let out: *mut [u8] = out; // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to // `out`. - let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) }; + let out = unsafe { &mut *(out as *mut [MaybeUninit<u8>]) }; self.read_raw(out) } @@ -348,6 +349,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { if len > self.length { return Err(EFAULT); } + let value: *const T = value; // SAFETY: The reference points to a value of type `T`, so it is valid for reading // `size_of::<T>()` bytes. // @@ -355,11 +357,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { // kernel pointer. This mirrors the logic on the C side that skips the check when the length // is a compile-time constant. let res = unsafe { - bindings::_copy_to_user( - self.ptr as *mut c_void, - (value as *const T).cast::<c_void>(), - len, - ) + bindings::_copy_to_user(self.ptr as *mut c_void, value.cast::<c_void>(), len) }; if res != 0 { return Err(EFAULT); diff --git a/rust/uapi/lib.rs b/rust/uapi/lib.rs index d5dab4dfabec..6230ba48201d 100644 --- a/rust/uapi/lib.rs +++ b/rust/uapi/lib.rs @@ -16,6 +16,7 @@ clippy::all, clippy::cast_lossless, clippy::ptr_as_ptr, + clippy::ref_as_ptr, clippy::undocumented_unsafe_blocks, dead_code, missing_docs,