linux-kernel - [RFC 1/2] srcu: Remove comment about prior read lock counts

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: <20221218191310.130904-2-joel@joelfernandes.org>
Date:   Sun, 18 Dec 2022 19:13:08 +0000
From:   "Joel Fernandes (Google)" <joel@...lfernandes.org>
To:     linux-kernel@...r.kernel.org
Cc:     "Joel Fernandes (Google)" <joel@...lfernandes.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: [RFC 1/2] srcu: Remove comment about prior read lock counts

The comment says that if an updater saw lock count updates, then
ensure the reader does not see the new srcu_idx.

However, there is no memory barrier between a READER reading srcu_idx
with respect to incrementing the lock count for that srcu_idx.

So what is really happening is, both "B" and "C" will order the current
reader's unlock count update, and the _next_ readers lock count update, with
respect to the write to the currently active index.

Consider first the case of the unlock count update being seen by the UPDATER:

(for brevity, the pseudocode shortens "srcu_idx" to "idx")

READER                            UPDATER

rcu_read_lock() {
    idx = READ(idx);
    lock_count[idx]++;

    smp_mb();    // B
}
                                srcu_flip() {
                                    smp_mb(); //E
                                    idx++;
                                    smp_mb();
                                }
rcu_read_unlock() {
    smp_mb();    // C
    unlock_count[idx]++;
}

Consider that the updater saw the unlock count update, and due to this, we
expect "E" to make sure that the reader only used the old srcu_idx.

However, say the reader used the new srcu_idx because we dropped "E".  That is
totally OK because both unlock and lock counts of this reader will negate each
other during the next scan of the srcu_idx. So we don't have to guarantee at
all that the reader used the old srcu_idx, that does not buy us anything
because if it used the new one, we would just ignore it during the next scan
anyway (the reader is "done").

Now lets look at the following case:

READER                            UPDATER

rcu_read_lock() {
    idx = READ(idx);
    lock_count[idx]++;

    smp_mb();    // B
}

rcu_read_unlock() {
    smp_mb();    // C
    unlock_count[idx]++;
}
                                srcu_flip() {
                                    smp_mb(); //E
                                    idx++;
rcu_read_lock() {
    idx = READ(idx);
    lock_count[idx]++;

    smp_mb();    // B
                                    smp_mb();
                                }
}

Consider that the updater saw the lock count update of the second
rcu_read_lock(). It does not matter that we guarantee that the reader sees only
the old srcu_idx. This is because, a reader could totally just sample
srcu_idx, and stay preempted for long periods of time. So, during any scan, we
already have the issue of a preempted-reader randomly springing up with a copy
of the index which we consider the "new index". So guaranteeing that the reader
saw the old srcu_idx instead of the new one if we saw its lock count updates,
also does not buy us anything.

Due to these reasons, drop the argument that the reader has to see a certain
srcu_idx since we have no control over that anyway, and guaranteeing that does not
buy us anything.

Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
---
 kernel/rcu/srcutree.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..d6a4c2439ca6 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -983,12 +983,10 @@ static bool try_check_zero(struct srcu_struct *ssp, int idx, int trycount)
 static void srcu_flip(struct srcu_struct *ssp)
 {
 	/*
-	 * 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.  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).
+	 * 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).
 	 */
 	smp_mb(); /* E */  /* Pairs with B and C. */

-- 
2.39.0.314.g84b9a713c41-goog