Re: [PATCH] fs: don't return -EINVAL for successful nested thaw

[Jan Kara]

On Fri 21-08-26 10:54:51, Moritz Tanner wrote:
> Commit 7366f8b6fc6a ("fs: handle freezing from multiple devices")
> replaced the freeze_holders bitmask with per-holder counters to allow
> nested freezes. In the bitmask version, a thaw that released a shared
> hold while another holder remained returned 0. Since the rework,
> thaw_super_locked() drops the freeze reference via freeze_dec() but
> then returns -EINVAL when other freezers remain, misinforming the
> caller: the thaw did succeed, the superblock just stays frozen for the
> remaining holders.
> 
> This breaks bdev-initiated freezing. When a filesystem is frozen with
> FIFREEZE and additionally frozen via bdev_freeze() -- which nests by
> design, see fs_bdev_freeze() -- the subsequent bdev_thaw() receives
> -EINVAL from the holder op although its freeze reference was dropped,
> and therefore keeps bd_fsfreeze_count elevated. Then device-mapper's
> unlock_fs() ignores bdev_thaw()'s return value, so nothing rebalances
> the count. After the user's FITHAW and umount, the block device can
> never be mounted again:
> 
>     dm-1: Can't mount, blockdev is frozen
> 
> There is no way for userspace to drop the leaked count; only
> destroying the block device (or a reboot) recovers the device.
> 
> Reproducer (any kernel since v6.8):
> 
>     dmsetup create dut --table "0 $(blockdev --getsz "$DEV") linear $DEV 0"
>     mkfs.ext4 /dev/mapper/dut
>     mount /dev/mapper/dut /mnt
>     fsfreeze --freeze /mnt      # freeze_ucount == 1
>     dmsetup suspend dut         # bd_fsfreeze_count == 1, ucount == 2
>     dmsetup resume dut          # ucount 2 -> 1, but thaw_super()
>                                 # returns -EINVAL, so bdev_thaw()
>                                 # keeps bd_fsfreeze_count at 1
>     fsfreeze --unfreeze /mnt    # filesystem thaws fine
>     umount /mnt
>     mount /dev/mapper/dut /mnt  # EBUSY, forever
> 
> The same happens with fsfreeze held across an LVM snapshot of the
> origin volume.
> 
> fs_bdev_thaw()'s documentation already describes the intended
> semantics: "If this function returns zero it doesn't mean that the
> filesystem is unfrozen as it may have been frozen multiple times".
> Restore them by returning 0 when a nested thaw drops its hold while
> other freezers remain. Thawing without holding a freeze still fails
> with -EINVAL as may_unfreeze() rejects that case before the reference
> count is touched.
> 
> Fixes: 7366f8b6fc6a ("fs: handle freezing from multiple devices")
> Cc: <stable@xxxxxxxxxxxxxxx> # needs adjustments for < 6.17 (no may_unfreeze())
> Signed-off-by: Moritz Tanner <moritz.tanner@xxxxxxxxxx>

Ouch. Good catch. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/super.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 05e443173038..01db6124e409 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -2369,11 +2369,14 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
>  		goto out_unlock;
>  
>  	/*
> -	 * All freezers share a single active reference.
> -	 * So just unlock in case there are any left.
> +	 * All freezers share a single active reference. If other freezers
> +	 * remain, drop our hold and report success; the superblock stays
> +	 * frozen until the last holder thaws it.
>  	 */
> -	if (freeze_dec(sb, who))
> +	if (freeze_dec(sb, who)) {
> +		error = 0;
>  		goto out_unlock;
> +	}
>  
>  	if (sb_rdonly(sb)) {
>  		sb->s_writers.frozen = SB_UNFROZEN;
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR