linux-kernel - Re: [PATCH v2] buffer: Fix I/O error due to ARM read-after-read hazard

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: <CAHk-=wjmyEdYW4vEaNDP4UMB+H7wWneOwLUR3FmPG-Fb6U8dZg@mail.gmail.com>
Date:   Wed, 13 Nov 2019 08:36:40 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Will Deacon <will@...nel.org>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Vincent Whitchurch <vincent.whitchurch@...s.com>,
        Jens Axboe <axboe@...nel.dk>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Vincent Whitchurch <rabinv@...s.com>,
        Richard Earnshaw <Richard.Earnshaw@....com>
Subject: Re: [PATCH v2] buffer: Fix I/O error due to ARM read-after-read hazard

On Wed, Nov 13, 2019 at 2:24 AM Will Deacon <will@...nel.org> wrote:
>
> Ok, I'll stick my neck out here, but if test_bit() is being used to read
> a bitmap that is being concurrently modified (e.g. by set_bit() which boils
> down to atomic_long_or()), then why isn't READ_ONCE() required? Right now,
> test_bit takes a 'const volatile unsigned long *addr' argument, so I don't
> see that you'll get a change in codegen except on alpha and, with this
> erratum, arm32.

You're right. We've moved back to actually having it volatile (or
possibly never got away from it). My bad.

At one point, we tried very hard to make test_bit() perform much
better when you tested multiple bits, and I thought we ended up having
that merged and didn't want to regress. But apparently we never did
that, or it got undone.

test_bit() is a very unfortunate interface, in that we actually use it
in some situations where we _really_ would want to merge reads (not
split them, but merge them). There are several cases where we do
constant test-bits on the same word, and don't care about ordering.
Things like thread flags etc.

It's explicitly not ordered, so *merging* reads would be right and
lovely, it's the "don't read twice" that is bad. But we have no way to
tell the compiler that ;(

Anyway, READ_ONCE() is ok by me when I look at the code, because as
you say, it's already volatile (like my memory ;).

           Linus