rust: sync: Make Guard::lock_ref unavailable if Lock is not Sync

Dear Linux Kernel Maintainers,

We are developing a tool called FerroLens to detect potential unsound behavior in Rust code in the Linux kernel.
The tool internally uses an LLM to detect bugs.
We then perform manual analysis to verify these reports. FerroLens reported the following bug in rust/kernel/sync/lock.rs.

Guard::lock_ref() returns a reference to the Lock stored in the Guard. Guard is Sync if T is Sync. However, Lock is Sync only if T is Send. This means that a reference to Guard can be sent across threads and multiple threads can acquire a reference to the Lock,
even if Lock is not Sync.

The following scenario is possible:
If T is sync but not send,
Thread A: Creates a Lock<T>. Because T is not Send, Lock<T> is not Sync. It cannot be shared with other threads.
Thread A: Locks the mutex and gets a Guard<T>.
Since T is Sync, code says Guard<T> is Sync.
Thread A can now safely give a &Guard<T> to Thread B.
Thread B: calls guard.lock_ref(). This returns a &'a Lock<T>. Thread B now has a reference to the Lock. In this case,
even after Thread A drops the guard, Thread B still has that &Lock. Thread B can now call lock.lock() and get a &mut T.
This is unsound.

Here is a poc kernel module which demonstrates this problem:

use core::sync::atomic::{AtomicBool, Ordering};

use kernel::{
    bindings, c_str,
    error::from_err_ptr,
    prelude::*,
    sync::{
        lock::{mutex::MutexBackend, Backend, Guard, Lock},
        new_mutex, Arc,
    },
    types::NotThreadSafe,
};

use pin_init::stack_pin_init;

module! {
type: LockRefPoc,
name: "lock_ref_poc",
authors: ["Priya Govindasamy"],
description: "Sample module demonstrating cross-thread Guard::lock_ref access",
license: "GPL",
}

struct LockRefPoc;

struct SyncButNotSend {
    value: u32,
    _not_send: NotThreadSafe,
}

// SAFETY: Access to value is synchronized by the lock in this sample, and the marker field is // only used to prevent Send.
unsafe impl Sync for SyncButNotSend {}

struct HandoffState {
lock_ref_ready: AtomicBool,
guard_dropped: AtomicBool,
relock_done: AtomicBool,
}

struct ThreadBContext {
    guard_addr: usize,
    handoff: Arc<HandoffState>,
    thread_id: u32,
}

fn guard_matches_lock<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) -> bool {
    core::ptr::eq(guard.lock_ref(), lock)
}

unsafe extern "C" fn thread_b_main(data: *mut kernel::ffi::c_void) -> i32 {
    // SAFETY: `data` comes from `Arc::into_raw` in `spawn_thread_b`, and the thread takes back
    // ownership of exactly one reference.
    let ctx = unsafe { Arc::from_raw(data.cast::<ThreadBContext>()) };

    let guard: &Guard<'_, SyncButNotSend, MutexBackend> =
        // SAFETY: Thread A passes the address of a live guard to thread B.
        unsafe { &*(ctx.guard_addr as *const Guard<'_, SyncButNotSend, MutexBackend>) };

    let leaked_lock = guard.lock_ref();
    pr_info!(
        "thread B obtained &Lock<SyncButNotSend, _> through &Guard\n",
    );
    ctx.handoff.lock_ref_ready.store(true, Ordering::Release);

    while !ctx.handoff.guard_dropped.load(Ordering::Acquire) {
        unsafe { bindings::msleep(20) };
    }

    let mut relocked = leaked_lock.lock();
    relocked.value += 1;
    pr_info!(
        "thread B locked after thread A dropped the original guard; value={}\n",
        relocked.value
    );
    ctx.handoff.relock_done.store(true, Ordering::Release);
    0
}

fn spawn_thread_b(guard_addr: usize, handoff: Arc<HandoffState>, thread_id: u32) -> Result {
    let ctx = Arc::new(
        ThreadBContext {
            guard_addr,
            handoff,
            thread_id,
        },
        GFP_KERNEL,
    )?;
    let data = Arc::into_raw(ctx) as *mut kernel::ffi::c_void;

    let task = match from_err_ptr(unsafe {
        bindings::kthread_create_on_node(
            Some(thread_b_main),
            data,
            bindings::NUMA_NO_NODE,
            c_str!("lock_ref_thread_b/%u").as_char_ptr(),
            thread_id,
        )
    }) {
        Ok(task) => task,
        Err(err) => {
            // SAFETY: Thread creation failed, so the raw reference was not handed off.
            unsafe { drop(Arc::from_raw(data.cast::<ThreadBContext>())) };
            return Err(err);
        }
    };

    // SAFETY: `task` is a valid sleeping kthread returned by `kthread_create_on_node`.
    unsafe { bindings::wake_up_process(task) };
    Ok(())
}

unsafe extern "C" fn thread_a_main(_data: *mut kernel::ffi::c_void) -> i32 {
    let handoff = match Arc::new(
        HandoffState {
            lock_ref_ready: AtomicBool::new(false),
            guard_dropped: AtomicBool::new(false),
            relock_done: AtomicBool::new(false),
        },
        GFP_KERNEL,
    ) {
        Ok(h) => h,
        Err(_) => return ENOMEM.to_errno(),
    };

    stack_pin_init!(let lock = new_mutex!(
        SyncButNotSend {
            value: 100,
            _not_send: NotThreadSafe,
        },
        "lock_ref_poc.thread_a_lock"
    ));

    let guard = lock.lock();
    if !guard_matches_lock(&guard, &lock) {
        pr_err!("thread A: guard.lock_ref() did not return the original lock\n");
        return EINVAL.to_errno();
    }

    if let Err(err) = spawn_thread_b(
        (&guard as *const Guard<'_, SyncButNotSend, MutexBackend>) as usize,
        handoff.clone(),
        2,
    ) {
        return err.to_errno();
    }

    while !handoff.lock_ref_ready.load(Ordering::Acquire) {
        unsafe { bindings::msleep(20) };
    }

    drop(guard);
    pr_info!("thread A dropped the original guard after thread B captured &Lock\n");
    handoff.guard_dropped.store(true, Ordering::Release);

    while !handoff.relock_done.load(Ordering::Acquire) {
        unsafe { bindings::msleep(20) };
    }

    0
}

fn spawn_thread_a() -> Result {
    let task = match from_err_ptr(unsafe {
        bindings::kthread_create_on_node(
            Some(thread_a_main),
            core::ptr::null_mut(),
            bindings::NUMA_NO_NODE,
            c_str!("lock_ref_thread_a").as_char_ptr(),
        )
    }) {
        Ok(task) => task,
        Err(err) => return Err(err),
    };

    // SAFETY: `task` is a valid sleeping kthread returned by `kthread_create_on_node`.
    unsafe { bindings::wake_up_process(task) };
    Ok(())
}

impl kernel::Module for LockRefPoc {
    fn init(_module: &'static ThisModule) -> Result<Self> {
        
        spawn_thread_a()?;

        Ok(Self)
    }
}

impl Drop for LockRefPoc {
    fn drop(&mut self) {
        pr_info!("lock_ref_poc unloaded\n");
    }
}

Makefile:

obj-m += lock_ref_poc.o

KDIR := /lib/modules/$(shell uname -r)/build

PWD := $(shell pwd)

all:

	$(MAKE) -C $(KDIR) M=$(PWD) LLVM=1 modules

clean:

	$(MAKE) -C $(KDIR) M=$(PWD) clean

Output:

[  360.591802] lock_ref_poc: thread B obtained &Lock<SyncButNotSend, _> through &Guard
[  360.617600] lock_ref_poc: thread A dropped the original guard after thread B captured &Lock
[  360.641550] lock_ref_poc: thread B locked after thread A dropped the original guard; value=101
[  362.580557] lock_ref_poc: lock_ref_poc unloaded

Priya Bala Govindasamy (1):
Make Guard::lock_ref unavailable if Lock is not Sync

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

2.34.1

Guard::lock_ref() returns a reference to the Lock stored in the Guard. Guard is Sync if T is Sync. However, Lock is Sync only if T is Send. This means that a reference to Guard can be sent across threads and multiple threads can acquire a reference to the Lock,
even if Lock is not Sync.

The following scenario is possible:
If T is sync but not send,
Thread A: Creates a Lock<T>. Because T is not Send, Lock<T> is not Sync. It cannot be shared with other threads.
Locks the mutex and gets a Guard<T>.
Since T is Sync, code says Guard<T> is Sync.
Thread A can now safely give a &Guard<T> to Thread B.
Thread B: calls guard.lock_ref(). This returns a &'a Lock<T>. Thread B now has a reference to the Lock. In this case,
even after Thread A drops the guard, Thread B still has that &Lock. Thread B can now call lock.lock() and get a &mut T.
This is unsound.

Fix this issue by adding a trait bound to Guard::lock_ref so that this method is available only when Lock<T, B> implements Sync. This ensures that multiple threads cannot acquire a reference to the Lock if it is not Sync.

---
changes in v2:
- Add a trait bound only to the Guard::lock_ref() method. Changes to the lock lifetime are not necessary.

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

index 10b6b5e9b024..91317d0d81b9 100644
@@ -234,7 +234,10 @@ impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> {
     /// // `g` originates from `l`.
     /// assert_held(&g, &l);
     /// ```
-    pub fn lock_ref(&self) -> &'a Lock<T, B> {
+    pub fn lock_ref(&self) -> &'a Lock<T, B>
+    where
+        Lock<T, B>: Sync,
+    {
         self.lock
     }
 

2.34.1

Please quote code with ``

This is way too verbose. Please be concise. Something like

For l: Lock<T>, this means that send_across_thread(&l.lock()).lock_ref() is able to send &Lock<T> across threads despite T: !Sync.

should be more readable.

Again, this is repeating the same thing with two different style. Typical LLM style verbosity. Just remove the second sentence and keep the first.