| Message ID | 20230608141805.1434230-1-tytso@mit.edu |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [1/2] Revert "ext4: don't clear SB_RDONLY when remounting r/w until quota is re-enabled" | expand |
On Thu 08-06-23 10:18:04, Theodore Ts'o wrote: > This reverts commit a44be64bbecb15a452496f60db6eacfee2b59c79. > > Link: https://lore.kernel.org/r/653b3359-2005-21b1-039d-c55ca4cffdcc@gmail.com > Signed-off-by: Theodore Ts'o <tytso@mit.edu> So I was looking more into how the warning in xattr code can trigger. Sadly the syzbot reproducer does not seem to reproduce the issue for me when I enable the warnings in xattr code. Anyway, after staring for some time into the code I think the problem has actually been introduced by the transition to the new mount API. Because in the old API, userspace could not start writes to the filesystem until we called set_mount_attributes() in do_remount() which cleared the MNT_READONLY flag on the mount (which happens after reconfigure_super(). In the new API, fsconfig(2) syscall allows you to toggle SB_RDONLY flag without touching the mount flags and so we can have userspace writing the filesystem as soon as we clear SB_RDONLY flag. I have checked and the other direction (i.e., remount read-only) is properly serialized in the VFS by setting sb->s_readonly_remount (and making sure we either return EBUSY or all writers are going to see s_readonly_remount set) before calling into filesystem's reconfigure code. I have actually a fixup within ext4 ready but I think it may be better to fix this within VFS and provide similar serialization like for the rw-ro transition there... Let's see what VFS people say to this. Honza
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 56a5d1c469fc..05fcecc36244 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -6388,7 +6388,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb) struct ext4_mount_options old_opts; ext4_group_t g; int err = 0; - int enable_rw = 0; #ifdef CONFIG_QUOTA int enable_quota = 0; int i, j; @@ -6575,7 +6574,7 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb) if (err) goto restore_opts; - enable_rw = 1; + sb->s_flags &= ~SB_RDONLY; if (ext4_has_feature_mmp(sb)) { err = ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block)); @@ -6622,9 +6621,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb) if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks) ext4_release_system_zone(sb); - if (enable_rw) - sb->s_flags &= ~SB_RDONLY; - /* * Reinitialize lazy itable initialization thread based on * current settings
This reverts commit a44be64bbecb15a452496f60db6eacfee2b59c79. Link: https://lore.kernel.org/r/653b3359-2005-21b1-039d-c55ca4cffdcc@gmail.com Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/ext4/super.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)