linux-kernel - Re: Fix quilt merge error in acpi-cpufreq.c

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: <200904221348.42677.rusty@rustcorp.com.au>
Date:	Wed, 22 Apr 2009 13:48:40 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Jonathan Corbet <corbet@....net>, "H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dave Jones <davej@...hat.com>, Theodore Tso <tytso@....edu>,
	Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: Fix quilt merge error in acpi-cpufreq.c

On Mon, 20 Apr 2009 08:08:05 pm Ingo Molnar wrote:
> Can you integrate these two into a single summary line? The obvious 
> solution would be to append them:
> 
>      tracing/ring-buffer: add unlock recursion protection on discard to fix spurious warning triggering tracing shutdown
> 
> but 121 characters width is _way_ too long for a summary.

No, if I'm reading this commit correctly, the commit message is well written,
just really bad.

    tracing/ring-buffer: Add unlock recursion protection on discard

-- This helps the patch reviewer, but noone else.  And the patch reviewer
   should be reading the entire thing anyway.  This is a fix, but you have
   to read to the bottom to know that.

    The pair of helpers trace_recursive_lock() and trace_recursive_unlock()
    have been introduced recently to provide generic tracing recursion
    protection.

    They are used in a symetric way:

     - trace_recursive_lock() on buffer reserve
     - trace_recursive_unlock() on buffer commit

-- Err, why is this verbiage in this patch at all?

    However sometimes, we don't commit but discard on entry
    to the buffer, ie: in case of filter checking.

    Then we must also unlock the recursion protection on discard time,
    otherwise the tracing gets definitely deactivated and a warning
    is raised spuriously, such as:

-- So, the problem is that tracing gets deactivated permanently?  Also a
   warning is raised (in which case is it really spurious?).   Since I have
   no idea what this code does, is it common?  When was this problem
   introduced?  

    111.119821] ------------[ cut here ]------------
    [  111.119829] WARNING: at kernel/trace/ring_buffer.c:1498 ring_buffer_lock_reserve+0x1b7/0x1d0()
    [  111.119835] Hardware name: AMILO Li 2727
    [  111.119839] Modules linked in:
    [  111.119846] Pid: 5731, comm: Xorg Tainted: G        W  2.6.30-rc1 #69
    [  111.119851] Call Trace:
    [  111.119863]  [<ffffffff8025ce68>] warn_slowpath+0xd8/0x130
    [  111.119873]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    [  111.119882]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    ...

-- Good, a backtrace is nice.

   [ Impact: fix spurious warning triggering tracing shutdown ]

-- Hidden away here, I don't think I like this.  Not an improvement.

    Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>

The patch itself basically adds trace_recursive_unlock() to event discard.
Seems like it should always have done that, and it's a simple bug that it
didn't.  But I had to work hard to figure that out.

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