Re: [PATCH] sysv: convert pointers_lock from rw_lock to rw_sem

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Mar 27, 2023 at 07:24:25AM +0900, Tetsuo Handa wrote:
> syzbot is reporting that __getblk_gfp() cannot be called from atomic
> context. Fix this problem by converting pointers_lock from rw_lock to
> rw_sem.
> 
> Reported-by: syzbot <syzbot+69b40dc5fd40f32c199f@syzkaller.appspotmail.com>
> Link: https://syzkaller.appspot.com/bug?extid=69b40dc5fd40f32c199f
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+69b40dc5fd40f32c199f@syzkaller.appspotmail.com>

Hmm...  The bug is real, all right (introduced back in 2002 during the
conversion away from BKL;
commit 3ba77f860fa7f359660e9d498a5b35940021cfba
Author: Christoph Hellwig <hch@sb.bsdonline.org>
Date:   Thu Apr 4 00:25:37 2002 +0200

    Replace BKL for chain locking with sysvfs-private rwlock.
is where it had happened).

However, I don't think this is the right fix.  Note that the problem is
with get_branch() done under the rwlock; all other places are safe.  But
in get_branch() we only need the lock (and only shared, at that) around
the verify+add pair.  See how it's done in fs/minix/itree_common.c...
Something like this:

diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
index b22764fe669c..cfa281fb6578 100644
--- a/fs/sysv/itree.c
+++ b/fs/sysv/itree.c
@@ -104,15 +104,18 @@ static Indirect *get_branch(struct inode *inode,
 		bh = sb_bread(sb, block);
 		if (!bh)
 			goto failure;
+		read_lock(&pointers_lock);
 		if (!verify_chain(chain, p))
 			goto changed;
 		add_chain(++p, bh, (sysv_zone_t*)bh->b_data + *++offsets);
+		read_unlock(&pointers_lock);
 		if (!p->key)
 			goto no_block;
 	}
 	return NULL;
 
 changed:
+	read_unlock(&pointers_lock);
 	brelse(bh);
 	*err = -EAGAIN;
 	goto no_block;
@@ -214,9 +217,7 @@ static int get_block(struct inode *inode, sector_t iblock, struct buffer_head *b
 		goto out;
 
 reread:
-	read_lock(&pointers_lock);
 	partial = get_branch(inode, depth, offsets, chain, &err);
-	read_unlock(&pointers_lock);
 
 	/* Simplest case - block found, no allocation needed */
 	if (!partial) {
@@ -287,10 +288,11 @@ static Indirect *find_shared(struct inode *inode,
 	for (k = depth; k > 1 && !offsets[k-1]; k--)
 		;
 
-	write_lock(&pointers_lock);
 	partial = get_branch(inode, k, offsets, chain, &err);
 	if (!partial)
 		partial = chain + k-1;
+
+	write_lock(&pointers_lock);
 	/*
 	 * If the branch acquired continuation since we've looked at it -
 	 * fine, it should all survive and (new) top doesn't belong to us.