linux-kernel - Re: [PATCH v4 11/13] rust: lock: add `Guard::do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANeycqqpR6Gs5Qb_pc2j_QV-JAc8xR360vth6We8xWU1GU5ASA@mail.gmail.com>
Date:   Wed, 12 Apr 2023 08:07:40 -0300
From:   Wedson Almeida Filho <wedsonaf@...il.com>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        linux-kernel@...r.kernel.org,
        Wedson Almeida Filho <walmeida@...rosoft.com>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>
Subject: Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@...il.com> wrote:
>
> On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> [...]
> > +
> > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > +        let _ = match guard_state {
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            None => unsafe { Self::lock(ptr) },
> > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > +            // initialised.
> > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > +        };
> > +    }
> >  }
> >
>
> One thing I'm little worried about the above is that we don't store back
> the new GuardState into `guard_state`, the particular case I'm worried
> about is as follow:
>
>         // IRQ is enabled.
>         // Disabling IRQ
>         unsafe { bindings::local_irq_disable(); }
>
>         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
>         // `g` records irq state is "irq disabled"
>
>         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
>         // restore into "irq disabled" mode.
>         // IRQ is disabled.
>
>         // Enabling IRQ
>         unsafe { bindings::local_irq_enable(); }
>         // IRQ is enabled.
>
>         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
>         // `g` still records irq state is "irq disabled"

Yes, that's by design. If you want it to record the new "irq enabled"
state, then you should call `lock()`, not `relock()`.

>         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
>         // restore into "irq disabled" mode.
>         // IRQ is disabled.
>
>
> This looks pretty scary to me, I would expect `relock()` updates the
> latest GuardState to the guard. Any reason it's implemented this way?

A `relock()` followed by an `unlock()` takes the state back to how it
was when `lock()` was originally called: this is precisely why
`relock()` exists.

Consider the following case:

```
local_disable_irq();
let mut guard = spinlock.lock();

guard.do_unlocked(|| {
    local_irq_enable();
    schedule();
});

drop(guard);
```

What would you expect the state to be? It's meant to be the state
right before `spinlock.lock()` was called, that's what the guard
represents.

If you want to preserve a new state, then you don't want `relock()`,
you just want a new `lock()` call.

> Regards,
> Boqun
>
> >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > +unsafe impl IrqSaveBackend for SpinLockBackend {
> >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> >          // memory, and that it has been initialised before.
> > --
> > 2.34.1
> >