linux-kernel - Re: [PATCH v2 07/17] debugobjects: Move printk out of db lock critical sections

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: <20181122020422.GA3441@jagdpanzerIV>
Date:   Thu, 22 Nov 2018 11:04:22 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Waiman Long <longman@...hat.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Will Deacon <will.deacon@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, kasan-dev@...glegroups.com,
        linux-mm@...ck.org, iommu@...ts.linux-foundation.org,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Tejun Heo <tj@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 07/17] debugobjects: Move printk out of db lock
 critical sections

On (11/21/18 11:49), Waiman Long wrote:
[..]
> >  	case ODEBUG_STATE_ACTIVE:
> > -		debug_print_object(obj, "init");
> >  		state = obj->state;
> >  		raw_spin_unlock_irqrestore(&db->lock, flags);
> > +		debug_print_object(obj, "init");
> >  		debug_object_fixup(descr->fixup_init, addr, state);
> >  		return;
> >  
> >  	case ODEBUG_STATE_DESTROYED:
> > -		debug_print_object(obj, "init");
> > +		debug_printobj = true;
> >  		break;
> >  	default:
> >  		break;
> >  	}
> >  
> >  	raw_spin_unlock_irqrestore(&db->lock, flags);
> > +	if (debug_chkstack)
> > +		debug_object_is_on_stack(addr, onstack);
> > +	if (debug_printobj)
> > +		debug_print_object(obj, "init");
> >
[..]
>
> As a side note, one of the test systems that I used generated a
> debugobjects splat in the bootup process and the system hanged
> afterward. Applying this patch alone fix the hanging problem and the
> system booted up successfully. So it is not really a good idea to call
> printk() while holding a raw spinlock.

Right, I like this patch.
And I think that we, maybe, can go even further.

Some serial consoles call mod_timer(). So what we could have with the
debug objects enabled was

	mod_timer()
	 lock_timer_base()
	  debug_activate()
	   printk()
	    call_console_drivers()
	     foo_console()
	      mod_timer()
	       lock_timer_base()       << deadlock

That's one possible scenario. The other one can involve console's
IRQ handler, uart port spinlock, mod_timer, debug objects, printk,
and an eventual deadlock on the uart port spinlock. This one can
be mitigated with printk_safe. But mod_timer() deadlock will require
a different fix.

So maybe we need to switch debug objects print-outs to _always_
printk_deferred(). Debug objects can be used in code which cannot
do direct printk() - timekeeping is just one example.

	-ss