linux-kernel - Re: [patch 1/2] dump_stack: serialize the output from dump

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: <20130508154135.6a954bc4424050c607bc8378@linux-foundation.org>
Date:	Wed, 8 May 2013 15:41:35 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	athorlton@....com
Cc:	linux-kernel@...r.kernel.org, Vineet Gupta <vgupta@...opsys.com>,
	"David S. Miller" <davem@...emloft.net>,
	Richard Kuo <rkuo@...eaurora.org>,
	Jesper Nilsson <jesper.nilsson@...s.com>,
	Robin Holt <holt@....com>
Subject: Re: [patch 1/2] dump_stack: serialize the output from dump_stack()

On Wed, 08 May 2013 16:01:03 -0500 athorlton@....com wrote:

> These patches fix up issues with interspersed output from multiple
> simultaneous calls to warn or dump_stack on multi-cpu systems.
> References: <20130508210102.898396979@...lum.americas.sgi.com>
> Content-Disposition: inline; filename=dump-stack-serialize.patch
> 
> This patch adds functionality to serialize the output from dump_stack() to 
> avoid mangling of the output when dump_stack is called simultaneously from
> multiple cpus.
> 
> ...
>
> The original discussion regarding this patch can be found in this thread:
> [PATCH] x86: Avoid intermixing cpu dump_stack output on multi-processor systems

If there was anything useful or interesting in that discussion then it
should be included in this patch's changelog, please.  Don't send
everyone off hunting for emails.  Email to which they can't reply in
the context of this thread...

> +++ linux/lib/dump_stack.c
> @@ -7,6 +7,8 @@
>  #include <linux/export.h>
>  #include <linux/sched.h>
>  
> +static atomic_t dump_lock = ATOMIC_INIT(-1);
> +
>  /**
>   * dump_stack - dump the current task information and its stack trace
>   *
> @@ -14,7 +16,30 @@
>   */
>  void dump_stack(void)
>  {
> +	int was_locked;
> +	int old;
> +	int cpu;
> +
> +	preempt_disable();
> +
> +retry:
> +	cpu = smp_processor_id();
> +	old = atomic_cmpxchg(&dump_lock, -1, cpu);
> +	if (old == -1) {
> +		was_locked = 0;
> +	} else if (old == cpu) {
> +		was_locked = 1;
> +	} else {
> +		cpu_relax();
> +		goto retry;
> +	}
> +
>  	dump_stack_print_info(KERN_DEFAULT);
>  	show_stack(NULL, NULL);
> +
> +	if (!was_locked)
> +		atomic_set(&dump_lock, -1);
> +
> +	preempt_enable();

This would benefit from a comment explaining what it's doing and why. 
"Permit this cpu to perform nested stack dumps while serialising
against other CPUs".

The patch adds a load of goop which is unneeded on uniprocessor
kernels.  I guess a non-messy way of avoiding that is to just have two
versions of dump_stack() in this file.

It would be prudent to toss some more #includes in there.  For
atomic_foo() and cpu_relax(), for example.

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