Mutex::lock_noguard() may be unsafe

[fbq]

@wedsonaf This came to me when I'm investigating the safety of schedule.

Linux's mutex_lock() sets ->lock to the current task once the lock is grabbed. If another thread tries to grab the lock, instead of sleeping immediately, it will check whether the lock owner is on cpu or not (mutex is a sleepable lock) via task_struct::on_cpu. And if the task is running on some CPU, the owner may finish its work soon and release the lock, so the other thread will spin instead of sleep.

Consider the following case

let m: Ref<Mutex<...>>= ...;
let m_a = m.clone();
let m_b = m.clone();

Task::spawn(move || {  // spawn "t1"
    let _ = m_a.lock_noguad(); // m's owner is "t1"
});

// "t1" gets freed. but m's owner is still "t1"

Task::spawn(move || { // spawn "t2"
    m_b.lock_noguard();
      mutex_lock():
        __mutex_lock_common():
          mutex_optimistic_spin():
            mutex_spin_on_owner():
              owner_on_cpu():
                <read owner->on_cpu> // UAF, unsafe

This means Mutex::lock_noguard() is unsafe, and the safety guarantee would be making sure that a thread releases the lock before exiting or https://github.com/Rust-for-Linux/linux/pull/863 is needed

[wedsonaf]

Very cool find, @fbq !

I'm not a big fan of the idea of having extra atomic incs/decs on the lock/unlock paths as that would clearly make the Rust version more expensive. We may have to do it though if we can't find another way of dealing with this. Let me think about this for a bit.

[fbq]

Very cool find, @fbq !

I'm not a big fan of the idea of having extra atomic incs/decs on the lock/unlock paths as that would clearly make the Rust version more expensive. We may have to do it though if we can't find another way of dealing with this. Let me think about this for a bit.

I share the same worry about having extra atomic incs/decs. Maybe we just make lock_noguard unsafe, because most of the lock users will use Guard-API, plus if a user use lock_noguard, the unsafe unlock will also be used, in other words, still need to engage with some unsafety.

[filip-hejsek]

I think the Guard API is also unsafe because one can mem::forget the Guard (or leak it in a reference cycle).

[fbq]

I think the Guard API is also unsafe because one can mem::forget the Guard (or leak it in a reference cycle).

Ah, you are right.