From 3eed58fe9b36c34b437ec0f8a043e38d49a8a613 Mon Sep 17 00:00:00 2001 From: Nexoniarz Date: Sat, 12 Sep 2026 17:13:59 +0200 Subject: [PATCH] kernel: holdingsleep asks who holds the lock, not whether anyone does int r = lk->locked; // before int r = lk->locked && lk->pid == myproc()->pid; // after Seven callers, and every one of them is an assertion of the form if (!holdingsleep(&b->lock)) panic("brelse"); whose entire purpose is to catch a process releasing a lock it never took. Answering the weaker question disarms all of them: iunlock() on an inode locked by another process, and brelse() on somebody else's buffer, both sailed through and unlocked it underneath the real holder. Two processes then believe they hold the same inode, and the corruption surfaces somewhere else entirely -- which is exactly the shape of bug these panics exist to prevent. Found while hunting the ls -lR hang. It is not proven to be that hang's cause, and the instrumentation in the next commits says it is probably not. It is a real defect regardless, and it re-arms the detector that would catch the rest. myproc() is null on the boot path before the scheduler runs and acquiresleep() stores 0 for that case, so this compares the same way rather than dereferencing null. Checked that nothing touches a buffer that early, so the stricter assertion cannot fire spuriously on boot. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VQ7Qx8E1q1M1bbCq8QLHS2 --- kernel/core/sleeplock.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/kernel/core/sleeplock.c b/kernel/core/sleeplock.c index 987dd34..f1eb5ed 100644 --- a/kernel/core/sleeplock.c +++ b/kernel/core/sleeplock.c @@ -45,11 +45,30 @@ releasesleep(struct sleeplock *lk) release(&lk->lk); } +// Does *this process* hold lk? +// +// The pid comparison is not decoration. Without it this answers "is anyone +// holding this", and every caller is an assertion of the form +// +// if (!holdingsleep(&b->lock)) panic("brelse"); +// +// whose entire purpose is to catch a process releasing a lock it never took. +// Answering the weaker question disarms all seven of them: iunlock() on an +// inode locked by another process, and brelse() on somebody else's buffer, +// both sailed through and unlocked it underneath the real holder. Two +// processes then believe they hold the same inode, and the corruption surfaces +// somewhere else entirely -- which is exactly the shape of bug these panics +// exist to prevent. +// +// myproc() is null on the boot path before the scheduler runs, and +// acquiresleep() stores 0 for that case, so compare the same way rather than +// dereferencing null here. int holdingsleep(struct sleeplock *lk) { + struct proc *p = myproc(); acquire(&lk->lk); - int r = lk->locked; + int r = lk->locked && lk->pid == (p ? p->pid : 0); release(&lk->lk); return r; }