linux-kernel - Re: [RFC PATCH 1/4] hazptr: Add initial implementation of hazard pointers

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: <28F2EAFF-1DF6-41EC-9DFC-3B437D67D840@gmail.com>
Date: Fri, 20 Sep 2024 00:10:03 +0800
From: Alan Huang <mmpgouride@...il.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Lai Jiangshan <jiangshanlai@...il.com>,
 LKML <linux-kernel@...r.kernel.org>,
 RCU <rcu@...r.kernel.org>,
 linux-mm@...ck.org,
 lkmm@...ts.linux.dev,
 "Paul E. McKenney" <paulmck@...nel.org>,
 Frederic Weisbecker <frederic@...nel.org>,
 Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
 Joel Fernandes <joel@...lfernandes.org>,
 Josh Triplett <josh@...htriplett.org>,
 "Uladzislau Rezki (Sony)" <urezki@...il.com>,
 Steven Rostedt <rostedt@...dmis.org>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Zqiang <qiang.zhang1211@...il.com>,
 Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>,
 Will Deacon <will@...nel.org>,
 Waiman Long <longman@...hat.com>,
 Mark Rutland <mark.rutland@....com>,
 Thomas Gleixner <tglx@...utronix.de>,
 Kent Overstreet <kent.overstreet@...il.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Vlastimil Babka <vbabka@...e.cz>,
 maged.michael@...il.com,
 Neeraj upadhyay <neeraj.upadhyay@....com>
Subject: Re: [RFC PATCH 1/4] hazptr: Add initial implementation of hazard
 pointers

2024年9月19日 15:10，Boqun Feng <boqun.feng@...il.com> wrote：
> 
> On Thu, Sep 19, 2024 at 02:39:13PM +0800, Lai Jiangshan wrote:
>> On Tue, Sep 17, 2024 at 10:34 PM Boqun Feng <boqun.feng@...il.com> wrote:
>> 
>>> +static void hazptr_context_snap_readers_locked(struct hazptr_reader_tree *tree,
>>> +                                              struct hazptr_context *hzcp)
>>> +{
>>> +       lockdep_assert_held(hzcp->lock);
>>> +
>>> +       for (int i = 0; i < HAZPTR_SLOT_PER_CTX; i++) {
>>> +               /*
>>> +                * Pairs with smp_store_release() in hazptr_{clear,free}().
>>> +                *
>>> +                * Ensure
>>> +                *
>>> +                * <reader>             <updater>
>>> +                *
>>> +                * [access protected pointers]
>>> +                * hazptr_clear();
>>> +                *   smp_store_release()
>>> +                *                      // in reader scan.
>>> +                *                      smp_load_acquire(); // is null or unused.
>>> +                *                      [run callbacks] // all accesses from
>>> +                *                                      // reader must be
>>> +                *                                      // observed.
>>> +                */
>>> +               hazptr_t val = smp_load_acquire(&hzcp->slots[i]);
>>> +
>>> +               if (!is_null_or_unused(val)) {
>>> +                       struct hazptr_slot_snap *snap = &hzcp->snaps[i];
>>> +
>>> +                       // Already in the tree, need to remove first.
>>> +                       if (!is_null_or_unused(snap->slot)) {
>>> +                               reader_del(tree, snap);
>>> +                       }
>>> +                       snap->slot = val;
>>> +                       reader_add(tree, snap);
>>> +               }
>>> +       }
>>> +}
>> 
>> Hello
>> 
>> I'm curious about whether there are any possible memory leaks here.
>> 
>> It seems that call_hazptr() never frees the memory until the slot is
>> set to another valid value.
>> 
>> In the code here, the snap is not deleted when hzcp->snaps[i] is null/unused
>> and snap->slot is not which I think it should be.
>> 
>> And it can cause unneeded deletion and addition of the snap if the slot
>> value is unchanged.
>> 
> 
> I think you're right. (Although the node will be eventually deleted at
> cleanup_hazptr_context(), however there could be a long-live
> hazptr_context). It should be:
> 
> hazptr_t val = smp_load_acquire(&hzcp->slots[i]);
> struct hazptr_slot_snap *snap = &hzcp->snaps[i];
> 
> if (val != snap->slot) { // val changed, need to update the tree node.
> // Already in the tree, need to remove first.
> if (!is_null_or_unused(snap->slot)) {
> reader_del(tree, snap);
> }
> 
> // use the latest snapshot.
> snap->slot = val;
> 
> // Add it into tree if there is a reader
> if (!is_null_or_unused(val))
> reader_add(tree, snap);
> }

Even using the same context, if two slots are used to protect the same pointer, let it be ptr1,
then if the second slot is reused for ptr2, ptr1’s callback will be invoked even the first slot still has the ptr1.

The original patch also has this problem.

> 
> Regards,
> Boqun
> 
>> I'm not so sure...
>> 
>> Thanks
>> Lai