From nobody Mon Nov 25 05:53:00 2024 Received: from mail-oo1-f50.google.com (mail-oo1-f50.google.com [209.85.161.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A0D871BD9E2; Wed, 30 Oct 2024 20:46:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.161.50 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730321208; cv=none; b=lGWkUqnthYHRwNxdRF+TwdSqcXHOkLjUCxjWyD1eWoewWfm2pEOcd/lTOOVumrKlhhR58xS9CC3m7+bYmKN1AGje3by3VJRIDQT1O/ub3jpwsjz3kz2gG+mbc7T4iHsAS1RgX0MV3X+HrDBmuWneu/L4rMu63ebSzP+5tzmC/jk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730321208; c=relaxed/simple; bh=s+JOsokl8HTzsLJu0YwhoCbFGKiWcJDGG6609M+g3TU=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=eiW0bmMF5MII7HIsqBLuH9AX3Es0HTN9FJ6R4aK5aGJZ8blyk1AKZ3CZ5zHnB9Zp+2i4274kheCMUh/S1V4jPFCRjgDNaGjXdAzOjolc4/zhEikaJt5LQGFiGO7legibKCkRp8jaPjvsmNQNPJVepq0Eewp/0wykLBBJ3ljgVNA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=A5jl6PzN; arc=none smtp.client-ip=209.85.161.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="A5jl6PzN" Received: by mail-oo1-f50.google.com with SMTP id 006d021491bc7-5ebc22e6362so177140eaf.2; Wed, 30 Oct 2024 13:46:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730321204; x=1730926004; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=OW45e7gx4fvje5qEi3cakB/2Tqj9cnWab4H35fyWZ0I=; b=A5jl6PzNrHVmrXgKcPMvwa9J5KYDxB6HJR7Vk1VyGpvrlcDPsiHHBGqUiB8fbuMYcn j6FRaJEGJ5GCxrSxnFBUpabl5r6pN7mR7KswSDkbzG3UQ8lHiiHmop4JTls2MBAPSbYp D8QMje78Zd4RCPEOVOop8y6uOovVTiLllMqLcCY+bAhn0SjVSrMs+fcjNvl0tPPGXfsV g5TwphyRyIQL6sUyYOw318nq8OBuvyHh6Z53ohXWF+TFK1nW7Bt+cId1n6jEiv49+FGC U0FSwYHwaJyGXRZwIn1cL7xg8ULTq9q5WwrO+WBhh8MsPS79ukv7jApEBILhc666HEQl +W6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730321204; x=1730926004; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=OW45e7gx4fvje5qEi3cakB/2Tqj9cnWab4H35fyWZ0I=; b=aDSdqrtXFZ7LbtVXWxTtMldlFBjrTuknVRwIas+3jTc3F8uqNohglvpI6zSt84OuES /dC61LueMUyIU9qXj+Iq3KTus1YTOUHgv/AF6j+hAgE1Zssu9nQQKhpiYJ9jAbZCYVIm ymzroy1aM6xV4NKAgVioTmSpC3aymMWPyE2PouhGSLgv+bC9LxPlKJj5tnYM4F9MBnC1 0QYVqb2642VIMWn8hSepHa4i13MFwJMVphNWXpPVdB1fpuqKjEqQWGx+DTOyQNaLSTe7 X7UwrCM9Gquik0j7I2MUaHpqkLmtiMMyhEqkz7DoCzaUJ8Ajuw1yKfpxqcoWBe1FAjyj HoZQ== X-Forwarded-Encrypted: i=1; AJvYcCUu07Nwyvm0MurhYLAif+Dlvf1qE15bGa2qXHcRz7HWN3Je0RwlLRLKn+YU9r2wqQdvRZdNwiaImhw+CmU=@vger.kernel.org X-Gm-Message-State: AOJu0YzCFaFOJjGpbuOB2Yr2WJZZzjyPTtxXdEWTVcsKF93td4SwSgbm HnrUDu00tkk9mWJXc8057AWnVkEYD4oOUgegIhq+RvCxhj7li2We X-Google-Smtp-Source: AGHT+IEdneqM5xabBYVFBPR9NlxPfCAAwq2JkQfVv3RZABU0fxOGjJbduxuNWiZU/u66i4/TE9lO6A== X-Received: by 2002:a05:6358:7e06:b0:1c5:e2ea:8993 with SMTP id e5c5f4694b2df-1c5ee958a69mr97166155d.11.1730321204360; Wed, 30 Oct 2024 13:46:44 -0700 (PDT) Received: from 1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa ([2620:10d:c091:600::1:89dc]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-462ad0c72efsm271271cf.50.2024.10.30.13.46.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 30 Oct 2024 13:46:43 -0700 (PDT) From: Tamir Duberstein Date: Wed, 30 Oct 2024 16:46:39 -0400 Subject: [PATCH 2/5] rust: types: avoid `as` casts, narrow unsafe scope Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Message-Id: <20241030-borrow-mut-v1-2-8f0ceaf78eaf@gmail.com> References: <20241030-borrow-mut-v1-0-8f0ceaf78eaf@gmail.com> In-Reply-To: <20241030-borrow-mut-v1-0-8f0ceaf78eaf@gmail.com> To: Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Alice Ryhl , Trevor Gross , Danilo Krummrich Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, Tamir Duberstein X-Mailer: b4 0.15-dev Replace `as` casts with `cast{,_const,_mut}` which are a bit safer. Reduce the scope of unsafe blocks and add missing safety comments where an unsafe block has been split into several unsafe blocks. Signed-off-by: Tamir Duberstein --- rust/kernel/alloc/kbox.rs | 32 +++++++++++++++---------- rust/kernel/sync/arc.rs | 59 +++++++++++++++++++++++++++++--------------= ---- rust/kernel/types.rs | 5 ++-- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs index d69c32496b86a2315f81cafc8e6771ebb0cf10d1..7a5fdf7b660fb91ca2a8e5023d6= 9d629b0d66062 100644 --- a/rust/kernel/alloc/kbox.rs +++ b/rust/kernel/alloc/kbox.rs @@ -182,12 +182,12 @@ impl Box, A> /// /// Callers must ensure that the value inside of `b` is in an initiali= zed state. pub unsafe fn assume_init(self) -> Box { - let raw =3D Self::into_raw(self); + let raw =3D Self::into_raw(self).cast(); =20 // SAFETY: `raw` comes from a previous call to `Box::into_raw`. By= the safety requirements // of this function, the value inside the `Box` is in an initializ= ed state. Hence, it is // safe to reconstruct the `Box` as `Box`. - unsafe { Box::from_raw(raw.cast()) } + unsafe { Box::from_raw(raw) } } =20 /// Writes the value and converts to `Box`. @@ -247,10 +247,10 @@ pub fn pin(x: T, flags: Flags) -> Result>, AllocError> =20 /// Forgets the contents (does not run the destructor), but keeps the = allocation. fn forget_contents(this: Self) -> Box, A> { - let ptr =3D Self::into_raw(this); + let ptr =3D Self::into_raw(this).cast(); =20 // SAFETY: `ptr` is valid, because it came from `Box::into_raw`. - unsafe { Box::from_raw(ptr.cast()) } + unsafe { Box::from_raw(ptr) } } =20 /// Drops the contents, but keeps the allocation. @@ -356,19 +356,21 @@ impl ForeignOwnable for Box type Borrowed<'a> =3D &'a T; =20 fn into_foreign(self) -> *const core::ffi::c_void { - Box::into_raw(self) as _ + Box::into_raw(self).cast_const().cast() } =20 unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { + let ptr =3D ptr.cast_mut().cast(); // SAFETY: The safety requirements of this function ensure that `p= tr` comes from a previous // call to `Self::into_foreign`. - unsafe { Box::from_raw(ptr as _) } + unsafe { Box::from_raw(ptr) } } =20 unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T { + let ptr =3D ptr.cast(); // SAFETY: The safety requirements of this method ensure that the = object remains alive and // immutable for the duration of 'a. - unsafe { &*ptr.cast() } + unsafe { &*ptr } } } =20 @@ -380,21 +382,25 @@ impl ForeignOwnable for Pin> =20 fn into_foreign(self) -> *const core::ffi::c_void { // SAFETY: We are still treating the box as pinned. - Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) as _ + Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) + .cast_const() + .cast() } =20 unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { + let ptr =3D ptr.cast_mut().cast(); // SAFETY: The safety requirements of this function ensure that `p= tr` comes from a previous // call to `Self::into_foreign`. - unsafe { Pin::new_unchecked(Box::from_raw(ptr as _)) } + unsafe { Pin::new_unchecked(Box::from_raw(ptr)) } } =20 unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Pin<&'a T> { + let ptr =3D ptr.cast(); // SAFETY: The safety requirements for this function ensure that t= he object is still alive, // so it is safe to dereference the raw pointer. // The safety requirements of `from_foreign` also ensure that the = object remains alive for // the lifetime of the returned value. - let r =3D unsafe { &*ptr.cast() }; + let r =3D unsafe { &*ptr }; =20 // SAFETY: This pointer originates from a `Pin>`. unsafe { Pin::new_unchecked(r) } @@ -445,12 +451,14 @@ impl Drop for Box fn drop(&mut self) { let layout =3D Layout::for_value::(self); =20 + let ptr =3D self.0.as_ptr(); // SAFETY: The pointer in `self.0` is guaranteed to be valid by th= e type invariant. - unsafe { core::ptr::drop_in_place::(self.deref_mut()) }; + unsafe { core::ptr::drop_in_place(ptr) }; =20 + let addr =3D self.0.cast(); // SAFETY: // - `self.0` was previously allocated with `A`. // - `layout` is equal to the `Layout=C2=B4 `self.0` was allocated= with. - unsafe { A::free(self.0.cast(), layout) }; + unsafe { A::free(addr, layout) }; } } diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 4857230bd8d410bcca97b2081c3ce2f617ee7921..88e5208369e40bdeebc6f758e89= f836a97790d89 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -148,9 +148,10 @@ unsafe fn container_of(ptr: *const T) -> NonNull> { let refcount_layout =3D Layout::new::(); // SAFETY: The caller guarantees that the pointer is valid. let val_layout =3D Layout::for_value(unsafe { &*ptr }); + let val_offset =3D refcount_layout.extend(val_layout); // SAFETY: We're computing the layout of a real struct that existe= d when compiling this // binary, so its layout is not so large that it can trigger arith= metic overflow. - let val_offset =3D unsafe { refcount_layout.extend(val_layout).unw= rap_unchecked().1 }; + let (_, val_offset) =3D unsafe { val_offset.unwrap_unchecked() }; =20 // Pointer casts leave the metadata unchanged. This is okay becaus= e the metadata of `T` and // `ArcInner` is the same since `ArcInner` is a struct with `T`= as its last field. @@ -164,9 +165,10 @@ unsafe fn container_of(ptr: *const T) -> NonNull> { // still valid. let ptr =3D unsafe { ptr.byte_sub(val_offset) }; =20 + let ptr =3D ptr.cast_mut(); // SAFETY: The pointer can't be null since you can't have an `ArcI= nner` value at the null // address. - unsafe { NonNull::new_unchecked(ptr.cast_mut()) } + unsafe { NonNull::new_unchecked(ptr) } } } =20 @@ -201,10 +203,11 @@ pub fn new(contents: T, flags: Flags) -> Result { }; =20 let inner =3D KBox::new(value, flags)?; + let inner =3D KBox::leak(inner).into(); =20 // SAFETY: We just created `inner` with a reference count of 1, wh= ich is owned by the new // `Arc` object. - Ok(unsafe { Self::from_inner(KBox::leak(inner).into()) }) + Ok(unsafe { Self::from_inner(inner) }) } } =20 @@ -333,13 +336,15 @@ impl ForeignOwnable for Arc { type Borrowed<'a> =3D ArcBorrow<'a, T>; =20 fn into_foreign(self) -> *const core::ffi::c_void { - ManuallyDrop::new(self).ptr.as_ptr() as _ + ManuallyDrop::new(self).ptr.as_ptr().cast_const().cast() } =20 unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T= > { + let ptr =3D ptr.cast_mut().cast(); + // SAFETY: The safety requirements of this function ensure that `p= tr` comes from a previous // call to `Self::into_foreign`. - let inner =3D unsafe { NonNull::new_unchecked(ptr as _) }; + let inner =3D unsafe { NonNull::new_unchecked(ptr) }; =20 // SAFETY: The safety requirements of `from_foreign` ensure that t= he object remains alive // for the lifetime of the returned value. @@ -347,9 +352,11 @@ unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) ->= ArcBorrow<'a, T> { } =20 unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { + let ptr =3D ptr.cast_mut().cast(); + // SAFETY: The safety requirements of this function ensure that `p= tr` comes from a previous // call to `Self::into_foreign`. - let inner =3D unsafe { NonNull::new_unchecked(ptr as _) }; + let inner =3D unsafe { NonNull::new_unchecked(ptr) }; =20 // SAFETY: By the safety requirement of this function, we know tha= t `ptr` came from // a previous call to `Arc::into_foreign`, which guarantees that `= ptr` is valid and @@ -376,10 +383,14 @@ fn as_ref(&self) -> &T { =20 impl Clone for Arc { fn clone(&self) -> Self { + // SAFETY: By the type invariant, there is necessarily a reference= to the object, so it is + // safe to dereference it. + let refcount =3D unsafe { self.ptr.as_ref() }.refcount.get(); + // INVARIANT: C `refcount_inc` saturates the refcount, so it canno= t overflow to zero. // SAFETY: By the type invariant, there is necessarily a reference= to the object, so it is // safe to increment the refcount. - unsafe { bindings::refcount_inc(self.ptr.as_ref().refcount.get()) = }; + unsafe { bindings::refcount_inc(refcount) }; =20 // SAFETY: We just incremented the refcount. This increment is now= owned by the new `Arc`. unsafe { Self::from_inner(self.ptr) } @@ -399,10 +410,11 @@ fn drop(&mut self) { // SAFETY: Also by the type invariant, we are allowed to decrement= the refcount. let is_zero =3D unsafe { bindings::refcount_dec_and_test(refcount)= }; if is_zero { + let ptr =3D self.ptr.as_ptr(); // The count reached zero, we must free the memory. // // SAFETY: The pointer was initialised from the result of `KBo= x::leak`. - unsafe { drop(KBox::from_raw(self.ptr.as_ptr())) }; + unsafe { drop(KBox::from_raw(ptr)) }; } } } @@ -550,7 +562,7 @@ impl Deref for ArcBorrow<'_, T> { fn deref(&self) -> &Self::Target { // SAFETY: By the type invariant, the underlying object is still a= live with no mutable // references to it, so it is safe to create a shared reference. - unsafe { &self.inner.as_ref().data } + &unsafe { self.inner.as_ref() }.data } } =20 @@ -652,11 +664,11 @@ pub fn new_uninit(flags: Flags) -> Result>, AllocError> }? AllocError), flags, )?; - Ok(UniqueArc { - // INVARIANT: The newly-created object has a refcount of 1. - // SAFETY: The pointer from the `KBox` is valid. - inner: unsafe { Arc::from_inner(KBox::leak(inner).into()) }, - }) + let inner =3D KBox::leak(inner).into(); + // INVARIANT: The newly-created object has a refcount of 1. + // SAFETY: The pointer from the `KBox` is valid. + let inner =3D unsafe { Arc::from_inner(inner) }; + Ok(UniqueArc { inner }) } } =20 @@ -675,18 +687,18 @@ pub fn write(mut self, value: T) -> UniqueArc { /// The caller guarantees that the value behind this pointer has been = initialized. It is /// *immediate* UB to call this when the value is not initialized. pub unsafe fn assume_init(self) -> UniqueArc { - let inner =3D ManuallyDrop::new(self).inner.ptr; - UniqueArc { - // SAFETY: The new `Arc` is taking over `ptr` from `self.inner= ` (which won't be - // dropped). The types are compatible because `MaybeUninit`= is compatible with `T`. - inner: unsafe { Arc::from_inner(inner.cast()) }, - } + let inner =3D ManuallyDrop::new(self).inner.ptr.cast(); + // SAFETY: The new `Arc` is taking over `ptr` from `self.inner` (w= hich won't be + // dropped). The types are compatible because `MaybeUninit` is = compatible with `T`. + let inner =3D unsafe { Arc::from_inner(inner) }; + UniqueArc { inner } } =20 /// Initialize `self` using the given initializer. pub fn init_with(mut self, init: impl Init) -> core::result::= Result, E> { + let ptr =3D self.as_mut_ptr(); // SAFETY: The supplied pointer is valid for initialization. - match unsafe { init.__init(self.as_mut_ptr()) } { + match unsafe { init.__init(ptr) } { // SAFETY: Initialization completed successfully. Ok(()) =3D> Ok(unsafe { self.assume_init() }), Err(err) =3D> Err(err), @@ -698,9 +710,10 @@ pub fn pin_init_with( mut self, init: impl PinInit, ) -> core::result::Result>, E> { + let ptr =3D self.as_mut_ptr(); // SAFETY: The supplied pointer is valid for initialization and we= will later pin the value // to ensure it does not move. - match unsafe { init.__pinned_init(self.as_mut_ptr()) } { + match unsafe { init.__pinned_init(ptr) } { // SAFETY: Initialization completed successfully. Ok(()) =3D> Ok(unsafe { self.assume_init() }.into()), Err(err) =3D> Err(err), @@ -729,7 +742,7 @@ fn deref_mut(&mut self) -> &mut Self::Target { // SAFETY: By the `Arc` type invariant, there is necessarily a ref= erence to the object, so // it is safe to dereference it. Additionally, we know there is on= ly one reference when // it's inside a `UniqueArc`, so it is safe to get a mutable refer= ence. - unsafe { &mut self.inner.ptr.as_mut().data } + &mut unsafe { self.inner.ptr.as_mut() }.data } } =20 diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index fae80814fa1c5e0f11933f2f15e173f0e3a10fe0..e8b7ff1387381e50d7963978e57= b1d567113b035 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -418,7 +418,7 @@ pub unsafe fn from_raw(ptr: NonNull) -> Self { /// } /// /// let mut data =3D Empty {}; - /// let ptr =3D NonNull::::new(&mut data as *mut _).unwrap(); + /// let ptr =3D NonNull::new(&mut data).unwrap(); /// # // SAFETY: TODO. /// let data_ref: ARef =3D unsafe { ARef::from_raw(ptr) }; /// let raw_ptr: NonNull =3D ARef::into_raw(data_ref); @@ -450,8 +450,9 @@ fn deref(&self) -> &Self::Target { impl From<&T> for ARef { fn from(b: &T) -> Self { b.inc_ref(); + let b =3D b.into(); // SAFETY: We just incremented the refcount above. - unsafe { Self::from_raw(NonNull::from(b)) } + unsafe { Self::from_raw(b) } } } =20 --=20 2.47.0