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

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

[Jason Devers]

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
and unintended. This patch adds a `#[must_use]` annotation to
`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

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

[Boqun Feng]

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
> 

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

[jos]

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.

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"]` ?

But would love to hear your thoughts and suggestions!

Best,
Jason


On Sun, Dec 8, 2024 at 11:40 AM jos <dev.json2@gmail.com> 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.
>
> 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"]` ?
>
> 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
>> >

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

[Boqun Feng]

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
> > >
> >

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

[Jason Devers]

The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

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..f7d355c5d90a 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, leading to an empty critical section"]
     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

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

[Tamir Duberstein]

On Sun, Dec 8, 2024 at 3:50 PM Jason Devers <dev.json2@gmail.com> wrote:
>
> The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> currently does not issue a warning if the return value is unused.
> To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

It'd be great to mention in this message (and perhaps in the code)
that the reason this annotation is necessary is that `Option<T>` is
not `#[must_use]` regardless of `T` being `#[must_use]`. It might also
be handy to leave a breadcrumb pointing to
https://github.com/rust-lang/rust/issues/71368.

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

[jos]

> I may be misunderstanding what you are trying to say, but the guard is
> created, and the lock is acquired.

Apologizes, I had a misunderstanding of the macro that was being used.
Thank you Boqun and Miguel for providing clarity, much appreciated!

> It'd be great to mention in this message (and perhaps in the code)
> that the reason this annotation is necessary is that `Option<T>` is
> not `#[must_use]` regardless of `T` being `#[must_use]`. It might also
> be handy to leave a breadcrumb pointing to
> https://github.com/rust-lang/rust/issues/71368.

Sure I can do that, thank you for the suggestion!

Thanks,
Jason


On Sun, Dec 8, 2024 at 1:58 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sun, Dec 8, 2024 at 3:50 PM Jason Devers <dev.json2@gmail.com> wrote:
> >
> > The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> > currently does not issue a warning if the return value is unused.
> > To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.
>
> It'd be great to mention in this message (and perhaps in the code)
> that the reason this annotation is necessary is that `Option<T>` is
> not `#[must_use]` regardless of `T` being `#[must_use]`. It might also
> be handy to leave a breadcrumb pointing to
> https://github.com/rust-lang/rust/issues/71368.

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

[Jason Devers]

The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: https://github.com/rust-lang/rust/issues/71368.

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..f7d355c5d90a 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, leading to an empty critical section"]
     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

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

[Miguel Ojeda]

On Sun, Dec 8, 2024 at 11:26 PM Jason Devers <dev.json2@gmail.com> wrote:
>
> The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> currently does not issue a warning if the return value is unused.
> To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.
>
> Note that `T` is `#[must_use]` but `Option<T>` is not.
> For more context, see: https://github.com/rust-lang/rust/issues/71368.
>
> 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>

Please use the version option of `format-patch`, e.g. `-v4`, when you
send new versions, so that they are tagged as such. Otherwise, all the
patches look like they are the first version, which is confusing for
tooling (and people :)

Also, in general, it is best to wait a day or two to give people a
chance to review your patches, so that you do not need to accumulate
too many versions.

> +    #[must_use = "if unused, leading to an empty critical section"]

I am not a native speaker, but this sounds a bit strange. Is it
written like this due to how it gets "rendered" by the compiler later
on?

Thanks!

Cheers,
Miguel

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

[jos]

> I use aliceryhl@google.com for kernel contributions.

Sorry about this, I will update with the correct email!

> Please use the version option of `format-patch`, e.g. `-v4`, when you
> send new versions, so that they are tagged as such. Otherwise, all the
> patches look like they are the first version, which is confusing for
> tooling (and people :)

> Also, in general, it is best to wait a day or two to give people a
> chance to review your patches, so that you do not need to accumulate
> too many versions.

Will do! I really appreciate the patience and feedback, please
continue doing so :)

> +    #[must_use = "if unused, leading to an empty critical section"]

> I am not a native speaker, but this sounds a bit strange. Is it
> written like this due to how it gets "rendered" by the compiler later
> on?

I agree this sounds a bit strange, perhaps it would sound more proper to say:

`#[must_use = "if unused, leads to an empty critical section"]`

Will give it some time to receive more thoughts before sending an
updated version.

Thanks,
Jason


On Mon, Dec 9, 2024 at 3:58 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sun, Dec 8, 2024 at 11:26 PM Jason Devers <dev.json2@gmail.com> wrote:
> >
> > The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> > currently does not issue a warning if the return value is unused.
> > To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.
> >
> > Note that `T` is `#[must_use]` but `Option<T>` is not.
> > For more context, see: https://github.com/rust-lang/rust/issues/71368.
> >
> > 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>
>
> Please use the version option of `format-patch`, e.g. `-v4`, when you
> send new versions, so that they are tagged as such. Otherwise, all the
> patches look like they are the first version, which is confusing for
> tooling (and people :)
>
> Also, in general, it is best to wait a day or two to give people a
> chance to review your patches, so that you do not need to accumulate
> too many versions.
>
> > +    #[must_use = "if unused, leading to an empty critical section"]
>
> I am not a native speaker, but this sounds a bit strange. Is it
> written like this due to how it gets "rendered" by the compiler later
> on?
>
> Thanks!
>
> Cheers,
> Miguel

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

[Jason Devers]

The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: https://github.com/rust-lang/rust/issues/71368.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://github.com/Rust-for-Linux/linux/issues/1133
Signed-off-by: Jason Devers <dev.json2@gmail.com>
---
 rust/kernel/sync/lock.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 41dcddac69e2..39f8c07d2287 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -147,6 +147,8 @@ 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.
+    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
+    #[must_use = "if unused, leads to an empty critical section"]
     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

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

[Alice Ryhl]

On Wed, Dec 11, 2024 at 3:05 AM Jason Devers <dev.json2@gmail.com> wrote:
>
> The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> currently does not issue a warning if the return value is unused.
> To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.
>
> Note that `T` is `#[must_use]` but `Option<T>` is not.
> For more context, see: https://github.com/rust-lang/rust/issues/71368.
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/1133
> Signed-off-by: Jason Devers <dev.json2@gmail.com>
> ---
>  rust/kernel/sync/lock.rs | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 41dcddac69e2..39f8c07d2287 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -147,6 +147,8 @@ 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.
> +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
> +    #[must_use = "if unused, leads to an empty critical section"]

Can't we use the same warning message as Guard?

#[must_use = "the lock unlocks immediately when the guard is unused"]

Alice

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

[Miguel Ojeda]

On Wed, Dec 11, 2024 at 3:05 AM Jason Devers <dev.json2@gmail.com> wrote:
>
> +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.

Should this be a comment, i.e. `//`, rather than part of the documentation?

I would also reword to explain the "what", not just "why", e.g. something like:

    // `Option<T>` is not `#[must_use]` even if `T` is, thus the
attribute is needed here.

Cheers,
Miguel

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

[jos]

> Can't we use the same warning message as Guard?

> #[must_use = "the lock unlocks immediately when the guard is unused"]

That is what I originally had in my first patch and how I understood
the annotation should be phrased, but @Boqun Feng maybe you can chime
in with your input?

>> +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
>
> Should this be a comment, i.e. `//`, rather than part of the documentation?

Agreed, after reading more documentation conventions
(https://github.com/rust-lang/rfcs/blob/master/text/1574-more-api-documentation-conventions.md),
this should not be part of the rustdocs.

> I would also reword to explain the "what", not just "why", e.g. something like:
>
>    // `Option<T>` is not `#[must_use]` even if `T` is, thus the
> attribute is needed here.

Thank you for the insight, I can see how there is missing clarity in
the current comment, will update in the next patch.

Thanks,
Jason


On Wed, Dec 11, 2024 at 1:45 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Dec 11, 2024 at 3:05 AM Jason Devers <dev.json2@gmail.com> wrote:
> >
> > +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
>
> Should this be a comment, i.e. `//`, rather than part of the documentation?
>
> I would also reword to explain the "what", not just "why", e.g. something like:
>
>     // `Option<T>` is not `#[must_use]` even if `T` is, thus the
> attribute is needed here.
>
> Cheers,
> Miguel

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

[Boqun Feng]

On Wed, Dec 11, 2024 at 09:05:32AM -0800, jos wrote:
> > Can't we use the same warning message as Guard?
> 
> > #[must_use = "the lock unlocks immediately when the guard is unused"]
> 
> That is what I originally had in my first patch and how I understood
> the annotation should be phrased, but @Boqun Feng maybe you can chime
> in with your input?
> 

Hmm... I think it was a mis-communication. In the v1, you said "..but it
currently does not issue a warning if the return value is unused. This
could result in the lock being unlocked immediately.." I thought "This"
means "not having `#[must_use]`", but you probably meant "not using the
value"? My comment is more to that commit log instead of the warning
message in the `#[must_use = ...]` block.

Anyway, I think the warning message in your v1 works for me. Again, to
be clear, my concern was on the commit log then. Hope it clarifies
things.

Regards,
Boqun

> >> +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
> >
> > Should this be a comment, i.e. `//`, rather than part of the documentation?
> 
> Agreed, after reading more documentation conventions
> (https://github.com/rust-lang/rfcs/blob/master/text/1574-more-api-documentation-conventions.md),
> this should not be part of the rustdocs.
> 
> > I would also reword to explain the "what", not just "why", e.g. something like:
> >
> >    // `Option<T>` is not `#[must_use]` even if `T` is, thus the
> > attribute is needed here.
> 
> Thank you for the insight, I can see how there is missing clarity in
> the current comment, will update in the next patch.
> 
> Thanks,
> Jason
> 
> 
> On Wed, Dec 11, 2024 at 1:45 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Wed, Dec 11, 2024 at 3:05 AM Jason Devers <dev.json2@gmail.com> wrote:
> > >
> > > +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
> >
> > Should this be a comment, i.e. `//`, rather than part of the documentation?
> >
> > I would also reword to explain the "what", not just "why", e.g. something like:
> >
> >     // `Option<T>` is not `#[must_use]` even if `T` is, thus the
> > attribute is needed here.
> >
> > Cheers,
> > Miguel

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

[Jason Devers]

The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: https://github.com/rust-lang/rust/issues/71368.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://github.com/Rust-for-Linux/linux/issues/1133
Signed-off-by: Jason Devers <dev.json2@gmail.com>
---
 rust/kernel/sync/lock.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 41dcddac69e2..dd187812aeb4 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -147,6 +147,8 @@ 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.
+    // Note that `T` is `#[must_use]` but `Option<T>` is not.
+    #[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

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

[Miguel Ojeda]

On Thu, Dec 12, 2024 at 4:11 AM Jason Devers <dev.json2@gmail.com> wrote:
>
> +    // Note that `T` is `#[must_use]` but `Option<T>` is not.

I think the wording of the comment was not updated? If there is a
reason for not doing it, then you should normally put it in the
changelog (i.e. after the first `---` line) or you would reply to the
original feedback with the reason.

Thanks!

Cheers,
Miguel

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

[Jason Devers]

The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
currently does not issue a warning if the return value is unused.
To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.

Note that `T` is `#[must_use]` but `Option<T>` is not.
For more context, see: https://github.com/rust-lang/rust/issues/71368.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://github.com/Rust-for-Linux/linux/issues/1133
Signed-off-by: Jason Devers <dev.json2@gmail.com>
---
 rust/kernel/sync/lock.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 41dcddac69e2..aacc227536cb 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -147,6 +147,8 @@ 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.
+    // `Option<T>` is not `#[must_use]` even if `T` is, thus the attribute is needed here.
+    #[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

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

[Boqun Feng]

Hi Jason,

On Thu, Dec 12, 2024 at 10:47:53AM -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.
> To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.
> 
> Note that `T` is `#[must_use]` but `Option<T>` is not.
> For more context, see: https://github.com/rust-lang/rust/issues/71368.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/1133

I think you could add Alice's Reviewed-by from the previous version:

	https://lore.kernel.org/rust-for-linux/CAH5fLghjR4fctjoLT9Zom3drNMQ6vdE9hUTnZZ4Ms1qQHGFe=A@mail.gmail.com/

or you have any reason that you need another "Reviewed-by" tag?

(No need to send a new version, I'm just making sure you and Alice both
agree the "Reviewed-by" tag still applies to this patch.)

Regards,
Boqun

> Signed-off-by: Jason Devers <dev.json2@gmail.com>
> ---
>  rust/kernel/sync/lock.rs | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 41dcddac69e2..aacc227536cb 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -147,6 +147,8 @@ 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.
> +    // `Option<T>` is not `#[must_use]` even if `T` is, thus the attribute is needed here.
> +    #[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
> 

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

[Alice Ryhl]

On Thu, Dec 12, 2024 at 4:48 PM Jason Devers <dev.json2@gmail.com> wrote:
>
> The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> currently does not issue a warning if the return value is unused.
> To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.
>
> Note that `T` is `#[must_use]` but `Option<T>` is not.
> For more context, see: https://github.com/rust-lang/rust/issues/71368.
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/1133
> Signed-off-by: Jason Devers <dev.json2@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

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

[Boqun Feng]

On Thu, Jan 09, 2025 at 02:05:41PM +0100, Alice Ryhl wrote:
> On Thu, Dec 12, 2024 at 4:48 PM Jason Devers <dev.json2@gmail.com> wrote:
> >
> > The `Lock::try_lock` function returns an `Option<Guard<...>>`, but it
> > currently does not issue a warning if the return value is unused.
> > To avoid potential bugs, the `#[must_use]` annotation is added to ensure proper usage.
> >
> > Note that `T` is `#[must_use]` but `Option<T>` is not.
> > For more context, see: https://github.com/rust-lang/rust/issues/71368.
> >
> > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > Link: https://github.com/Rust-for-Linux/linux/issues/1133
> > Signed-off-by: Jason Devers <dev.json2@gmail.com>
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Queued. I reworded a bit on the commit log because of the tip maintainer
guideline. Thank you both!

Regards,
Boqun

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

[Miguel Ojeda]

On Sun, Dec 8, 2024 at 8:40 PM jos <dev.json2@gmail.com> wrote:
>
> 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.

I may be misunderstanding what you are trying to say, but the guard is
created, and the lock is acquired.

So the issue is that someone does not keep the guard around, so the
guard is dropped right away and so the critical section is empty.

Cheers,
Miguel

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

[Alice Ryhl]

On Sun, Dec 8, 2024 at 7:49 PM Jason Devers <dev.json2@gmail.com> 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
> and unintended. This patch adds a `#[must_use]` annotation to
> `Lock::try_lock` to prevent this.

I agree that "unsafe" is the wrong word here. We can just say "which
is probably not what the user intended".

> Suggested-by: Alice Ryhl <alice@ryhl.io>

I use aliceryhl@google.com for kernel contributions.

> Link: https://github.com/Rust-for-Linux/linux/issues/1133
> Signed-off-by: Jason Devers <dev.json2@gmail.com>

Overall looks good to me! You may add:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>