linux-kernel - Re: [RFC 2/1] seqlock: make the read_seqbegin_or_lock() API more simple and less error-prone ?

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: <20251001115137.GB20441@redhat.com>
Date: Wed, 1 Oct 2025 13:51:37 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: David Howells <dhowells@...hat.com>
Cc: Boqun Feng <boqun.feng@...il.com>, Ingo Molnar <mingo@...hat.com>,
	Li RongQing <lirongqing@...du.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Waiman Long <longman@...hat.com>, Will Deacon <will@...nel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC 2/1] seqlock: make the read_seqbegin_or_lock() API more
 simple and less error-prone ?

On 09/30, David Howells wrote:
>
> It would also be nice to fix the static lock-balance detection stuff that you
> get when you enable advanced checking during a kernel build.  It doesn't
> seem to understand seqlocks.
>
> > 	-	nextseq = 0;
> > 	+	seq = 0;
>
> Perhaps an init function or macro that hides this bit?
>
> 	void init_read_seqlock(int *seq)
> 	{
> 		*seq = 0;
> 	}
>
> 	init_read_seqlock(&seq);
>
> or:
>
> 	#define INIT_READ_SEQBEGIN 0
>
> 	seq = INIT_READ_SEQBEGIN;

Agreed,

> Though if we can fold the whole loop inside a macro, that might make it easier
> to use.

Yes...

> d_walk() in fs/dcache.c might give you issues, though.

Yes. But note that it can use need_seqretry_xxx() too, see the patch below.

So. Do you think it makes any sense to add the new helper for the start?
Can you suggest a better name? Say, check_restart() ?

Oleg.

--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1355,7 +1355,7 @@ static void d_walk(struct dentry *parent, void *data,
 		spin_lock(&this_parent->d_lock);
 
 		/* might go back up the wrong parent if we have had a rename. */
-		if (need_seqretry(&rename_lock, seq))
+		if (need_seqretry_xxx(&rename_lock, &seq))
 			goto rename_retry;
 		/* go into the first sibling still alive */
 		hlist_for_each_entry_continue(dentry, d_sib) {
@@ -1366,7 +1366,7 @@ static void d_walk(struct dentry *parent, void *data,
 		}
 		goto ascend;
 	}
-	if (need_seqretry(&rename_lock, seq))
+	if (need_seqretry_xxx(&rename_lock, &seq))
 		goto rename_retry;
 	rcu_read_unlock();
 
@@ -1378,11 +1378,8 @@ static void d_walk(struct dentry *parent, void *data,
 rename_retry:
 	spin_unlock(&this_parent->d_lock);
 	rcu_read_unlock();
-	BUG_ON(seq & 1);
-	if (!retry)
-		return;
-	seq = 1;
-	goto again;
+	if (retry)
+		goto again;
 }
 
 struct check_mount {