Re: [PATCH v2 0/5] gfs2: protect sysfs callbacks from superblock teardown

[Andreas Gruenbacher <agruenba@redhat.com>]

Hello,

On Fri, Aug 21, 2026 at 6:09 AM Jiacheng Xu <stitch@zju.edu.cn> wrote:
> The GFS2 sysfs files become visible before fill_super() completes and
> remain present until after filesystem resources have been released.
> Consequently, callbacks that access quota, statfs, glock or journal
> state can race with mount failure rollback or unmount teardown.
>
> The quota refresh fix was originally sent as a standalone [PATCH]. This
> version folds it into a complete series and adds the corresponding
> lifetime protection for the other affected sysfs callbacks.
>
> All callbacks use down_read_trylock() on s_umount and verify SB_ACTIVE.
> Returning -EAGAIN avoids deadlock when mount failure or unmount holds
> the write side of s_umount while removing the sysfs files.

thanks for these patches, they look useful.  I would like to suggest
some changes; please see the below patch.  Could you please apply those
to the individual patches and repost?

> Changes in v2:
> - Folded the original quota refresh fix into a five-patch series.
> - Added statfs_sync, quota_sync, demote_rq and status fixes.
>
> Jiacheng Xu (5):
>   gfs2: protect quota refresh from superblock teardown
>   gfs2: protect statfs sync sysfs callback
>   gfs2: protect quota sync sysfs callback
>   gfs2: protect demote requests during superblock teardown
>   gfs2: protect status sysfs reads during teardown

With the patch I've just posted to the gfs2 mailing list [*], dumping
the status file shouldn't require any additional locking, so the last
patch in qour queue can be dropped.

[*] https://lore.kernel.org/gfs2/20260827190632.686583-1-agruenba@redhat.com/T/#u

Thanks,
Andreas

--

 fs/gfs2/quota.c | 19 +-------------
 fs/gfs2/sys.c   | 70 ++++++++++++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 51 deletions(-)

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index b1fb60b4cd10..dbfc21693dd5 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -1387,36 +1387,19 @@ int gfs2_quota_sync(struct super_block *sb, int type)
 
 int gfs2_quota_refresh(struct gfs2_sbd *sdp, struct kqid qid)
 {
-	struct super_block *sb = sdp->sd_vfs;
 	struct gfs2_quota_data *qd;
 	struct gfs2_holder q_gh;
 	int error;
 
-	/*
-	 * The sysfs files are created before fill_super completes. Avoid
-	 * blocking on s_umount because the mount failure path removes the
-	 * sysfs files while holding it for writing.
-	 */
-	if (!down_read_trylock(&sb->s_umount))
-		return -EAGAIN;
-
-	if (!(sb->s_flags & SB_ACTIVE)) {
-		error = -EAGAIN;
-		goto out_unlock;
-	}
-
 	error = qd_get(sdp, qid, &qd);
 	if (error)
-		goto out_unlock;
+		return error;
 
 	error = do_glock(qd, FORCE, &q_gh);
 	if (!error)
 		gfs2_glock_dq_uninit(&q_gh);
 
 	qd_put(qd);
-
-out_unlock:
-	up_read(&sb->s_umount);
 	return error;
 }
 
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 345d0527675f..ff90ec2eb798 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -63,20 +63,30 @@ static ssize_t id_show(struct gfs2_sbd *sdp, char *buf)
 			MAJOR(sdp->sd_vfs->s_dev), MINOR(sdp->sd_vfs->s_dev));
 }
 
+static bool super_trylock_shared_active(struct super_block *sb)
+{
+	if (!down_read_trylock(&sb->s_umount))
+		return false;
+	if (sb->s_flags & SB_ACTIVE)
+		return true;
+	up_read(&sb->s_umount);
+	return false;
+}
+
+static void super_unlock_active(struct super_block *sb)
+{
+	up_read(&sb->s_umount);
+}
+
 static ssize_t status_show(struct gfs2_sbd *sdp, char *buf)
 {
 	struct super_block *sb = sdp->sd_vfs;
 	unsigned long f;
 	ssize_t s;
 
-	if (!down_read_trylock(&sb->s_umount))
+	if (!super_trylock_shared_active(sb))
 		return -EAGAIN;
 
-	if (!(sb->s_flags & SB_ACTIVE)) {
-		s = -EAGAIN;
-		goto out_unlock;
-	}
-
 	f = sdp->sd_flags;
 	s = sysfs_emit(buf,
 		     "Journal Checked:          %d\n"
@@ -136,8 +146,7 @@ static ssize_t status_show(struct gfs2_sbd *sdp, char *buf)
 		     atomic_read(&sdp->sd_log_thresh1),
 		     atomic_read(&sdp->sd_log_thresh2));
 
-out_unlock:
-	up_read(&sb->s_umount);
+	super_unlock_active(sb);
 	return s;
 }
 
@@ -236,19 +245,13 @@ static ssize_t statfs_sync_store(struct gfs2_sbd *sdp, const char *buf,
 	if (val != 1)
 		return -EINVAL;
 
-	if (!down_read_trylock(&sb->s_umount))
+	if (!super_trylock_shared_active(sb))
 		return -EAGAIN;
 
-	if (!(sb->s_flags & SB_ACTIVE)) {
-		error = -EAGAIN;
-		goto out_unlock;
-	}
-
 	gfs2_statfs_sync(sb, 0);
 
-out_unlock:
-	up_read(&sb->s_umount);
-	return error ? error : len;
+	super_unlock_active(sb);
+	return len;
 }
 
 static ssize_t quota_sync_store(struct gfs2_sbd *sdp, const char *buf,
@@ -267,24 +270,19 @@ static ssize_t quota_sync_store(struct gfs2_sbd *sdp, const char *buf,
 	if (val != 1)
 		return -EINVAL;
 
-	if (!down_read_trylock(&sb->s_umount))
+	if (!super_trylock_shared_active(sb))
 		return -EAGAIN;
 
-	if (!(sb->s_flags & SB_ACTIVE)) {
-		error = -EAGAIN;
-		goto out_unlock;
-	}
-
 	gfs2_quota_sync(sb, 0);
 
-out_unlock:
-	up_read(&sb->s_umount);
-	return error ? error : len;
+	super_unlock_active(sb);
+	return len;
 }
 
 static ssize_t quota_refresh_user_store(struct gfs2_sbd *sdp, const char *buf,
 					size_t len)
 {
+	struct super_block *sb = sdp->sd_vfs;
 	struct kqid qid;
 	int error;
 	u32 id;
@@ -300,13 +298,19 @@ static ssize_t quota_refresh_user_store(struct gfs2_sbd *sdp, const char *buf,
 	if (!qid_valid(qid))
 		return -EINVAL;
 
+	if (!super_trylock_shared_active(sb))
+		return -EAGAIN;
+
 	error = gfs2_quota_refresh(sdp, qid);
+
+	super_unlock_active(sb);
 	return error ? error : len;
 }
 
 static ssize_t quota_refresh_group_store(struct gfs2_sbd *sdp, const char *buf,
 					 size_t len)
 {
+	struct super_block *sb = sdp->sd_vfs;
 	struct kqid qid;
 	int error;
 	u32 id;
@@ -322,7 +326,12 @@ static ssize_t quota_refresh_group_store(struct gfs2_sbd *sdp, const char *buf,
 	if (!qid_valid(qid))
 		return -EINVAL;
 
+	if (!super_trylock_shared_active(sb))
+		return -EAGAIN;
+
 	error = gfs2_quota_refresh(sdp, qid);
+
+	super_unlock_active(sb);
 	return error ? error : len;
 }
 
@@ -363,14 +372,9 @@ static ssize_t demote_rq_store(struct gfs2_sbd *sdp, const char *buf, size_t len
 	if (glops == NULL)
 		return -EINVAL;
 
-	if (!down_read_trylock(&sb->s_umount))
+	if (!super_trylock_shared_active(sb))
 		return -EAGAIN;
 
-	if (!(sb->s_flags & SB_ACTIVE)) {
-		rv = -EAGAIN;
-		goto out_unlock;
-	}
-
 	if (!test_and_set_bit(SDF_DEMOTE, &sdp->sd_flags))
 		fs_info(sdp, "demote interface used\n");
 	rv = gfs2_glock_get(sdp, glnum, glops, NO_CREATE, &gl);
@@ -381,7 +385,7 @@ static ssize_t demote_rq_store(struct gfs2_sbd *sdp, const char *buf, size_t len
 	rv = 0;
 
 out_unlock:
-	up_read(&sb->s_umount);
+	super_unlock_active(sb);
 	return rv ? rv : len;
 }
 
-- 
2.55.0