linux-kernel - Re: [RFC PATCH 1/1] hpref: Hazard Pointers with Reference Counter

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] [day] [month] [year] [list]

Message-ID: <b3b9e8e8-a3b5-4398-92b9-ff46b8ec53a9@huaweicloud.com>
Date: Wed, 25 Sep 2024 14:02:44 +0200
From: Jonas Oberhauser <jonas.oberhauser@...weicloud.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Boqun Feng <boqun.feng@...il.com>, "Paul E. McKenney" <paulmck@...nel.org>
Cc: linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>, Alan Stern
 <stern@...land.harvard.edu>, John Stultz <jstultz@...gle.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Frederic Weisbecker <frederic@...nel.org>,
 Joel Fernandes <joel@...lfernandes.org>,
 Josh Triplett <josh@...htriplett.org>, Uladzislau Rezki <urezki@...il.com>,
 Steven Rostedt <rostedt@...dmis.org>, Lai Jiangshan
 <jiangshanlai@...il.com>, Zqiang <qiang.zhang1211@...il.com>,
 Ingo Molnar <mingo@...hat.com>, Waiman Long <longman@...hat.com>,
 Mark Rutland <mark.rutland@....com>, Thomas Gleixner <tglx@...utronix.de>,
 Vlastimil Babka <vbabka@...e.cz>, maged.michael@...il.com,
 Mateusz Guzik <mjguzik@...il.com>, rcu@...r.kernel.org, linux-mm@...ck.org,
 lkmm@...ts.linux.dev
Subject: Re: [RFC PATCH 1/1] hpref: Hazard Pointers with Reference Counter

Am 9/25/2024 um 1:36 PM schrieb Mathieu Desnoyers:
> On 2024-09-25 12:06, Jonas Oberhauser wrote:
>>
>>
>> Am 9/25/2024 um 8:35 AM schrieb Mathieu Desnoyers:
>>> On 2024-09-25 07:57, Jonas Oberhauser wrote:
>>>> Hi Mathieu,
>>
>>>> I haven't read your code in detail but it seems to me you have an 
>>>> ABA bug: as I explained elsewhere, you could read the same pointer 
>>>> after ABA but you don't synchronize with the newer store that gave 
>>>> you node2, leaving you to speculatively read stale values through 
>>>> *ctx->hp.
>>>> (I am assuming here that ctx->hp is essentially an out parameter 
>>>> used to let the caller know which node got protected).
>>>
>>> The following change should fix it:
>>>
>>>       cmm_barrier();
>>> -    node2 = uatomic_load(node_p, CMM_RELAXED);    /* Load A */
>>> +    node2 = rcu_dereference(*node_p);    /* Load A */
>>>
>>
>> I don't think this fixes it, because IIRC rcu_dereference relies on 
>> the address dependency (which we don't have here) to provide ordering.
>>
>> I would recommend either:
>>
>> -    ctx->hp = node;
>> +    ctx->hp = node2;
>>
>> which fixes the problem under the perhaps too weak assumption that the 
>> compiler doesn't use its knowledge that node==node2 to just undo this 
>> fix, or more strictly,
> 
> As stated in Documentation/RCU/rcu_dereference.rst from the Linux
> kernel, comparing the result of rcu_dereference against another
> non-NULL pointer is discouraged, as you rightly point out.
> 
>>
>> +    ctx->hp = READ_ONCE(node2);
>>
>> which I believe makes sure that the value of node2 is used.
> 
> I am not entirely sure this extra READ_ONCE() would be sufficient
> to prevent the compiler from making assumptions about the content
> of node2 and thus use the result of the first load (node) instead.
> It would also not suffice to prevent the CPU from speculatively
> using the result of the first load to perform dependent loads AFAIU.

The reason I think it should be sufficient is that it forces the 
compiler to assume that since the comparison, the value of node2 might 
have changed.
Therefore, simply using the value of node at that point should be 
unsound from the compiler's POV.

But I'm not a compiler expert... So I definitely support uneasiness 
about this construct :))

jonas