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: <1997032737.615438.1581179485507.JavaMail.zimbra@efficios.com>
Date:   Sat, 8 Feb 2020 11:31:25 -0500 (EST)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     "Joel Fernandes, Google" <joel@...lfernandes.org>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Ingo Molnar <mingo@...hat.com>,
        Richard Fontana <rfontana@...hat.com>,
        rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        paulmck <paulmck@...nel.org>,
        Josh Triplett <josh@...htriplett.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Subject: Re: [RFC 0/3] Revert SRCU from tracepoint infrastructure

----- On Feb 7, 2020, at 3:56 PM, Joel Fernandes, Google joel@...lfernandes.org wrote:

> Hi,
> These patches remove SRCU usage from tracepoints. The reason for proposing the
> reverts is because the whole point of SRCU was to avoid having to call
> rcu_irq_enter_irqson(). However this was added back in 865e63b04e9b2 ("tracing:
> Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") because perf
> was breaking..

I think the original patch re-enabling the rcu_irq_enter/exit_irqson() is a
tracepoint band-aid over what should actually been fixed within perf instead.

Perf should not do rcu_read_lock/unlock()/synchronize_rcu(), but rather use
tracepoint_synchronize_unregister() to match the read-side provided by
tracepoints.

If perf can then just rely on the underlying synchronization provided by each
instrumentation providers (tracepoint, kprobe, ...) and not explicitly add its own
unneeded synchronization on top (e.g. rcu_read_lock/unlock), then it should simplify
all this.

> 
> Further it occurs to me that, by using SRCU for tracepoints, we forgot that RCU
> is not really watching the tracepoint callbacks. This means that anyone doing
> preempt_disable() in their tracepoint callback, and expecting RCU to listen to
> them is in for a big surprise. When RCU is not watching, it does not care about
> preempt-disable sections on CPUs as you can see in the forced-quiescent state
> loop.

As Paul noted, SRCU is the exception to the "RCU watching".

> 
> Since SRCU is not providing any benefit because of 865e63b04e9b2 anyway, let us
> revert SRCU tracepoint code to maintain the sanity of potential
> tracepoint callback registerers.

Introducing SRCU was done to simplify handling of rcuidle, thus removing some
significant overhead that has been noticed due to use of rcu_irq_enter/exit_irqson().

There is another longer-term goal in adding SRCU-synchronized tracepoints: adding
the ability to create tracepoint probes which will be allowed to handle page
faults properly. This is very relevant for the syscall tracepoints reading the
system call parameters from user-space. Currently, we are stuck with sub par
hacks such as filling the missing data with zeroes. Usually nobody notices because
most syscall arguments are typically hot in the page cache, but it is still fragile.

I'd very much prefer see commits moving syscall tracepoints to use of SRCU
(without preempt disable around the tracepoint calls) rather than a commit removing
tracepoint SRCU use because of something that needs to be fixed within perf.

Thanks,

Mathieu


> 
> Joel Fernandes (Google) (3):
> Revert "tracepoint: Use __idx instead of idx in DO_TRACE macro to make
> it unique"
> Revert "tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle
> tracepoints"
> Revert "tracepoint: Make rcuidle tracepoint callers use SRCU"
> 
> include/linux/tracepoint.h | 40 ++++++--------------------------------
> kernel/tracepoint.c        | 10 +---------
> 2 files changed, 7 insertions(+), 43 deletions(-)
> 
> --
> 2.25.0.341.g760bfbb309-goog

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ