linux-kernel - Re: [PATCH] kernel/trace: Add TRACING_ALLOW

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: <20200628220209.3oztcjnzsotlfria@ast-mbp.dhcp.thefacebook.com>
Date:   Sun, 28 Jun 2020 15:02:09 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Nicolas Boichat <drinkcat@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Peter Zijlstra <peterz@...radead.org>,
        Vinod Koul <vkoul@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Guilherme G . Piccoli" <gpiccoli@...onical.com>,
        Will Deacon <will@...nel.org>,
        Douglas Anderson <dianders@...omium.org>,
        Guenter Roeck <groeck@...omium.org>, bpf@...r.kernel.org
Subject: Re: [PATCH] kernel/trace: Add TRACING_ALLOW_PRINTK config option

On Sun, Jun 28, 2020 at 03:43:31PM -0400, Steven Rostedt wrote:
>  On Sun, 28 Jun 2020 12:21:07 -0700
> Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> 
> > > Re-teach them, or are you finally admitting that the tracing system is
> > > a permanent API?  This is the reason people are refusing to add trace
> > > points into their subsystems. Because user space may make it required.
> > > 
> > > I see no reason why you can't create a dedicated BPF tracing instance
> > > (you only need one) to add all your trace_array_printk()s to.  
> > 
> > All bpf helpers are stable api. We cannot remove bpf_trace_printk() and
> > cannot change the fact that it has to print into /sys/kernel/debug/tracing/trace.
> 
> Then do a bpf trace event and enable it when a bpf_trace_printk() is
> loaded. It will work the same for your users.

I'm not sure I follow. How that would preserve the expectation
to see the output in /sys/kernel/debug/tracing/trace ?

> > If we do so a lot of users will complain. Loudly.
> > If you really want to see the flames, go ahead and rename 'trace_pipe'
> > into something else.
> 
> The layout of the tracefs system *is* a stable API. No argument there.
> 
> > This has nothing to do with tracing in general and tracepoints.
> > Those come and go.
> 
> And in this case, trace_printk() is no different than any other trace
> event. Obviously, your use case doesn't let it go. If some tool starts
> relying on another trace event (say someone adds another bpf handler that
> enables a trace event, and is documented) then under your scenario,
> it's a stable API.

not quite. Documneting kprobe+bpf as an example and writing a blog and a book
about it doesn't make it stable.

> 
> Hence, your "tracepoints come and go" is not universal, and there's no
> telling which ones will end up being a stable API.
> 
> 
> > If you really want to nuke trace_printk from the kernel we need time
> > to work on replacement and give users at least few releases of helper
> > deprecation time.
> 
> I never said I would nuke it. This patch in question makes it so those
> that don't want that banner to ever show up can do so. A trace-printk()
> is something to add via compiling. And since I and others use it
> heavily for debugging, I would have this option not be a default, but
> something that others can enable.
> 
> > We've never done in the past though.
> > There could be flames even if we deprecate it gradually.
> > Looking how unyielding you're about this banner I guess we have to start
> > working on replacement sooner than later. Oh well.
> 
> Hmm, so you are happier to bully and burn bridges with me to deprecate
> the trace_printk() interface, than to work with me and add an update to
> look into an instance for the print instead of the top level? That's
> not very collaborative.

I'm seeing it differently.
I'm saying bpf users are complaining about misleading dmesg warning.
You're saying 'screw your users I want to keep that warning'.
Though the warning is lying with a straight face. The only thing happened
is few pages were allocated that will never be freed. The kernel didn't
suddenly become non-production. It didn't become slower. No debug features
were turned on.