Re: [PATCH v4] lib/spinlock_debug.c: prevent a recursive cycle in the debug code

Sergey Senozhatsky
2016-01-29T04:03:49Z

On (01/29/16 12:00), Byungchul Park wrote:
[..]
> > it took a while to even find out that you are reporting this issues
> > not against a real H/W, but a qemu. I suppose qemu-arm running on
> > x86_64 box.
>
> No matter what kind of box I used because I only kept talking about the
> possiblity. It does not depend on a box at all.

well, qemu completely invalidates all of the H/W theories - powered off,
etc. so in a way it's important.

> > on very spin_dump recursive call it waits for the spin_lock and when
> > it eventually grabs it, it does the job that it wanted to do under
> > that spin lock, unlock it and return back. and the only case when it
> > never "return back" is when it never "eventually grabs it".
>
> Right. I missed it.

hm... we also can hit problems in spin_unlock() path. AND there are chances
that spin_unlock() can explode WITH OUT any memory corruption on sight, but
due to a coding error... a theoretical one:

we do unlock logbuf_lock, and debug_spin_unlock() is performed on a
locked logbuf_lock spin_lock

static inline void debug_spin_unlock(raw_spinlock_t *lock)
{
SPIN_BUG_ON(lock->magic != SPINLOCK_MAGIC, lock, "bad magic");
SPIN_BUG_ON(!raw_spin_is_locked(lock), lock, "already unlocked");
SPIN_BUG_ON(lock->owner != current, lock, "wrong owner");
SPIN_BUG_ON(lock->owner_cpu != raw_smp_processor_id(),
lock, "wrong CPU");
lock->owner = SPINLOCK_OWNER_INIT;
lock->owner_cpu = -1;
}

void do_raw_spin_unlock(raw_spinlock_t *lock)
{
debug_spin_unlock(lock);
arch_spin_unlock(&lock->raw_lock);
}

so if there was a coding error (schedule while atomic, or unlock from another
CPU) which resulted in faulty
lock->owner_cpu != raw_smp_processor_id()
OR
lock->owner != current

then this will explode:

printk
spin_lock
>> coding error <<
spin_unlock
printk
spin_lock
printk
spin_lock
printk
spin_lock
... boom

vprintk_emit() recursion detection code will not work for logbuf_lock here.
because the only criteria how vprintk_emit() can detect a recursion is via
static `logbuf_cpu' which is set to UINT_MAX right before it
raw_spin_unlock(&logbuf_lock). so from vprintk_emit() POV the logbuf_lock is
already unlocked. which is not true.

in case of memory corruption I don't think we must care, 'coding error case'
is _probably/may be_ something that can be improved, but I'm not really 100%
sure... and this still doesn't explain your console_sem.lock case.

-ss