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

[Filipe Manana <fdmanana@kernel.org>]

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
>