linux-kernel - Re: [RFC][PATCH v2 -next 2/2] Adding lock operations to kmsg_dump()/pstore

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] [day] [month] [year] [list]

Message-ID: <20111110133329.GI5629@redhat.com>
Date:	Thu, 10 Nov 2011 08:33:29 -0500
From:	Don Zickus <dzickus@...hat.com>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Seiji Aguchi <seiji.aguchi@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Chen, Gong" <gong.chen@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Luck, Tony" <tony.luck@...el.com>,
	Matthew Garrett <mjg@...hat.com>,
	Vivek Goyal <vgoyal@...hat.com>,
	"len.brown@...el.com" <len.brown@...el.com>,
	"ying.huang@...el.com" <ying.huang@...el.com>,
	"ak@...ux.intel.com" <ak@...ux.intel.com>,
	"hughd@...omium.org" <hughd@...omium.org>,
	"mingo@...e.hu" <mingo@...e.hu>,
	"jmorris@...ei.org" <jmorris@...ei.org>,
	"namhyung@...il.com" <namhyung@...il.com>,
	"dle-develop@...ts.sourceforge.net" 
	<dle-develop@...ts.sourceforge.net>,
	Satoru Moriya <satoru.moriya@....com>
Subject: Re: [RFC][PATCH v2 -next 2/2] Adding lock operations to
 kmsg_dump()/pstore_dump()

On Thu, Nov 10, 2011 at 01:50:25PM +0100, Peter Zijlstra wrote:
> On Fri, 2011-10-21 at 17:21 -0400, Seiji Aguchi wrote:
> > +++ b/fs/pstore/platform.c
> > @@ -97,6 +97,17 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> >         else
> >                 why = "Unknown";
> >  
> > +       /*
> > +        * pstore_dump() is called after smp_send_stop() in panic path.
> > +        * So, spin_lock should be bust for avoiding deadlock.
> > +        */
> > +       if (reason == KMSG_DUMP_PANIC)
> > +               spin_lock_init(&psinfo->buf_lock);
> > +
> > +       /*
> > +        * While a cpu is in NMI handler, other cpus may be running.
> > +        * So, trylock should be called so that lockdep checking works.
> > +        */
> 
> Don't be silly, lockdep doesn't cover NMI, in fact you shouldn't use
> locks from NMI context ever.

Heh.  I would normally agree, but in this case we have a piece of hardware
that can be accessed from normal, irq and NMI context.  I still scratch my
head for the best way to handle this.  This approach was sorta of a
bandaid effort to prevent a deadlock in the NMI panic case.

> 
> >         if (in_nmi()) {
> >                 is_locked = spin_trylock(&psinfo->buf_lock);
> >                 if (!is_locked)
> > diff --git a/kernel/printk.c b/kernel/printk.c
> > index 1455a0d..f51f547 100644
> > --- a/kernel/printk.c
> > +++ b/kernel/printk.c
> > @@ -1730,15 +1730,37 @@ void kmsg_dump(enum kmsg_dump_reason reason)
> >         struct kmsg_dumper *dumper;
> >         const char *s1, *s2;
> >         unsigned long l1, l2;
> > -       unsigned long flags;
> > +       unsigned long flags = 0;
> > +       int is_locked = 0;
> >  
> >         /* Theoretically, the log could move on after we do this, but
> >            there's not a lot we can do about that. The new messages
> >            will overwrite the start of what we dump. */
> > -       raw_spin_lock_irqsave(&logbuf_lock, flags);
> > +
> > +       /*
> > +        * kmsg_dump() is called after smp_send_stop() in panic path.
> > +        * So, spin_lock should be bust for avoiding deadlock.
> > +        */
> > +       if (reason == KMSG_DUMP_PANIC)
> > +               raw_spin_lock_init(&logbuf_lock); 
> 
> In both cases where you bust the spinlock at least yell loudly and
> disable lock debugging.
> 
> And I guess this is where Don wants to use NMIs for smp_send_stop() so
> what you get around the fact that this lock you're busting disabled
> IRQs?

:-)  I thought it would be safer to bust spinlocks if we could have a
better gaurantee the other cpus were not accessing the hardware at the
same time.

> 
> All in all this patch is way ugly and doesn't make me feel all warm and
> fuzzy.

I understand.

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/