| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <m2bvkh2v56akvvomku4w6n4lbw3zkc2awlutijndb7cc3tuirz@o64zcabrekch>
Date: Wed, 7 May 2025 13:18:34 +0200
From: Jan Kara <jack@...e.cz>
To: Christian Brauner <brauner@...nel.org>
Cc: Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
Ard Biesheuvel <ardb@...nel.org>, linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
James Bottomley <James.Bottomley@...senpartnership.com>, mcgrof@...nel.org, hch@...radead.org, david@...morbit.com,
rafael@...nel.org, djwong@...nel.org, pavel@...nel.org, peterz@...radead.org,
mingo@...hat.com, will@...nel.org, boqun.feng@...il.com
Subject: Re: [PATCH] fs: allow nesting with FREEZE_EXCL
On Fri 04-04-25 12:24:09, Christian Brauner wrote:
> If hibernation races with filesystem freezing (e.g. DM reconfiguration),
> then hibernation need not freeze a filesystem because it's already
> frozen but userspace may thaw the filesystem before hibernation actually
> happens.
>
> If the race happens the other way around, DM reconfiguration may
> unexpectedly fail with EBUSY.
>
> So allow FREEZE_EXCL to nest with other holders. An exclusive freezer
> cannot be undone by any of the other concurrent freezers.
>
> Signed-off-by: Christian Brauner <brauner@...nel.org>
This has fallen through the cracks in my inbox but the patch now looks good
to me. Maybe we should fold it into "fs: add owner of freeze/thaw" to not
have strange intermediate state in the series?
Honza
> ---
> fs/super.c | 71 ++++++++++++++++++++++++++++++++++++++++++------------
> include/linux/fs.h | 2 +-
> 2 files changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index b4bdbc509dba..e2fee655fbed 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1979,26 +1979,34 @@ static inline int freeze_dec(struct super_block *sb, enum freeze_holder who)
> return sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount;
> }
>
> -static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
> +static inline bool may_freeze(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner)
> {
> + lockdep_assert_held(&sb->s_umount);
> +
> WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
>
> if (who & FREEZE_EXCL) {
> if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
> return false;
> -
> - if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
> + if (WARN_ON_ONCE(who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL)))
> return false;
> -
> - return (sb->s_writers.freeze_kcount +
> - sb->s_writers.freeze_ucount) == 0;
> + if (WARN_ON_ONCE(!freeze_owner))
> + return false;
> + /* This freeze already has a specific owner. */
> + if (sb->s_writers.freeze_owner)
> + return false;
> + /*
> + * This is already frozen multiple times so we're just
> + * going to take a reference count and mark it as
> + * belonging to use.
> + */
> + if (sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount)
> + sb->s_writers.freeze_owner = freeze_owner;
> + return true;
> }
>
> - /* This filesystem is already exclusively frozen. */
> - if (sb->s_writers.freeze_owner)
> - return false;
> -
> if (who & FREEZE_HOLDER_KERNEL)
> return (who & FREEZE_MAY_NEST) ||
> sb->s_writers.freeze_kcount == 0;
> @@ -2011,20 +2019,51 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
> static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
> const void *freeze_owner)
> {
> + lockdep_assert_held(&sb->s_umount);
> +
> WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
>
> if (who & FREEZE_EXCL) {
> - if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
> - return false;
> if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
> return false;
> - if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
> + if (WARN_ON_ONCE(who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL)))
> + return false;
> + if (WARN_ON_ONCE(!freeze_owner))
> + return false;
> + if (WARN_ON_ONCE(sb->s_writers.freeze_kcount == 0))
> return false;
> - return sb->s_writers.freeze_owner == freeze_owner;
> + /* This isn't exclusively frozen. */
> + if (!sb->s_writers.freeze_owner)
> + return false;
> + /* This isn't exclusively frozen by us. */
> + if (sb->s_writers.freeze_owner != freeze_owner)
> + return false;
> + /*
> + * This is still frozen multiple times so we're just
> + * going to drop our reference count and undo our
> + * exclusive freeze.
> + */
> + if ((sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount) > 1)
> + sb->s_writers.freeze_owner = NULL;
> + return true;
> + }
> +
> + if (who & FREEZE_HOLDER_KERNEL) {
> + /*
> + * Someone's trying to steal the reference belonging to
> + * @sb->s_writers.freeze_owner.
> + */
> + if (sb->s_writers.freeze_kcount == 1 &&
> + sb->s_writers.freeze_owner)
> + return false;
> + return sb->s_writers.freeze_kcount > 0;
> }
>
> - return sb->s_writers.freeze_owner == NULL;
> + if (who & FREEZE_HOLDER_USERSPACE)
> + return sb->s_writers.freeze_ucount > 0;
> +
> + return false;
> }
>
> /**
> @@ -2095,7 +2134,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
>
> retry:
> if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
> - if (may_freeze(sb, who))
> + if (may_freeze(sb, who, freeze_owner))
> ret = !!WARN_ON_ONCE(freeze_inc(sb, who) == 1);
> else
> ret = -EBUSY;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1edcba3cd68e..7a3f821d2723 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2270,7 +2270,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
> * @FREEZE_HOLDER_KERNEL: kernel wants to freeze or thaw filesystem
> * @FREEZE_HOLDER_USERSPACE: userspace wants to freeze or thaw filesystem
> * @FREEZE_MAY_NEST: whether nesting freeze and thaw requests is allowed
> - * @FREEZE_EXCL: whether actual freezing must be done by the caller
> + * @FREEZE_EXCL: a freeze that can only be undone by the owner
> *
> * Indicate who the owner of the freeze or thaw request is and whether
> * the freeze needs to be exclusive or can nest.
>
> ---
> base-commit: a83fe97e0d53f7d2b0fc62fd9a322a963cb30306
> change-id: 20250404-work-freeze-5eacb515f044
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists