linux-kernel - Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq

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: <D8F8F41A-F785-4E17-83CE-4101137ADC5B@lca.pw>
Date:   Mon, 11 May 2020 12:44:26 -0400
From:   Qian Cai <cai@....pw>
To:     Will Deacon <will@...nel.org>
Cc:     "Paul E. McKenney" <paulmck@...nel.org>,
        Elver Marco <elver@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>
Subject: Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock



> On May 11, 2020, at 11:58 AM, Will Deacon <will@...nel.org> wrote:
> 
> I'm fine with the data_race() placement, but I don't find the comment
> very helpful. We assign the result of a READ_ONCE() to 'prev' in the
> loop, so I don't think that the cpu_relax() is really relevant.
> 
> The reason we don't need READ_ONCE() here is because if we race with
> the writer then either we'll go round the loop again after accidentally
> thinking prev->next != node, or we'll erroneously attempt the cmpxchg()
> because we thought they were equal and that will fail.
> 
> Make sense?

I think the significant concern from the previous reviews was if compilers could prove that prev->next == node was always true because it had no knowledge of the concurrency, and then took out the whole if statement away resulting in an infinite loop.

The comment tried to explain that the cpu_relax() would save us from the infinite loop in theory here.