linux-kernel - Re: [PATCH] kernel: Introduce a write lock/unlock wrapper for tasklist

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: <ZY06xtvaQ4ZJ5dXa@casper.infradead.org>
Date: Thu, 28 Dec 2023 09:07:18 +0000
From: Matthew Wilcox <willy@...radead.org>
To: Hillf Danton <hdanton@...a.com>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>,
	Maria Yu <quic_aiquny@...cinc.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernel: Introduce a write lock/unlock wrapper for
 tasklist_lock

On Wed, Dec 27, 2023 at 07:07:27PM +0800, Hillf Danton wrote:
> Feel free to ignore the following leg works.
> 
> 	/* Set the waiting flag to notify readers that a writer is pending */
> 	atomic_or(_QW_WAITING, &lock->cnts);
> 
> 	enable irq;
> 
> 	/* When no more readers or writers, set the locked flag */
> 	do {
> 		cnts = atomic_cond_read_relaxed(&lock->cnts, VAL == _QW_WAITING);
> 	} while (!atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED));
> 
> 	int
> 	atomic_cond_read_acquire(&lock->cnts, !(VAL & _QW_LOCKED));
> 	deadlock
> 	disable irq;

That would be a buggy implementation, and would not be what I was
thinking.

> Though the case below is safe, it looks not pretty but clumsy.
> 
> 	/* Set the waiting flag to notify readers that a writer is pending */
> 	atomic_or(_QW_WAITING, &lock->cnts);
> 
> 	/* When no more readers or writers, set the locked flag */
> 	do {
> 		enable irq;
> 
> 		cnts = atomic_cond_read_relaxed(&lock->cnts, VAL == _QW_WAITING);
> 
> 		disable irq;
> 
> 	} while (!atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED));

Why do you think it looks clumsy?  It's more or less what I was
thinking.

-void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock)
+void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock, bool irq)
...
        do {
+               if (irq)
+                       local_irq_enable();
                cnts = atomic_cond_read_relaxed(&lock->cnts, VAL == _QW_WAITING);
+               if (irq)
+                       local_irq_disable();
        } while (!atomic_try_cmpxchg_acquire(&lock->cnts, &cnts, _QW_LOCKED));