[PATCH] btrfs: prevent subvol with swapfile from being deleted

[PATCH] btrfs: prevent subvol with swapfile from being deleted

[Kaiwen Hu]

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.

Signed-off-by: Kaiwen Hu <kevinhu@synology.com>
---
 fs/btrfs/inode.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5bbea5ec31fc..e388b9043710 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);
@@ -10419,7 +10425,17 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
 	 * before walking the extents because we don't want a concurrent
 	 * snapshot to run after we've already checked the extents.
 	 */
+	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

Re: [PATCH] btrfs: prevent subvol with swapfile from being deleted

[Qu Wenruo]

On 2022/3/22 18:27, 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.
>
> Signed-off-by: Kaiwen Hu <kevinhu@synology.com>
> ---
>   fs/btrfs/inode.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5bbea5ec31fc..e388b9043710 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;
> +	}

This part looks fine to me.

>   	root_flags = btrfs_root_flags(&dest->root_item);
>   	btrfs_set_root_flags(&dest->root_item,
>   			     root_flags | BTRFS_ROOT_SUBVOL_DEAD);
> @@ -10419,7 +10425,17 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
>   	 * before walking the extents because we don't want a concurrent
>   	 * snapshot to run after we've already checked the extents.
>   	 */
> +	spin_lock(&root->root_item_lock);
> +	if (btrfs_root_dead(root)) {

This looks a little weird to me.

If the root is already dead, it means we should not be able to access
any file inside the subvolume.

How could we go into btrfs_swap_activate() while the root is already dead?

Or is there some special race I missed?

Thanks,
Qu

> +		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);
>

Re: [PATCH] btrfs: prevent subvol with swapfile from being deleted

[David Sterba]

On Tue, Mar 22, 2022 at 06:47:59PM +0800, Qu Wenruo wrote:
> > @@ -10419,7 +10425,17 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
> >   	 * before walking the extents because we don't want a concurrent
> >   	 * snapshot to run after we've already checked the extents.
> >   	 */
> > +	spin_lock(&root->root_item_lock);
> > +	if (btrfs_root_dead(root)) {
> 
> This looks a little weird to me.
> 
> If the root is already dead, it means we should not be able to access
> any file inside the subvolume.
> 
> How could we go into btrfs_swap_activate() while the root is already dead?
> 
> Or is there some special race I missed?

The deletion has a few steps, eg. the dentry is removed, root is marked
as dead and different thing locking protection.

I was wondering about file descriptor access to the subvolume and
calling swapon on that, but swapon/swapoff is a syscall and work with a
path argument so that can't happen. I haven't checked in what order are
the dentry removal and dead flag done, it could be theoretically
possible that there's a short window where the dentry is accessible and
subvolume already marked.

Re: [PATCH] btrfs: prevent subvol with swapfile from being deleted

[Kaiwen Hu]

On 3/23/2022 3:39 AM, David Sterba wrote:
> On Tue, Mar 22, 2022 at 06:47:59PM +0800, Qu Wenruo wrote:
>> This looks a little weird to me.
>>
>> If the root is already dead, it means we should not be able to access
>> any file inside the subvolume.
>>
>> How could we go into btrfs_swap_activate() while the root is already dead?
>>
>> Or is there some special race I missed?
> The deletion has a few steps, eg. the dentry is removed, root is marked
> as dead and different thing locking protection.
>
> I was wondering about file descriptor access to the subvolume and
> calling swapon on that, but swapon/swapoff is a syscall and work with a
> path argument so that can't happen. I haven't checked in what order are
> the dentry removal and dead flag done, it could be theoretically
> possible that there's a short window where the dentry is accessible and
> subvolume already marked.


Thanks for david's help.  Yes, it is possible the subvolume is marked 
for deletion,  but still not remove yet.  So like 
|btrfs_ioctl_send()|doing, I check the dead-root mark before activating 
swapfile to prevent the race condition.


Thanks,

Kaiwen Hu

Re: [PATCH] btrfs: prevent subvol with swapfile from being deleted

[Qu Wenruo]

On 2022/3/23 11:13, Kaiwen Hu wrote:
>
> On 3/23/2022 3:39 AM, David Sterba wrote:
>> On Tue, Mar 22, 2022 at 06:47:59PM +0800, Qu Wenruo wrote:
>>> This looks a little weird to me.
>>>
>>> If the root is already dead, it means we should not be able to access
>>> any file inside the subvolume.
>>>
>>> How could we go into btrfs_swap_activate() while the root is already
>>> dead?
>>>
>>> Or is there some special race I missed?
>> The deletion has a few steps, eg. the dentry is removed, root is marked
>> as dead and different thing locking protection.
>>
>> I was wondering about file descriptor access to the subvolume and
>> calling swapon on that, but swapon/swapoff is a syscall and work with a
>> path argument so that can't happen. I haven't checked in what order are
>> the dentry removal and dead flag done, it could be theoretically
>> possible that there's a short window where the dentry is accessible and
>> subvolume already marked.
>
>
> Thanks for david's help.  Yes, it is possible the subvolume is marked
> for deletion,  but still not remove yet.  So like
> |btrfs_ioctl_send()|doing, I check the dead-root mark before activating
> swapfile to prevent the race condition.

OK, I just went to check btrfs_delete_subvolume(), it's indeed marking
the subvolume dead first, before even starting to delete the dentry.
Not the reverse.

Then it looks fine to me, better with a short comment on it.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

>
>
> Thanks,
>
> Kaiwen Hu
>
>
>
>

[PATCH v2] btrfs: prevent subvol with swapfile from being deleted

[Kaiwen Hu]

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)) {
+		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

Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted

[Qu Wenruo]

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);
>

Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted

[David Sterba]

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.

Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted

[Filipe Manana]

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.

Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted

[Filipe Manana]

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
> 

Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted

[David Sterba]

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.

Re: [PATCH v2] btrfs: prevent subvol with swapfile from being deleted

[Kaiwen Hu]

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