Date:   Thu, 2 Feb 2023 16:41:47 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Gary Guo <gary@garyguo.net>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        =?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
        Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Subject: Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()
Message-ID: <Y9vZu08L2WaLNJIc@kroah.com>
References: <20230201232244.212908-1-boqun.feng@gmail.com>
 <20230201232244.212908-3-boqun.feng@gmail.com>
 <Y9t+3u+2UcAFQc+r@hirez.programming.kicks-ass.net>
 <20230202142153.352ba479.gary@garyguo.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230202142153.352ba479.gary@garyguo.net>
Precedence: bulk

On Thu, Feb 02, 2023 at 02:21:53PM +0000, Gary Guo wrote:
> On Thu, 2 Feb 2023 10:14:06 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> > > This allows reading the current count of a refcount in an `ArcInner`.
> > > 
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > ---
> > >  rust/helpers.c          | 6 ++++++
> > >  rust/kernel/sync/arc.rs | 9 +++++++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > index 09a4d93f9d62..afc5f1a39fef 100644
> > > --- a/rust/helpers.c
> > > +++ b/rust/helpers.c
> > > @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> > >  }
> > >  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> > >  
> > > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > > +{
> > > +	return refcount_read(r);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> > > +
> > >  /*
> > >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> > >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > > index fc680a4a795c..fbfceaa3096e 100644
> > > --- a/rust/kernel/sync/arc.rs
> > > +++ b/rust/kernel/sync/arc.rs
> > > @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
> > >      data: T,
> > >  }
> > >  
> > > +impl<T: ?Sized> ArcInner<T> {
> > > +    /// Returns the current reference count of [`ArcInner`].
> > > +    fn count(&self) -> u32 {
> > > +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> > > +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> > > +        unsafe { bindings::refcount_read(self.refcount.get()) }
> > > +    }
> > > +}  
> > 
> > This is completely unsafe vs concurrency. In order to enable correct
> > tracing of refcount manipulations we have the __refcount_*(.oldp) API.
> 
> Retrieving the reference count is safe. It's just that in many
> scenarios it's very hard to use the retrieved reference count
> correctly, because it might be concurrently changed.

Yes, so you really should never ever ever care about the value, and that
includes printing it out as it will be wrong the instant you read it.

> But there are correct ways to use a refcount, e.g. if you own
> `Arc` and `.count()` returns 1, then you know that you are the
> exclusive owner of the `Arc` and nobody else is going to touch it.

But you should never know this, as it is not relevant.

So no, please don't allow printing out of a reference count, that will
only cause problems and allow people to think it is safe to do so.

Peter is right, please don't do this.

thanks,

greg k-h