| Message ID | 20181119162034.13132-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: fix race between enabling quotas and subvolume creation | expand |
On 2018/11/20 上午12:20, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > We have a race between enabling quotas end subvolume creation that cause > subvolume creation to fail with -EINVAL, and the following diagram shows > how it happens: > > CPU 0 CPU 1 > > btrfs_ioctl() > btrfs_ioctl_quota_ctl() > btrfs_quota_enable() > mutex_lock(fs_info->qgroup_ioctl_lock) > > btrfs_ioctl() > create_subvol() > btrfs_qgroup_inherit() > -> save fs_info->quota_root > into quota_root > -> stores a NULL value > -> tries to lock the mutex > qgroup_ioctl_lock > -> blocks waiting for > the task at CPU0 > > -> sets BTRFS_FS_QUOTA_ENABLED in fs_info > -> sets quota_root in fs_info->quota_root > (non-NULL value) > > mutex_unlock(fs_info->qgroup_ioctl_lock) > > -> checks quota enabled > flag is set > -> returns -EINVAL because > fs_info->quota_root was > NULL before it acquired > the mutex > qgroup_ioctl_lock > -> ioctl returns -EINVAL > > Returning -EINVAL to user space will be confusing if all the arguments > passed to the subvolume creation ioctl were valid. > > Fix it by grabbing the value from fs_info->quota_root after acquiring > the mutex. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/qgroup.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index ae1358253b7b..0bdf28499790 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -2250,7 +2250,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid, > int i; > u64 *i_qgroups; > struct btrfs_fs_info *fs_info = trans->fs_info; > - struct btrfs_root *quota_root = fs_info->quota_root; > + struct btrfs_root *quota_root; > struct btrfs_qgroup *srcgroup; > struct btrfs_qgroup *dstgroup; > u32 level_size = 0; > @@ -2260,6 +2260,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid, > if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) > goto out; > > + quota_root = fs_info->quota_root; > if (!quota_root) { > ret = -EINVAL; > goto out; >
On Mon, Nov 19, 2018 at 04:20:34PM +0000, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > We have a race between enabling quotas end subvolume creation that cause > subvolume creation to fail with -EINVAL, and the following diagram shows > how it happens: > > CPU 0 CPU 1 > > btrfs_ioctl() > btrfs_ioctl_quota_ctl() > btrfs_quota_enable() > mutex_lock(fs_info->qgroup_ioctl_lock) > > btrfs_ioctl() > create_subvol() > btrfs_qgroup_inherit() > -> save fs_info->quota_root > into quota_root > -> stores a NULL value > -> tries to lock the mutex > qgroup_ioctl_lock > -> blocks waiting for > the task at CPU0 > > -> sets BTRFS_FS_QUOTA_ENABLED in fs_info > -> sets quota_root in fs_info->quota_root > (non-NULL value) > > mutex_unlock(fs_info->qgroup_ioctl_lock) > > -> checks quota enabled > flag is set > -> returns -EINVAL because > fs_info->quota_root was > NULL before it acquired > the mutex > qgroup_ioctl_lock > -> ioctl returns -EINVAL > > Returning -EINVAL to user space will be confusing if all the arguments > passed to the subvolume creation ioctl were valid. > > Fix it by grabbing the value from fs_info->quota_root after acquiring > the mutex. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index ae1358253b7b..0bdf28499790 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -2250,7 +2250,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid, int i; u64 *i_qgroups; struct btrfs_fs_info *fs_info = trans->fs_info; - struct btrfs_root *quota_root = fs_info->quota_root; + struct btrfs_root *quota_root; struct btrfs_qgroup *srcgroup; struct btrfs_qgroup *dstgroup; u32 level_size = 0; @@ -2260,6 +2260,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid, if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) goto out; + quota_root = fs_info->quota_root; if (!quota_root) { ret = -EINVAL; goto out;