* Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted
2022-03-23 7:10 ` [PATCH v2] " Kaiwen Hu
@ 2022-03-23 7:59 ` Qu Wenruo
2022-03-23 12:34 ` David Sterba
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Qu Wenruo @ 2022-03-23 7:59 UTC (permalink / raw)
To: Kaiwen Hu, linux-btrfs, dsterba; +Cc: robbieko, cccheng, seanding
On 2022/3/23 15:10, Kaiwen Hu wrote:
> This patch prevent subvol being deleted when the subvol contains
> any active swapfile.
>
> Since the subvolume is deleted, we cannot swapoff the swapfile in
> this deleted subvolume. However, the swapfile is still active,
> we unable to unmount this volume. Let it into some deadlock
> situation.
>
> The test looks like this:
> mkfs.btrfs -f $dev > /dev/null
> mount $dev $mnt
>
> btrfs sub create $mnt/subvol
> touch $mnt/subvol/swapfile
> chmod 600 $mnt/subvol/swapfile
> chattr +C $mnt/subvol/swapfile
> dd if=/dev/zero of=$mnt/subvol/swapfile bs=1K count=4096
> mkswap $mnt/subvol/swapfile
> swapon $mnt/subvol/swapfile
>
> btrfs sub delete $mnt/subvol
> swapoff $mnt/subvol/swapfile // failed: No such file or directory
> swapoff --all
>
> unmount $mnt // target is busy.
>
> To prevent above issue, we simply check that whether the subvolume
> contains any active swapfile, and stop the deleting process. This
> behavior is like snapshot ioctl dealing with a swapfile.
>
> Reviewed-by: Robbie Ko <robbieko@synology.com>
> Signed-off-by: Kaiwen Hu <kevinhu@synology.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
>
> Add comment on it.
>
> fs/btrfs/inode.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5bbea5ec31fc..b32def311f44 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4460,6 +4460,12 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
> dest->root_key.objectid);
> return -EPERM;
> }
> + if (atomic_read(&dest->nr_swapfiles)) {
> + spin_unlock(&dest->root_item_lock);
> + btrfs_warn(fs_info,
> + "attempt to delete subvolume with active swapfile");
> + return -ETXTBSY;
> + }
> root_flags = btrfs_root_flags(&dest->root_item);
> btrfs_set_root_flags(&dest->root_item,
> root_flags | BTRFS_ROOT_SUBVOL_DEAD);
> @@ -10418,8 +10424,22 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
> * set. We use this counter to prevent snapshots. We must increment it
> * before walking the extents because we don't want a concurrent
> * snapshot to run after we've already checked the extents.
> + *
> + * It is possible that subvolume is marked for deletion but still not
> + * remove yet. To prevent this race, we check the root's mark before
> + * activating swapfile.
> */
> + spin_lock(&root->root_item_lock);
> + if (btrfs_root_dead(root)) {
> + spin_unlock(&root->root_item_lock);
> + btrfs_exclop_finish(fs_info);
> + btrfs_warn(fs_info,
> + "cannot activate swapfile because subvolume is marked for deletion");
> + return -EINVAL;
> + }
> atomic_inc(&root->nr_swapfiles);
> + spin_unlock(&root->root_item_lock);
> +
>
> isize = ALIGN_DOWN(inode->i_size, fs_info->sectorsize);
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted
2022-03-23 7:10 ` [PATCH v2] " Kaiwen Hu
2022-03-23 7:59 ` Qu Wenruo
@ 2022-03-23 12:34 ` David Sterba
2022-03-23 13:33 ` Filipe Manana
2022-03-23 13:37 ` Filipe Manana
2022-03-23 21:45 ` David Sterba
3 siblings, 1 reply; 12+ messages in thread
From: David Sterba @ 2022-03-23 12:34 UTC (permalink / raw)
To: Kaiwen Hu
Cc: quwenruo.btrfs, linux-btrfs, dsterba, robbieko, cccheng, seanding
On Wed, Mar 23, 2022 at 03:10:32PM +0800, Kaiwen Hu wrote:
> This patch prevent subvol being deleted when the subvol contains
> any active swapfile.
>
> Since the subvolume is deleted, we cannot swapoff the swapfile in
> this deleted subvolume. However, the swapfile is still active,
> we unable to unmount this volume. Let it into some deadlock
> situation.
>
> The test looks like this:
> mkfs.btrfs -f $dev > /dev/null
> mount $dev $mnt
>
> btrfs sub create $mnt/subvol
> touch $mnt/subvol/swapfile
> chmod 600 $mnt/subvol/swapfile
> chattr +C $mnt/subvol/swapfile
> dd if=/dev/zero of=$mnt/subvol/swapfile bs=1K count=4096
> mkswap $mnt/subvol/swapfile
> swapon $mnt/subvol/swapfile
>
> btrfs sub delete $mnt/subvol
> swapoff $mnt/subvol/swapfile // failed: No such file or directory
> swapoff --all
>
> unmount $mnt // target is busy.
>
> To prevent above issue, we simply check that whether the subvolume
> contains any active swapfile, and stop the deleting process. This
> behavior is like snapshot ioctl dealing with a swapfile.
>
> Reviewed-by: Robbie Ko <robbieko@synology.com>
> Signed-off-by: Kaiwen Hu <kevinhu@synology.com>
> ---
>
> Add comment on it.
>
> fs/btrfs/inode.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5bbea5ec31fc..b32def311f44 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4460,6 +4460,12 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
> dest->root_key.objectid);
> return -EPERM;
> }
> + if (atomic_read(&dest->nr_swapfiles)) {
This is under the root_item_lock and the increment in
btrfs_swap_activate as well, but reading and decrementing nr_swapfiles
is not, so this is inconsistent. It may not be wrong although still
racy. The case where I think it could be racing is:
btrfs_swap_activate
create_snapshot
atomic_read(&root->nr_swapfiles)
<continue snapshot>
atomic_inc(&root->nr_swapfiles)
<activate swapfile>
and this patch would not prevent it.
If it turns out that the root_item_lock is necessary then it would also
make sense to switch the type of nr_swapfiles to a plain int if we don't
use the semantics of atomic_t.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted
2022-03-23 12:34 ` David Sterba
@ 2022-03-23 13:33 ` Filipe Manana
0 siblings, 0 replies; 12+ messages in thread
From: Filipe Manana @ 2022-03-23 13:33 UTC (permalink / raw)
To: dsterba, Kaiwen Hu, quwenruo.btrfs, linux-btrfs, robbieko,
cccheng, seanding
On Wed, Mar 23, 2022 at 01:34:29PM +0100, David Sterba wrote:
> On Wed, Mar 23, 2022 at 03:10:32PM +0800, Kaiwen Hu wrote:
> > This patch prevent subvol being deleted when the subvol contains
> > any active swapfile.
> >
> > Since the subvolume is deleted, we cannot swapoff the swapfile in
> > this deleted subvolume. However, the swapfile is still active,
> > we unable to unmount this volume. Let it into some deadlock
> > situation.
> >
> > The test looks like this:
> > mkfs.btrfs -f $dev > /dev/null
> > mount $dev $mnt
> >
> > btrfs sub create $mnt/subvol
> > touch $mnt/subvol/swapfile
> > chmod 600 $mnt/subvol/swapfile
> > chattr +C $mnt/subvol/swapfile
> > dd if=/dev/zero of=$mnt/subvol/swapfile bs=1K count=4096
> > mkswap $mnt/subvol/swapfile
> > swapon $mnt/subvol/swapfile
> >
> > btrfs sub delete $mnt/subvol
> > swapoff $mnt/subvol/swapfile // failed: No such file or directory
> > swapoff --all
> >
> > unmount $mnt // target is busy.
> >
> > To prevent above issue, we simply check that whether the subvolume
> > contains any active swapfile, and stop the deleting process. This
> > behavior is like snapshot ioctl dealing with a swapfile.
> >
> > Reviewed-by: Robbie Ko <robbieko@synology.com>
> > Signed-off-by: Kaiwen Hu <kevinhu@synology.com>
> > ---
> >
> > Add comment on it.
> >
> > fs/btrfs/inode.c | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 5bbea5ec31fc..b32def311f44 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -4460,6 +4460,12 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
> > dest->root_key.objectid);
> > return -EPERM;
> > }
> > + if (atomic_read(&dest->nr_swapfiles)) {
>
> This is under the root_item_lock and the increment in
> btrfs_swap_activate as well, but reading and decrementing nr_swapfiles
> is not, so this is inconsistent. It may not be wrong although still
> racy. The case where I think it could be racing is:
>
> btrfs_swap_activate
> create_snapshot
> atomic_read(&root->nr_swapfiles)
> <continue snapshot>
> atomic_inc(&root->nr_swapfiles)
> <activate swapfile>
>
> and this patch would not prevent it.
That race can not happen due to the protection of the snapshot lock.
In fact that race used to exist, but it was fixed last year by
commit dd0734f2a866f9 ("btrfs: fix race between swap file activation and snapshot creation").
> If it turns out that the root_item_lock is necessary then it would also
> make sense to switch the type of nr_swapfiles to a plain int if we don't
> use the semantics of atomic_t.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted
2022-03-23 7:10 ` [PATCH v2] " Kaiwen Hu
2022-03-23 7:59 ` Qu Wenruo
2022-03-23 12:34 ` David Sterba
@ 2022-03-23 13:37 ` Filipe Manana
2022-03-23 21:45 ` David Sterba
3 siblings, 0 replies; 12+ messages in thread
From: Filipe Manana @ 2022-03-23 13:37 UTC (permalink / raw)
To: Kaiwen Hu
Cc: quwenruo.btrfs, linux-btrfs, dsterba, robbieko, cccheng, seanding
On Wed, Mar 23, 2022 at 03:10:32PM +0800, Kaiwen Hu wrote:
> This patch prevent subvol being deleted when the subvol contains
> any active swapfile.
>
> Since the subvolume is deleted, we cannot swapoff the swapfile in
> this deleted subvolume. However, the swapfile is still active,
> we unable to unmount this volume. Let it into some deadlock
> situation.
>
> The test looks like this:
> mkfs.btrfs -f $dev > /dev/null
> mount $dev $mnt
>
> btrfs sub create $mnt/subvol
> touch $mnt/subvol/swapfile
> chmod 600 $mnt/subvol/swapfile
> chattr +C $mnt/subvol/swapfile
> dd if=/dev/zero of=$mnt/subvol/swapfile bs=1K count=4096
> mkswap $mnt/subvol/swapfile
> swapon $mnt/subvol/swapfile
>
> btrfs sub delete $mnt/subvol
> swapoff $mnt/subvol/swapfile // failed: No such file or directory
> swapoff --all
>
> unmount $mnt // target is busy.
>
> To prevent above issue, we simply check that whether the subvolume
> contains any active swapfile, and stop the deleting process. This
> behavior is like snapshot ioctl dealing with a swapfile.
>
> Reviewed-by: Robbie Ko <robbieko@synology.com>
> Signed-off-by: Kaiwen Hu <kevinhu@synology.com>
Looks good.
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Are you planning on sending in a test case for fstests as well?
It's great to have a reproducer in a changelog, but unless it is
turned into a test case for fstests, it doesn't prevent a regression
in the future.
Thanks.
> ---
>
> Add comment on it.
>
> fs/btrfs/inode.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5bbea5ec31fc..b32def311f44 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4460,6 +4460,12 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
> dest->root_key.objectid);
> return -EPERM;
> }
> + if (atomic_read(&dest->nr_swapfiles)) {
> + spin_unlock(&dest->root_item_lock);
> + btrfs_warn(fs_info,
> + "attempt to delete subvolume with active swapfile");
> + return -ETXTBSY;
> + }
> root_flags = btrfs_root_flags(&dest->root_item);
> btrfs_set_root_flags(&dest->root_item,
> root_flags | BTRFS_ROOT_SUBVOL_DEAD);
> @@ -10418,8 +10424,22 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
> * set. We use this counter to prevent snapshots. We must increment it
> * before walking the extents because we don't want a concurrent
> * snapshot to run after we've already checked the extents.
> + *
> + * It is possible that subvolume is marked for deletion but still not
> + * remove yet. To prevent this race, we check the root's mark before
> + * activating swapfile.
> */
> + spin_lock(&root->root_item_lock);
> + if (btrfs_root_dead(root)) {
> + spin_unlock(&root->root_item_lock);
> + btrfs_exclop_finish(fs_info);
> + btrfs_warn(fs_info,
> + "cannot activate swapfile because subvolume is marked for deletion");
> + return -EINVAL;
> + }
> atomic_inc(&root->nr_swapfiles);
> + spin_unlock(&root->root_item_lock);
> +
>
> isize = ALIGN_DOWN(inode->i_size, fs_info->sectorsize);
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted
2022-03-23 7:10 ` [PATCH v2] " Kaiwen Hu
` (2 preceding siblings ...)
2022-03-23 13:37 ` Filipe Manana
@ 2022-03-23 21:45 ` David Sterba
2022-03-24 4:59 ` Kaiwen Hu
3 siblings, 1 reply; 12+ messages in thread
From: David Sterba @ 2022-03-23 21:45 UTC (permalink / raw)
To: Kaiwen Hu
Cc: quwenruo.btrfs, linux-btrfs, dsterba, robbieko, cccheng, seanding
On Wed, Mar 23, 2022 at 03:10:32PM +0800, Kaiwen Hu wrote:
> This patch prevent subvol being deleted when the subvol contains
> any active swapfile.
>
> Since the subvolume is deleted, we cannot swapoff the swapfile in
> this deleted subvolume. However, the swapfile is still active,
> we unable to unmount this volume. Let it into some deadlock
> situation.
>
> The test looks like this:
> mkfs.btrfs -f $dev > /dev/null
> mount $dev $mnt
>
> btrfs sub create $mnt/subvol
> touch $mnt/subvol/swapfile
> chmod 600 $mnt/subvol/swapfile
> chattr +C $mnt/subvol/swapfile
> dd if=/dev/zero of=$mnt/subvol/swapfile bs=1K count=4096
> mkswap $mnt/subvol/swapfile
> swapon $mnt/subvol/swapfile
>
> btrfs sub delete $mnt/subvol
> swapoff $mnt/subvol/swapfile // failed: No such file or directory
> swapoff --all
>
> unmount $mnt // target is busy.
>
> To prevent above issue, we simply check that whether the subvolume
> contains any active swapfile, and stop the deleting process. This
> behavior is like snapshot ioctl dealing with a swapfile.
>
> Reviewed-by: Robbie Ko <robbieko@synology.com>
> Signed-off-by: Kaiwen Hu <kevinhu@synology.com>
Added to misc-next, thanks.
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4460,6 +4460,12 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
> dest->root_key.objectid);
> return -EPERM;
> }
> + if (atomic_read(&dest->nr_swapfiles)) {
> + spin_unlock(&dest->root_item_lock);
> + btrfs_warn(fs_info,
> + "attempt to delete subvolume with active swapfile");
I've added the subvolume id to the two messages, otherwise it's not a
very useful for user.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted
2022-03-23 21:45 ` David Sterba
@ 2022-03-24 4:59 ` Kaiwen Hu
0 siblings, 0 replies; 12+ messages in thread
From: Kaiwen Hu @ 2022-03-24 4:59 UTC (permalink / raw)
To: dsterba, quwenruo.btrfs, linux-btrfs, robbieko, cccheng,
seanding, fdmanana
On 3/23/2022 9:37 PM, Filipe Manana wrote:
> Are you planning on sending in a test case for fstests as well?
> It's great to have a reproducer in a changelog, but unless it is
> turned into a test case for fstests, it doesn't prevent a regression
> in the future.
Ok, I'll work on it.
On 3/24/2022 5:45 AM, David Sterba wrote:
> I've added the subvolume id to the two messages, otherwise it's not a
> very useful for user.
Thanks,
Kaiwen Hu
^ permalink raw reply [flat|nested] 12+ messages in thread