linux-kernel - Re: [PATCH RFC v5 1/6] softirq: reorder trace_softirqs

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: <CAJWu+op6MhP_i+6b6aqTtbEKmFvb3nX8qZMN0pkE_9EpJBJVSw@mail.gmail.com>
Date:   Tue, 01 May 2018 15:00:52 +0000
From:   Joel Fernandes <joelaf@...gle.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Tom Zanussi <tom.zanussi@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Boqun Feng <boqun.feng@...il.com>,
        Paul McKenney <paulmck@...ux.vnet.ibm.com>,
        "Cc: Frederic Weisbecker" <fweisbec@...il.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Fenguang Wu <fengguang.wu@...el.com>,
        Baohong Liu <baohong.liu@...el.com>,
        Vedang Patel <vedang.patel@...el.com>,
        "Cc: Android Kernel" <kernel-team@...roid.com>
Subject: Re: [PATCH RFC v5 1/6] softirq: reorder trace_softirqs_on to prevent
 lockdep splat

On Tue, May 1, 2018 at 7:02 AM Steven Rostedt <rostedt@...dmis.org> wrote:

> On Mon, 30 Apr 2018 18:41:59 -0700
> Joel Fernandes <joelaf@...gle.com> wrote:

> > I'm able to reproduce a lockdep splat when CONFIG_PROVE_LOCKING=y and
> > CONFIG_PREEMPTIRQ_EVENTS=y.

> Needs more info in the change log. It also requires that
> CONFIG_DEBUG_LOCKED=y is set.

> Add this to the change log:

> The issue was this:

> Start with: preempt_count = 1 << SOFTIRQ_SHIFT

>          __local_bh_enable(cnt = 1 << SOFTIRQ_SHIFT) {
>                  if (softirq_count() == (cnt && SOFTIRQ_MASK)) {
>                          trace_softirqs_on() {
>                                  current->softirqs_enabled = 1;
>                          }
>                  }
>                  preempt_count_sub(cnt) {
>                          trace_preempt_on() {
>                                  tracepoint() {
>                                          rcu_read_lock_sched() {
>                                                  // jumps into lockdep

> Where preempt_count still has softirqs disabled, but
> current->softirqs_enabled is true, and we get a splat.

> This patch should be separate (as you had it before), and needs to be
> submitted now because it already causes issues. We can add:

> Cc: stable@...r.kernel.org
> Fixes: d59158162e032 ("tracing: Add support for preempt and irq
enable/disable events")

Ok, I'll split and resubmit with your reasoning in the changelog.

Just to clarify, my changelog was based on older patches, that's why the
reason appears different but the root cause is the same. In an earlier
series, I was doing lockdep_{off,on} around the rcu_read_lock_sched call so
that stage wasn't splatting, but then after the read_lock is held, the
tracepoint probe such as those registered by preemptoff tracer  was
acquiring locks and causing the same splat.

Anyway, this just for some justification of why changelog appears to
present a different scenario with the same fix. But I'll change it to yours
and resubmit with the tags, thanks a lot for looking into it,

thanks,

- Joel