linux-kernel - Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary buffer

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]

Date:	Mon, 7 Dec 2015 15:16:46 +0100
From:	Petr Mladek <pmladek@...e.com>
To:	yalin wang <yalin.wang2010@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Daniel Thompson <daniel.thompson@...aro.org>,
	Jiri Kosina <jkosina@...e.com>, Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-kernel <linux-kernel@...r.kernel.org>, x86@...nel.org,
	linux-arm-kernel@...ts.infradead.org,
	adi-buildroot-devel@...ts.sourceforge.net,
	linux-cris-kernel@...s.com, linux-mips@...ux-mips.org,
	linuxppc-dev@...ts.ozlabs.org, linux-s390@...r.kernel.org,
	linux-sh@...r.kernel.org, sparclinux@...r.kernel.org
Subject: Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary
 buffer

On Mon 2015-11-30 08:42:04, yalin wang wrote:
> 
> > On Nov 27, 2015, at 19:09, Petr Mladek <pmladek@...e.com> wrote:
> > 
> > Testing has shown that the backtrace sometimes does not fit
> > into the 4kB temporary buffer that is used in NMI context.
> > 
> > The warnings are gone when I double the temporary buffer size.
> > 
> > Note that this problem existed even in the x86-specific
> > implementation that was added by the commit a9edc8809328
> > ("x86/nmi: Perform a safe NMI stack trace on all CPUs").
> > Nobody noticed it because it did not print any warnings.
> > 
> > Signed-off-by: Petr Mladek <pmladek@...e.com>
> > ---
> > kernel/printk/nmi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> > index 8af1e4016719..6111644d5f01 100644
> > --- a/kernel/printk/nmi.c
> > +++ b/kernel/printk/nmi.c
> > @@ -42,7 +42,7 @@ atomic_t nmi_message_lost;
> > struct nmi_seq_buf {
> > 	atomic_t		len;	/* length of written data */
> > 	struct irq_work		work;	/* IRQ work that flushes the buffer */
> > -	unsigned char		buffer[PAGE_SIZE - sizeof(atomic_t) -
> > +	unsigned char		buffer[2 * PAGE_SIZE - sizeof(atomic_t) -
> > 				       sizeof(struct irq_work)];
> > };
> > 
> 
> why not define like this:
> 
> union {
> struct {atomic_t		len;	
> 	struct irq_work		work;
> }
> unsigned char		buffer[PAGE_SIZE * 2] ;
> }
> 
> we can make sure the union is 2 PAGE_SIZE .

IMHO, this would add more confusion. It would just move the
computation somewhere else. The union will have 2*PAGE_SIZE
but the beginning of "buffer" will be shared with "len"
and "work". Therefore, we would need to skip the beginning
of the buffer when storing the data. By other words, we still
will be able to use only (sizeof(buffer) - sizeof(atomic_t) -
sizeof(struct irq_work)] of the "buffer".

Or did I miss something, please?

Best Regards,
Petr
--
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/