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

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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

---
 rust/kernel/sync/lock.rs | 1 +
 1 file changed, 1 insertion(+)

rust/kernel/sync/lock.rs +1 -0

index 41dcddac69e2..f6a5e83ff685 100644
@@ -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.

Signature · 1 line

2.44.2

I don't think not having #[must_use] will result in the locking being unlocked immediately, do you have an example?

Hi Boqun,

Thank you for the feedback, I am new to kernel development and appreciate the response.

To answer your question(s):

Quoted 2 lines

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

Quoted 44 lines · Boqun Feng

Hi Jason,

Thanks for the patch.

Quoted 3 lines · 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

I don't think not having #[must_use] will result in the locking being unlocked immediately, do you have an example?

Quoted 1 line

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

Quoted 24 lines

Lock::try_lock to prevent this.

Suggested-by Alice Ryhl <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

---
 rust/kernel/sync/lock.rs | 1 +
 1 file changed, 1 insertion(+)

rust/kernel/sync/lock.rs +1 -1

index 41dcddac69e2..f6a5e83ff685 100644
@@ -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

Hi Boqun,

Thank you for the feedback, I am new to kernel development and appreciate the response.

To answer your question(s):

Quoted 2 lines

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

Quoted 46 lines · Boqun Feng

Hi Jason,

Thanks for the patch.

Quoted 3 lines · 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

I don't think not having #[must_use] will result in the locking being unlocked immediately, do you have an example?

Quoted 1 line

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

Quoted 14 lines

Lock::try_lock to prevent this.

Suggested-by Alice Ryhl <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

---
 rust/kernel/sync/lock.rs | 1 +
 1 file changed, 1 insertion(+)

rust/kernel/sync/lock.rs +0 -0

index 41dcddac69e2..f6a5e83ff685 100644

rust/kernel/sync/lock.rs pub fn lock(&self) -> Guard<'_, T, B> { · line 149

Quoted 3 lines

rust/kernel/sync/lock.rs +0 -0

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

rust/kernel/sync/lock.rs pub fn lock(&self) -> Guard<'_, T, B> {

Quoted 3 lines

rust/kernel/sync/lock.rs +1 -0

+    #[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

Quoted 4 lines

rust/kernel/sync/lock.rs +0 -1

         // that `init` was called.
--

2.44.2

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.

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

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.

Quoted 4 lines · 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.

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.

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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

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

I use [email protected] for kernel contributions.

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?

Quoted 11 lines · 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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

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.

Quoted 1 line

+ #[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

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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

---
 rust/kernel/sync/lock.rs | 2 ++
 1 file changed, 2 insertions(+)

rust/kernel/sync/lock.rs +0 -0

index 41dcddac69e2..39f8c07d2287 100644

+ /// Note that T is #[must_use] but Option<T> is not.

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

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

Quoted 1 line

+ /// 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.

Quoted 2 lines · Jason Devers

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

Quoted 3 lines

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?

Quoted 3 lines

Quoted 1 line

+ /// 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.

Quoted 4 lines

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

Quoted 14 lines · Miguel Ojeda

Quoted 2 lines · Jason Devers

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

+ // Note that T is #[must_use] but Option<T> is not.

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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

---
 rust/kernel/sync/lock.rs | 2 ++
 1 file changed, 2 insertions(+)

rust/kernel/sync/lock.rs +2 -0

index 41dcddac69e2..aacc227536cb 100644
@@ -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.

Signature · 1 line

2.44.2

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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

Quoted 5 lines · 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.

Quoted 6 lines

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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>

Reviewed-by Alice Ryhl <[email protected]>

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.

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 <[email protected]>

Link https://github.com/Rust-for-Linux/linux/issues/1133

Signed-off-by Jason Devers <[email protected]>