2022-01-04 14:35:39

by Lukas Czerner

Subject: [PATCH 1/2] ext4: don't use kfree() on rcu protected pointer sbi->s_qf_names

During ext4 mount api rework the commit e6e268cb6822 ("ext4: move quota
configuration out of handle_mount_opt()") introduced a bug where we
would kfree(sbi->s_qf_names[i]) before assigning the new quota name in
ext4_apply_quota_options().

This is wrong because we're using kfree() on rcu prointer that could be
simultaneously accessed from ext4_show_quota_options() during remount.
Fix it by using rcu_replace_pointer() to replace the old qname with the
new one and then kfree_rcu() the old quota name.

Also use get_qf_name() instead of sbi->s_qf_names in strcmp() to silence
the sparse warning.

Fixes: e6e268cb6822 ("ext4: move quota configuration out of handle_mount_opt()")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Lukas Czerner <[email protected]>
---
fs/ext4/super.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b72d989b77fb..acb0c58cd3d1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2633,8 +2633,10 @@ static void ext4_apply_quota_options(struct fs_context *fc,

qname = ctx->s_qf_names[i]; /* May be NULL */
ctx->s_qf_names[i] = NULL;
- kfree(sbi->s_qf_names[i]);
- rcu_assign_pointer(sbi->s_qf_names[i], qname);
+ qname = rcu_replace_pointer(sbi->s_qf_names[i], qname,
+ lockdep_is_held(&sb->s_umount));
+ if (qname)
+ kfree_rcu(qname);
set_opt(sb, QUOTA);
}
}
@@ -2688,7 +2690,7 @@ static int ext4_check_quota_consistency(struct fs_context *fc,
goto err_jquota_change;

if (sbi->s_qf_names[i] && ctx->s_qf_names[i] &&
- strcmp(sbi->s_qf_names[i],
+ strcmp(get_qf_name(sb, sbi, i),
ctx->s_qf_names[i]) != 0)
goto err_jquota_specified;
}
--
2.31.1

2022-01-04 14:35:43

by Lukas Czerner

Subject: [PATCH 2/2] ext4: only set EXT4_MOUNT_QUOTA when journalled quota file is specified

Only set EXT4_MOUNT_QUOTA when journalled quota file is specified,
otherwise simply disabling specific quota type (usrjquota=) will also
set the EXT4_MOUNT_QUOTA super block option.

Signed-off-by: Lukas Czerner <[email protected]>
Fixes: e6e268cb6822 ("ext4: move quota configuration out of handle_mount_opt()")
---
fs/ext4/super.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index acb0c58cd3d1..52e0be447b9f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2632,12 +2632,13 @@ static void ext4_apply_quota_options(struct fs_context *fc,
continue;

qname = ctx->s_qf_names[i]; /* May be NULL */
+ if (qname)
+ set_opt(sb, QUOTA);
ctx->s_qf_names[i] = NULL;
qname = rcu_replace_pointer(sbi->s_qf_names[i], qname,
lockdep_is_held(&sb->s_umount));
if (qname)
kfree_rcu(qname);
- set_opt(sb, QUOTA);
}
}

--
2.31.1

2022-01-05 03:14:49

by Theodore Ts'o

Subject: Re: [PATCH 1/2] ext4: don't use kfree() on rcu protected pointer sbi->s_qf_names

On Tue, 4 Jan 2022 15:35:17 +0100, Lukas Czerner wrote:
> During ext4 mount api rework the commit e6e268cb6822 ("ext4: move quota
> configuration out of handle_mount_opt()") introduced a bug where we
> would kfree(sbi->s_qf_names[i]) before assigning the new quota name in
> ext4_apply_quota_options().
>
> This is wrong because we're using kfree() on rcu prointer that could be
> simultaneously accessed from ext4_show_quota_options() during remount.
> Fix it by using rcu_replace_pointer() to replace the old qname with the
> new one and then kfree_rcu() the old quota name.
>
> [...]

Applied, thanks!

[1/2] ext4: don't use kfree() on rcu protected pointer sbi->s_qf_names
commit: e1577876127c1e6827225997b64ef3577a4afcf3
[2/2] ext4: only set EXT4_MOUNT_QUOTA when journalled quota file is specified
commit: d2717c29596304ada9edb78959baed8e0977018f

Best regards,
--
Theodore Ts'o <[email protected]>