| Message ID | a95c38a253bedfa00d073e120a2599faa0f8139d.1661254155.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix race between quota enable and quota rescan ioctl | expand |
On Tue, Aug 23, 2022 at 12:45:42PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When enabling quotas, at btrfs_quota_enable(), after committing the > transaction, we change fs_info->quota_root to point to the quota root we > created and set BTRFS_FS_QUOTA_ENABLED at fs_info->flags. Then we try > to start the qgroup rescan worker, first by initalizing it with a call > to qgroup_rescan_init() - however if that fails we end up freeing the > quota root but we leave fs_info->quota_root still pointing to it, this > can later result in a use-after-free somewhere else. > > We have previously set the flags BTRFS_FS_QUOTA_ENABLED and > BTRFS_QGROUP_STATUS_FLAG_ON, so we can only fail with -EINPROGRESS at > btrfs_quota_enable(), which is possible if someone already called the > quota rescan ioctl, and therefore started the rescan worker. > > So fix this by ignoring an -EINPROGRESS and asserting we can't get any > other error. > > Reported-by: Ye Bin <yebin10@huawei.com> > Link: https://lore.kernel.org/linux-btrfs/20220823015931.421355-1-yebin10@huawei.com/ > Signed-off-by: Filipe Manana <fdmanana@suse.com> Added to misc-next, thanks.
On 2022/8/23 19:45, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When enabling quotas, at btrfs_quota_enable(), after committing the > transaction, we change fs_info->quota_root to point to the quota root we > created and set BTRFS_FS_QUOTA_ENABLED at fs_info->flags. Then we try > to start the qgroup rescan worker, first by initalizing it with a call > to qgroup_rescan_init() - however if that fails we end up freeing the > quota root but we leave fs_info->quota_root still pointing to it, this > can later result in a use-after-free somewhere else. > > We have previously set the flags BTRFS_FS_QUOTA_ENABLED and > BTRFS_QGROUP_STATUS_FLAG_ON, so we can only fail with -EINPROGRESS at > btrfs_quota_enable(), which is possible if someone already called the > quota rescan ioctl, and therefore started the rescan worker. > > So fix this by ignoring an -EINPROGRESS and asserting we can't get any > other error. > > Reported-by: Ye Bin <yebin10@huawei.com> > Link: https://lore.kernel.org/linux-btrfs/20220823015931.421355-1-yebin10@huawei.com/ > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/qgroup.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index db723c0026bd..ba323dcb0a0b 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -1174,6 +1174,21 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) > fs_info->qgroup_rescan_running = true; > btrfs_queue_work(fs_info->qgroup_rescan_workers, > &fs_info->qgroup_rescan_work); > + } else { > + /* > + * We have set both BTRFS_FS_QUOTA_ENABLED and > + * BTRFS_QGROUP_STATUS_FLAG_ON, so we can only fail with > + * -EINPROGRESS. That can happen because someone started the > + * rescan worker by calling quota rescan ioctl before we > + * attempted to initialize the rescan worker. Failure due to > + * quotas disabled in the meanwhile is not possible, because > + * we are holding a write lock on fs_info->subvol_sem, which > + * is also acquired when disabling quotas. > + * Ignore such error, and any other error would need to undo > + * everything we did in the transaction we just committed. > + */ > + ASSERT(ret == -EINPROGRESS); > + ret = 0; > } > > out_free_path:
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index db723c0026bd..ba323dcb0a0b 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1174,6 +1174,21 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) fs_info->qgroup_rescan_running = true; btrfs_queue_work(fs_info->qgroup_rescan_workers, &fs_info->qgroup_rescan_work); + } else { + /* + * We have set both BTRFS_FS_QUOTA_ENABLED and + * BTRFS_QGROUP_STATUS_FLAG_ON, so we can only fail with + * -EINPROGRESS. That can happen because someone started the + * rescan worker by calling quota rescan ioctl before we + * attempted to initialize the rescan worker. Failure due to + * quotas disabled in the meanwhile is not possible, because + * we are holding a write lock on fs_info->subvol_sem, which + * is also acquired when disabling quotas. + * Ignore such error, and any other error would need to undo + * everything we did in the transaction we just committed. + */ + ASSERT(ret == -EINPROGRESS); + ret = 0; } out_free_path: