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

jos
2024-12-08T19:43:23Z

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