Re: [PATCH] rust: sync: add #[must_use] to Lock::try_lock

[Boqun Feng <boqun.feng@gmail.com>]

On Sun, Dec 08, 2024 at 11:40:04AM -0800, jos wrote:
> Hi Boqun,
> 
> Thank you for the feedback, I am new to kernel development and appreciate
> the response.
> 
> To answer your question(s):
> 
> > I don't think not having `#[must_use]` will result in the locking being
> > unlocked immediately, do you have an example?
> 
> You're correct that the absence of `#[must_use]` itself does not directly
> result in the lock being unlocked immediately, my mistake.
> 
> How I understand the issue arises when the result of `Lock::try_lock` is
> called and its return value can be ignored, the lock is never acquired, and
> no guard is created. Making the caller assume the lock was successfully
> acquired and proceeding with operations as if the data is safely protected,
> e.g. calling `lock.try_lock()` and proceeding with critical section code.
> 

Well, in most cases, locking is data-oriented, i.e. you have to have the
`Guard` to access locked data, so the "issue" you mention could not
happen. Besides, it's relatively easy to "get around" `#[must_use]` by
doing:

	let _ = l.try_lock()?;

Overall, `#[must_use]` here should be the tool to avoid some code that
obviously doesn't make sense (like acquring and releasing lock without
any code in between), rather than silver bullets for a serious safety
issue. So I think you should reword the commit log to reflect this.

> The use of the `#[must_use]` macro annotation will ensure the caller cannot
> silently ignore its return value. I believe the annotation can be revised
> to:
> `#[must_use = "if unused, return value may be unhandled, leading to
> potential race conditions"]` ?
> 

I would just say "if unused, leading to an empty critical section", i.e.
it's bad but not a correct-or-wrong issue. Does this make sense?

Regards,
Boqun

> But would love to hear your thoughts?
> 
> Best,
> Jason
> 
> On Sun, Dec 8, 2024 at 11:11 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > Hi Jason,
> >
> > Thanks for the patch.
> >
> > On Sun, Dec 08, 2024 at 01:48:34PM -0500, Jason Devers wrote:
> > > The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> > > currently does not issue a warning if the return value is unused. This
> > > could result in the lock being unlocked immediately, which is unsafe
> >
> > I don't think not having `#[must_use]` will result in the locking being
> > unlocked immediately, do you have an example?
> >
> > > and unintended. This patch adds a `#[must_use]` annotation to
> >
> > Are you sure this is unsafe? Again do you have an example showing this?
> >
> > Regards,
> > Boqun
> >
> > > `Lock::try_lock` to prevent this.
> > >
> > > Suggested-by: Alice Ryhl <alice@ryhl.io>
> > > Link: https://github.com/Rust-for-Linux/linux/issues/1133
> > > Signed-off-by: Jason Devers <dev.json2@gmail.com>
> > > ---
> > >  rust/kernel/sync/lock.rs | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > > index 41dcddac69e2..f6a5e83ff685 100644
> > > --- a/rust/kernel/sync/lock.rs
> > > +++ b/rust/kernel/sync/lock.rs
> > > @@ -147,6 +147,7 @@ pub fn lock(&self) -> Guard<'_, T, B> {
> > >      /// Tries to acquire the lock.
> > >      ///
> > >      /// Returns a guard that can be used to access the data protected
> > by the lock if successful.
> > > +    #[must_use = "if unused, the lock will be immediately unlocked"]
> > >      pub fn try_lock(&self) -> Option<Guard<'_, T, B>> {
> > >          // SAFETY: The constructor of the type calls `init`, so the
> > existence of the object proves
> > >          // that `init` was called.
> > > --
> > > 2.44.2
> > >
> >