linux-kernel - Re: [PATCH] tracing: (backport) Replace kmap with copy_from_user() in trace

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: <20161209080551.GA7533@clotho.rd.cisco.com>
Date:   Fri, 9 Dec 2016 09:05:51 +0100
From:   Henrik Austad <haustad@...co.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Henrik Austad <henrik@...tad.us>, linux-kernel@...r.kernel.org,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>, stable@...r.kernel.org
Subject: Re: [PATCH] tracing: (backport) Replace kmap with copy_from_user()
 in trace_marker

On Fri, Dec 09, 2016 at 08:22:05AM +0100, Greg KH wrote:
> On Fri, Dec 09, 2016 at 07:34:04AM +0100, Henrik Austad wrote:
> > Instead of using get_user_pages_fast() and kmap_atomic() when writing
> > to the trace_marker file, just allocate enough space on the ring buffer
> > directly, and write into it via copy_from_user().
> > 
> > Writing into the trace_marker file use to allocate a temporary buffer
> > to perform the copy_from_user(), as we didn't want to write into the
> > ring buffer if the copy failed. But as a trace_marker write is suppose
> > to be extremely fast, and allocating memory causes other tracepoints to
> > trigger, Peter Zijlstra suggested using get_user_pages_fast() and
> > kmap_atomic() to keep the user space pages in memory and reading it
> > directly.
> > 
> > Instead, just allocate the space in the ring buffer and use
> > copy_from_user() directly. If it faults, return -EFAULT and write
> > "<faulted>" into the ring buffer.
> > 
> > On architectures without a arch-specific get_user_pages_fast(), this
> > will end up in the generic get_user_pages_fast() and this grabs
> > mm->mmap_sem. Once you do this, then suddenly writing to the
> > trace_marker can cause priority-inversions.
> > 
> > This is a backport of Steven Rostedts patch [1] and applied to 3.10.x so the
> > signed-off-chain by is somewhat uncertain at this stage.
> > 
> > The patch compiles, boots and does not immediately explode on impact. By
> > definition [2] it must therefore be perfect
> > 
> > 2) https://www.spinics.net/lists/kernel/msg2400769.html
> > 2) http://lkml.iu.edu/hypermail/linux/kernel/9804.1/0149.html
> > 
> > Cc: Ingo Molnar <mingo@...nel.org>
> > Cc: Henrik Austad <henrik@...tad.us>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Steven Rostedt <rostedt@...dmis.org>
> > Cc: stable@...r.kernel.org
> > 
> > Suggested-by: Thomas Gleixner <tglx@...utronix.de>
> > Used-to-be-signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> > Backported-by: Henrik Austad <haustad@...co.com>
> > Tested-by: Henrik Austad <haustad@...co.com>
> > Signed-off-by: Henrik Austad <haustad@...co.com>
> > ---
> >  kernel/trace/trace.c | 78 +++++++++++++++-------------------------------------
> >  1 file changed, 22 insertions(+), 56 deletions(-)
> 
> What is the git commit id of this patch in Linus's tree?  And what
> stable trees do you feel it should be applied to?

Ah, perhaps I jumped the gun here. I don't think Linus has picked this one 
up yet, Steven sent out the patch yesterday.

Since then, I've backported it to 3.10 and ran the first set of tests 
over night and it looks good. So ideally this would find its way into 
3.10(.104).

Do you want med to resubmit when Stevens patch is merged upstream?

-Henrik