linux-kernel - [PATCH] locking/pvqspinlock: Use try_cmpxchg() in pv

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

Message-Id: <20241223074704.18857-1-maobibo@loongson.cn>
Date: Mon, 23 Dec 2024 15:47:04 +0800
From: Bibo Mao <maobibo@...ngson.cn>
To: Ingo Molnar <mingo@...hat.com>,
	Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>
Cc: Boqun Feng <boqun.feng@...il.com>,
	linux-kernel@...r.kernel.org
Subject: [PATCH] locking/pvqspinlock: Use try_cmpxchg() in pv_unhash

We ported pv spinlock to old linux kernel on LoongArch platform, there is
error with some stress tests. The error report is something like this for
short:
 kernel BUG at kernel/locking/qspinlock_paravirt.h:261!
 Oops - BUG[#1]:
 CPU: 1 PID: 6613 Comm: pidof Not tainted 4.19.190+ #43
 Hardware name: Loongson KVM, BIOS 0.0.0 02/06/2015
    ra: 9000000000509cfc do_task_stat+0x29c/0xaf0
   ERA: 9000000000291308 __pv_queued_spin_unlock_slowpath+0xf8/0x100
  CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
  PRMD: 00000000 (PPLV0 -PIE -PWE)
         ...
 Call Trace:
 [<9000000000291308>] __pv_queued_spin_unlock_slowpath+0xf8/0x100
 [<9000000000509cf8>] do_task_stat+0x298/0xaf0
 [<9000000000502570>] proc_single_show+0x60/0xe0

The problem is that memory accessing is out of order on LoongArch
platform, there is contension between pv_unhash() and pv_hash().

CPU0 pv_unhash:                                CPU1 pv_hash:

  for_each_hash_entry(he, offset, hash) {        for_each_hash_entry(he, offset, hash) {
    if (READ_ONCE(he->lock) == lock) {             struct qspinlock *old = NULL;
      node = READ_ONCE(he->node);
      WRITE_ONCE(he->lock, NULL);

On LoongArch platform which is out of order, the execution order may be
switched like this:
>      WRITE_ONCE(he->lock, NULL);
                                                    if (try_cmpxchg(&he->lock, &old, lock)) {
                                                      WRITE_ONCE(he->node, node);
                                                      return &he->lock;

CPU1 pv_hash() is executing and watch that lock is set with NULL. Write
he->node with node of new lock.
>      node = READ_ONCE(he->node);
READ_ONCE(he->node) on CPU0 will return node of new lock rather than itself.

Here READ_ONCE/WRITE_ONCE is replaced with try_cmpxchg().

Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
---
 kernel/locking/qspinlock_paravirt.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
index dc1cb90e3644..cc4eabce092d 100644
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -240,9 +240,10 @@ static struct pv_node *pv_unhash(struct qspinlock *lock)
 	struct pv_node *node;
 
 	for_each_hash_entry(he, offset, hash) {
-		if (READ_ONCE(he->lock) == lock) {
+		struct qspinlock *old = lock;
+
+		if (try_cmpxchg(&he->lock, &old, NULL)) {
 			node = READ_ONCE(he->node);
-			WRITE_ONCE(he->lock, NULL);
 			return node;
 		}
 	}

base-commit: 48f506ad0b683d3e7e794efa60c5785c4fdc86fa
-- 
2.39.3