Date:   Thu, 2 Feb 2023 14:24:21 +0000
From:   Gary Guo <gary@garyguo.net>
To:     Boqun Feng <boqun.feng@gmail.com>
Cc:     rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
        Will Deacon <will@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        =?UTF-8?B?QmrDtnJu?= Roy Baron <bjorn3_gh@protonmail.com>,
        Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Subject: Re: [RFC 3/5] rust: sync: Arc: Introduces Arc::get_inner() helper
Message-ID: <20230202142421.7ab724dc.gary@garyguo.net>
In-Reply-To: <20230201232244.212908-4-boqun.feng@gmail.com>
References: <20230201232244.212908-1-boqun.feng@gmail.com>
        <20230201232244.212908-4-boqun.feng@gmail.com>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1/EfPZFtswdk7b17Zubp4jwhaNIfpJRm9w5f7En7EGhOfE/JShDqlowmTArI?=
 =?us-ascii?Q?iGpyjHGg3+Mvx7xmNvdrst2iZ4uZwM8b+MsY2iR15n/WQISYrERU1aGgy7BJ?=
 =?us-ascii?Q?UffKzU5pYcUP6Qco26LOqXkhhA6ci5JFY5hzLOaH9S3LaEByKmisorhrMn8+?=
 =?us-ascii?Q?pa89+ZkaWdXJRTFu8LXuqXKbB/nuiOuD8I1PI+bH2s9roIJYu6HlBXeW2ZMU?=
 =?us-ascii?Q?VBaw7nuCQ/LrfmEb6YD28xJ9he2Ai+tF8Oa/fPDA6D5sNznnEbJjglHobzPj?=
 =?us-ascii?Q?vhDw/fCnBlIFxMhQIzhZVTdAkq9x37Yi/8QfYxmGxohm5qj8855XnbaEtYke?=
 =?us-ascii?Q?N/t9PFinGyB4+tN+5yU2VL9076qrS8XJs8lmjvSQIz/wVnFu4YeJFGiUxFtv?=
 =?us-ascii?Q?1V3htfl4XGnSQ9S+UBBx6i1xiBMUDxCZWANeYsIl/jXoMwuZnXbzappuAOmB?=
 =?us-ascii?Q?w3EiEeiAVI9uGemmNvMosCEQ7z9v5I2yAkdgmdmGdZ92pJDDXBvHcPPod022?=
 =?us-ascii?Q?TGu2/bwyAnGcwBFf00l15L5k0Gz3GKPq91q2lQvyXpPOJw+15TyI1dFKj80T?=
 =?us-ascii?Q?i6aMZrbCRLRp2orvMTLeWKuLrZYkte5fum0zf+S5zifk/GOIc8opAePXWNX+?=
 =?us-ascii?Q?LleDoFi19m57vutYL9YT0CZK/EYvr0SPA78FYwcLx5pbKXdOsnD+7BhRN3dI?=
 =?us-ascii?Q?8Tr/aG7A7Pkxukl4w2VljwZ73UowG4jazlszHvwKP9tbFcUlI9BCvU2o/nXO?=
 =?us-ascii?Q?l/dDlhlC9oSq4/pIVhEh3Izsu+YCEyZ8yz+/v8OXPTreS5YHbTSEgOoHULO5?=
 =?us-ascii?Q?hRdL65MyVvJqtmEnXfxBXVvFRcvzoiB3gImtZEAdBmeXWl0duYL0d1HrVMWY?=
 =?us-ascii?Q?ZFbYbgDWa0/7Ol1GJATaeqYlkOIhpxOFpHc+iba4xDBLngXBoT0jvC6jxMHJ?=
 =?us-ascii?Q?+RZOYKBWCoLYS4cFdywY4IiozD8fhpsAAUxIbTNdX3UCxTo+vHB6Yo/byHP5?=
 =?us-ascii?Q?/kyjeSUAyydjIoGxVsSpqr8EVyCf9p8LA5YwA7Nxt2P07LDGKAKo97luZXic?=
 =?us-ascii?Q?iFmYBw1V+LSSckB49MwK++VczN0MeKak7pNlAnfSrX4TYyQyfoGfLW8UukE9?=
 =?us-ascii?Q?Fs4sxmqzXyseAadwqQJsJjgyyUCIN7+sZ/GB7K+z/DYd60mh+5Tk+JZMoSKF?=
 =?us-ascii?Q?jUuLK2tdlebythg2pcSNZVQLzxWnXfKuDSpqU3hPdGkMkTRIifU3QYl6ZlXC?=
 =?us-ascii?Q?UCN5b4j4/6dPcr+y3Fd+LHaHtrov+6+sVBslGAdSNjFigsOeb2C5pSUmX9ZG?=
 =?us-ascii?Q?0FB1RmjOOrOBhwdCKEpBQ3CiMt2l5jE7GJ+490jURbp06+I+DauCp4SvjoNP?=
 =?us-ascii?Q?/NoujmJpw5IF2xQvrgnlTfcS3ck15qprLgpm6pjvzaFVVwbuROrkLaiuIaPL?=
 =?us-ascii?Q?KYE27pgOn6er0gqbatLVvs503og8tO5yaCdhkZsqxk61yZbuuGBVRPKlsRDm?=
 =?us-ascii?Q?PP7kDHEE1bHSxL39Jh+x+RVqmS48E4YVnPhoucdBLZrpVuPfJk9bw4H1DAux?=
 =?us-ascii?Q?padk+qomwqpp5jEgjaJY1RQ5O5lVbgHmvlMoXy+E?=
X-OriginatorOrg: garyguo.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 12b6e48f-44ea-4c60-23e2-08db05292e01
X-MS-Exchange-CrossTenant-AuthSource: LO2P265MB5183.GBRP265.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2023 14:24:22.7619
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bbc898ad-b10f-4e10-8552-d9377b823d45
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8l9TfancJghKHTI0us81qRi19haK0BZXSzENbMGfatClSdhtogHCPrZ8bSvAbyKhJqKcvspGRFPXlFYVWrBpEg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LO6P265MB6029
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed,  1 Feb 2023 15:22:42 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> Getting a `&ArcInner<T>` should always be safe from a `Arc<T>`,
> therefore add this helper function to avoid duplicate unsafe
> `ptr.as_ref()`.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/sync/arc.rs | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index fbfceaa3096e..4d8de20c996f 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -201,6 +201,13 @@ impl<T: ?Sized> Arc<T> {
>          // reference can be created.
>          unsafe { ArcBorrow::new(self.ptr) }
>      }
> +
> +    /// Returns a reference to the internal [`ArcInner`].
> +    fn get_inner(&self) -> &ArcInner<T> {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
>  }
>  
>  impl<T: 'static> ForeignOwnable for Arc<T> {
> @@ -233,9 +240,7 @@ impl<T: ?Sized> Deref for Arc<T> {
>      type Target = T;
>  
>      fn deref(&self) -> &Self::Target {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> -        // safe to dereference it.
> -        unsafe { &self.ptr.as_ref().data }
> +        &self.get_inner().data
>      }
>  }
>  
> @@ -244,7 +249,7 @@ impl<T: ?Sized> Clone for Arc<T> {
>          // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot 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(self.get_inner().refcount.get()) };
>  
>          // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>          unsafe { Self::from_inner(self.ptr) }
> @@ -253,11 +258,7 @@ impl<T: ?Sized> Clone for Arc<T> {
>  
>  impl<T: ?Sized> Drop for Arc<T> {
>      fn drop(&mut self) {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
> -        // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
> -        // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
> -        // freed/invalid memory as long as it is never dereferenced.
> -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> +        let refcount = self.get_inner().refcount.get();
>  
>          // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
>          // this instance is being dropped, so the broken invariant is not observable.