linux-kernel - Re: [PATCH] time/sched_clock: mark sched_clock_read

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]

Date:   Tue, 29 Sep 2020 10:43:07 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     quanyang.wang@...driver.com, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Leo Yan <leo.yan@...aro.org>, Will Deacon <will@...nel.org>,
        a.darwish@...utronix.de,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Paul Cercueil <paul@...pouillou.net>,
        Randy Dunlap <rdunlap@...radead.org>, ben.dooks@...ethink.co.uk
Subject: Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as
 notrace

On Tue, 29 Sep 2020 09:13:33 +0200
Peter Zijlstra <peterz@...radead.org> wrote:

> On Mon, Sep 28, 2020 at 05:33:31PM -0400, Steven Rostedt wrote:
> > On Mon, 28 Sep 2020 12:58:59 +0200
> > Peter Zijlstra <peterz@...radead.org> wrote:
> >   
> > > > -struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > > +notrace struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > >  {
> > > >  	*seq = raw_read_seqcount_latch(&cd.seq);
> > > >  	return cd.read_data + (*seq & 1);    
> > > 
> > > At the very least sched_clock_read_retry() should also be marked such.
> > > 
> > > But Steve, how come x86 works? Our sched_clock() doesn't have notrace on
> > > at all.  
> > 
> > It's because of that magic in the Makefile that you love so much ;-)
> > 
> > CFLAGS_REMOVE_tsc.o = -pg  
> 
> ARGH!!, I really should write a script to fix up that mess :/

Please don't.

I did this because these files contain lots of functions, that if traced
can cause function tracing to reboot without any information, and as you
have recently found, they are very hard to find when they do happen.

I much rather blacklist an entire file, than to spend time debugging what
function caused the system to reboot.

I'd be OK with both. That is, add "notrace" to all the functions in the
file that isn't traced, just for documentation purposes. Perhaps call it
something else:

  file_notrace  ?

and have a comment about it being "this is just to let you know that the
functions are not traced because of the file".

And this "file_notrace" could even be:

/*
 * Denote functions that are not traced because the make file removes the
 * tracing features via a CONFIG_REMOVE_xxx.o = CC_FLAGS_FTRACE or similar.
 * This is for documentation purposes only, to remind people why a function
 * is not being traced.
 */
#define file_notrace /* Not traced because the file is blacklisted */

We could also add:

#define dir_notrace /* Not traced because the entire directory is blacklisted */

-- Steve