linux-kernel - Re: [RFC 0/2] srcu: Remove pre-flip memory barrier

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]

Date:   Tue, 20 Dec 2022 23:44:59 +0100
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     linux-kernel@...r.kernel.org,
        Josh Triplett <josh@...htriplett.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        "Paul E. McKenney" <paulmck@...nel.org>, rcu@...r.kernel.org,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [RFC 0/2] srcu: Remove pre-flip memory barrier

On Tue, Dec 20, 2022 at 09:20:08AM -0500, Joel Fernandes wrote:
> > On Dec 20, 2022, at 9:07 AM, Frederic Weisbecker <frederic@...nel.org> wrote:
> > 
> > On Tue, Dec 20, 2022 at 08:44:40AM -0500, Joel Fernandes wrote:
> >>> C w-depend-r
> >>> 
> >>> {
> >>>   PLOCK=LOCK0;
> >>> }
> >>> 
> >>> // updater
> >>> P0(int *LOCK1, int **PLOCK)
> >>> {
> >>>   int lock1;
> >>> 
> >>>   lock1 = READ_ONCE(*LOCK1); // READ from inactive idx
> >>>   smp_mb();
> >>>   WRITE_ONCE(*PLOCK, LOCK1); // Flip idx
> >>> }
> >>> 
> >>> // reader
> >>> P1(int **PLOCK)
> >>> {
> >>>   int *plock;
> >>> 
> >>>   plock = READ_ONCE(*PLOCK);    // Read active idx
> >>>   WRITE_ONCE(*plock, 1); // Write to active idx
> >> 
> >> I am a bit lost here, why would the reader want to write to the active idx?
> >> The reader does not update the idx, only the lock count.
> > 
> > So &ssp->sda->srcu_lock_count is the base address and idx is the offset, right?
> > The write is then displayed that way:
> > 
> >     this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
> > 
> > But things could be also thought the other way around with idx being the base address and
> > ssp->sda->srcu_lock_count being the offset.
> > 
> >     this_cpu_inc(idx[ssp->sda->srcu_lock_count].counter);
> > 
> > That would require to change some high level types but the result would be the same from
> > the memory point of view (and even from the ASM point of view). In the end we
> > are dealing with the same address and access.
> > 
> > Now ssp->sda->srcu_lock_count is a constant address value. It doesn't change.
> > So it can be zero for example. Then the above increment becomes:
> > 
> >   this_cpu_inc(idx.counter);
> > 
> > And then it can be modelized as in the above litmus test.
> > 
> > I had to play that trick because litmus doesn't support arrays but I believe
> > it stands. Now of course I may well have got something wrong since I've always
> > been terrible at maths...
> 
> Ah ok, I get where you were going with that. Yes there is address dependency
> between reading idx and writing lock count. But IMHO, the access on the update
> side is trying to order write to index, and reads from a lock count of a
> previous index (as far as E / B+C is concerned). So IMHO, on the read side you
> have to consider 2 consecutive readers and not the same reader in order to
> pair the same accesses correctly. But I could be missing something.

And you're right, for the first part of the comment (let's call that (1)):

	 * Ensure that if this updater saw a given reader's increment
	 * from __srcu_read_lock(), that reader was using an old value
	 * of ->srcu_idx.

My litmus test shows the ordering displayed in the second part of the comment
(call it (2)):

         * Also ensure that if a given reader sees the
	 * new value of ->srcu_idx, this updater's earlier scans cannot
	 * have seen that reader's increments (which is OK, because this
	 * grace period need not wait on that reader).

_ In (1), E indeed pairs with B and C
_ In (2), E pairs with the address-dependency between idx and lock_count.

Thanks.