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: <38b04b86-1e85-40f0-8174-3c8ab29cbcaf@efficios.com>
Date: Wed, 25 Sep 2024 13:36:47 +0200
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Jonas Oberhauser <jonas.oberhauser@...weicloud.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

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.

> Alternatively you could always use an acquire load.

Unless someone comes up with a sound alternate approach,
I am tempted to go with an acquire load as the second load
within hpref_hp_get().

This way, the compiler would not attempt to use the
node value from the first load for dependent loads,
and the and CPU won't try to speculate dependent loads
either.

Thanks,

Mathieu

> 
> 
> Best wishes,
> 
>    jonas
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ