linux-kernel - Re: BUG: KCSAN: data-race in tick_nohz_next_event / tick_nohz_stop

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: <87wnxw86bv.fsf@nanos.tec.linutronix.de>
Date:   Sat, 05 Dec 2020 19:18:28 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Marco Elver <elver@...gle.com>,
        Naresh Kamboju <naresh.kamboju@...aro.org>
Cc:     open list <linux-kernel@...r.kernel.org>,
        kasan-dev <kasan-dev@...glegroups.com>, rcu@...r.kernel.org,
        lkft-triage@...ts.linaro.org,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Ingo Molnar <mingo@...nel.org>, fweisbec@...il.com,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: BUG: KCSAN: data-race in tick_nohz_next_event / tick_nohz_stop_tick

On Fri, Dec 04 2020 at 20:53, Marco Elver wrote:
> On Fri, 4 Dec 2020 at 20:04, Naresh Kamboju <naresh.kamboju@...aro.org> wrote:
>> LKFT started testing KCSAN enabled kernel from the linux next tree.
>> Here we have found BUG: KCSAN: data-race in tick_nohz_next_event /
>> tick_nohz_stop_tick
>
> Thank you for looking into KCSAN. Would it be possible to collect
> these reports in a moderation queue for now?

Yes please. This is the forth or fifth incarnation of report for that
data race in the tick code and I just did not come around to work on it.

> I'm currently trying to work out a strategy on how to best proceed
> with all the data races in the kernel. We do know there are plenty. On

I think having a central point where the reports are collected, i.e. a
moderation queue, is a good start. Reports like the one at hand should
stick out because they should reproduce pretty instantanious as it's an
intentional one and on NOHZ=y machines where CPUs are not fully loaded
its hard not to detect it :)

> The report below looks to be of type (A). Generally, the best strategy
> for resolving these is to send a patch, and not a report. However, be
> aware that sometimes it is really quite difficult to say if we're
> looking at a type (A) or (B) issue, in which case it may still be fair
> to send a report and briefly describe what you think is happening
> (because that'll increase the likelihood of getting a response). I
> recommend also reading "Developer/Maintainer data-race strategies" in
> https://lwn.net/Articles/816854/ -- specifically note "[...] you
> should not respond to KCSAN reports by mindlessly adding READ_ONCE(),
> data_race(), and WRITE_ONCE(). Instead, a patch addressing a KCSAN
> report must clearly identify the fix's approach and why that approach
> is appropriate."

Yes. I've seen a fair amount of 'Fix KCSAN warnings' patches which just
slap READ/WRITE_ONCE() all over the place to shut it up without any
justification. Most of them ended in limbo when asking for that
justification.

But the problem is that it is not necessarily trivial to understand code
when there are intentional data races without a lot of comments - guilty
as charged in this case. I actually felt so guilty that I sat down and
annotated and documented it now. Took me quite some time to comment all
the racy reads correctly as I really had to think about each of them
carefully again.

OTOH, in general it's a good exercise for reporters to do such analysis
and maintainers are happy to help when the analysis is not entirely
correct or comes to the wrong conclusion, e.g. assuming type B when it's
actually A. That's way better than just reports or mechanical "paper
over it" patches.

Just getting the reports over and over is not going to solve anything
because as in this case there is always more important stuff to do and
to the people familiar with the code it's clear that it's A and
therefore not urgent.

But that causes the problem that the A types are staying around for a
long time and blend over the B/C issues which are the real interesting
ones.

> This report should have line numbers, otherwise it's impossible to say
> which accesses are racing.

I just had to look at the function names to know that it is about:

tick_do_timer_cpu :)

> [ For those curious, this is the same report on syzbot's moderation
> queue, with line numbers:
> https://syzkaller.appspot.com/bug?id=d835c53d1a5e27922fcd1fbefc926a74790156cb
> ]

Confirmed :)

So you have quite some of the same report collected and there are a few
other patterns which are all related to tick_do_timer_cpu, so I assume
there is a stash of the other variants as well. And indeed:

 https://syzkaller.appspot.com/bug?id=03911d1370705fe3667dae48c9cda46d982cea30
 https://syzkaller.appspot.com/bug?id=440c51f56c3f3923f9b364679da48b0c1a0bdfe7

It might be useful to find the actual variable, data member or whatever
which is involved in the various reports and if there is a match then
the reports could be aggregated. The 3 patterns here are not even the
complete possible picture.

So if you sum them up: 58 + 148 + 205 instances then their weight
becomes more significant as well.

/me goes back to read the tick_do_timer_cpu comments once more before
posting.

Thanks,

        tglx