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

[Filipe Manana <fdmanana@kernel.org>]

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.