linux-kernel - Re: [PATCH 3.19-rc2 v15 5/8] printk: Simple implementation for NMI backtracing

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: <alpine.DEB.2.11.1501242105230.5526@nanos>
Date:	Sat, 24 Jan 2015 22:44:05 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Daniel Thompson <daniel.thompson@...aro.org>
cc:	Jason Cooper <jason@...edaemon.net>,
	Russell King <linux@....linux.org.uk>,
	Will Deacon <will.deacon@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Marc Zyngier <marc.zyngier@....com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	John Stultz <john.stultz@...aro.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	patches@...aro.org, linaro-kernel@...ts.linaro.org,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Dirk Behme <dirk.behme@...bosch.com>,
	Daniel Drake <drake@...lessm.com>,
	Dmitry Pervushin <dpervushin@...il.com>,
	Tim Sander <tim@...eglstein.org>
Subject: Re: [PATCH 3.19-rc2 v15 5/8] printk: Simple implementation for NMI
 backtracing

On Fri, 23 Jan 2015, Daniel Thompson wrote:
> +#ifdef CONFIG_ARCH_WANT_NMI_PRINTK
> +extern __printf(1, 0) int nmi_vprintk(const char *fmt, va_list args);
> +
> +struct cpumask;
> +extern int prepare_nmi_printk(struct cpumask *cpus);
> +extern void complete_nmi_printk(struct cpumask *cpus);
> +
> +/*
> + * Replace printk to write into the NMI seq.
> + *
> + * To avoid include hell this is a macro rather than an inline function
> + * (printk_func is not declared in this header file).
> + */
> +#define this_cpu_begin_nmi_printk() ({ \
> +	printk_func_t __orig = this_cpu_read(printk_func); \
> +	this_cpu_write(printk_func, nmi_vprintk); \
> +	__orig; \
> +})
> +#define this_cpu_end_nmi_printk(fn) this_cpu_write(printk_func, fn)

Why can't we just make it a proper function in printk.c and make
DEFINE_PER_CPU(printk_func_t, printk_func) static once x86 is
converted over, thereby getting rid of the misplaced declaration in
percpu.h?

It's really not performance critical at all. If you do system wide
backtraces a function call is the least of your worries.

> +#ifdef CONFIG_ARCH_WANT_NMI_PRINTK

Why can't this simply be CONFIG_PRINTK_NMI and live at the same place
as the other printk related options?

> +int nmi_vprintk(const char *fmt, va_list args)
> +{
> +	struct nmi_seq_buf *s = this_cpu_ptr(&nmi_print_seq);
> +	unsigned int len = seq_buf_used(&s->seq);
> +
> +	seq_buf_vprintf(&s->seq, fmt, args);
> +	return seq_buf_used(&s->seq) - len;
> +}
> +EXPORT_SYMBOL_GPL(nmi_vprintk);

What's the point of these exports? This stuff is really not supposed
to be used inside random modules.

> +/*
> + * Check for concurrent usage and set up per_cpu seq_buf buffers that the NMIs
> + * running on the other CPUs will write to. Provides the mask of CPUs it is
> + * safe to write from (i.e. a copy of the online mask).
> + */
> +int prepare_nmi_printk(struct cpumask *cpus)

Can we please make all this proper prefixed? , i.e. printk_nmi_*

> +{
> +	struct nmi_seq_buf *s;
> +	int cpu;
> +
> +	if (test_and_set_bit(0, &nmi_print_flag)) {
> +		/*
> +		 * If something is already using the NMI print facility we
> +		 * can't allow a second one...
> +		 */
> +		return -EBUSY;

So what's the point of saving and restoring the printk_func pointer at
the call site?

void printk_nmi_begin()
{
	if (__this_cpu_inc_return(nmi_printk_nest_level) == 1)
	      this_cpu_write(printk_func, nmi_vprintk);
}

void printk_nmi_end()
{
	if (__this_cpu_dec_return(nmi_printk_nest_level) > 0)
	        return;
        this_cpu_write(printk_func, default_vprintk);
	if (in_nmi())
		irq_work_schedule();
        else
		printk_nmi_complete();
}

> +	}
> +
> +	cpumask_copy(cpus, cpu_online_mask);

Why do you need external storage for this if nesting is not allowed?
What's wrong with having a printk_nmi_mask? It's protected by the
nmi_print_flag, so the call sites do not have to take care about
protecting it until printk_nmi_complete() has been invoked.

> +	for_each_cpu(cpu, cpus) {
> +		s = &per_cpu(nmi_print_seq, cpu);
> +		seq_buf_init(&s->seq, s->buffer, NMI_BUF_SIZE);

Why do you want to do this here? The buffers should be initialized
before the first NMI can hit and the complete code should reinit them
before the next printk_nmi_prepare() sees the nmi_print_flag cleared.

> +static void print_seq_line(struct nmi_seq_buf *s, int start, int end)
> +{
> +	const char *buf = s->buffer + start;
> +
> +	printk("%.*s", (end - start) + 1, buf);
> +}
> +
> +void complete_nmi_printk(struct cpumask *cpus)
> +{
> +	struct nmi_seq_buf *s;
> +	int len;
> +	int cpu;
> +	int i;

Please condense all ints to a single line, but what's worse is the
completely inconsistency versus scopes.

len and i are only used in the for_each loop. Either we put all of
them at the top of the function or we do it right.

Thanks,

	tglx
--
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/