[Devel] [PATCH vz10] ovl: release the upperdir/workdir in-use locks before the umount sync

[Devel] [PATCH vz10] ovl: release the upperdir/workdir in-use locks before the umount sync

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

The in-use locks on upperdir and workdir are released by ovl_free_fs()
from ->put_super(). generic_shutdown_super() calls sync_filesystem()
before that, and ovl_sync_fs() syncs the whole upper filesystem, so the
locks stay held across a journal commit and a cache flush of the upper
fs after the overlay is gone. With index=on a new overlay that uses the
same upperdir, or uses it as a lowerdir, fails to mount in that window:

  overlayfs: upperdir is in-use as upperdir/workdir of another mount,
  mount with '-o index=off' to override exclusive upperdir protection.

Userspace hits this when the last reference is not dropped by umount(2)
itself. Docker with the containerd snapshotter mounts a snapshot on a
thread with a private mount namespace for docker cp and for buildkit,
and the superblock dies when that thread exits, after the reply to the
client has gone out. The next request then gets EBUSY. The window grows
with the flush latency of the upper fs and is there even if nothing was
written through the overlay, as the mount itself dirties the upper.

->kill_sb() is called once s_active drops to zero, with no mount and no
open file left. Release the locks there, before generic_shutdown_super()
runs. What the dying superblock still does to the upper fs, the sync and
dropping its references to upper dentries and inodes, does not touch the
contents of upperdir or workdir. ovl_free_fs() still releases the locks
for a failed mount, so clear the *_locked flags to keep it from
unlocking a second time and dropping the lock of a newer mount.

This takes the sync out of the window but does not close it: the final
mntput of a dying mount namespace runs from task_work, after the exiting
thread may have reported completion. Userspace that needs the
directories free should umount(2) before saying the job is done.

https://virtuozzo.atlassian.net/browse/VSTOR-153253

Fixes: 2cac0c00a6cd ("ovl: get exclusive ownership on upper/work dirs")
Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Cc: Konstantin Khorenko <khorenko@virtuozzo.com>
Feature: fix ms/overlayfs
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/params.c    |  5 +----
 fs/overlayfs/super.c     | 12 +++++++++++-
 fs/overlayfs/util.c      | 12 ++++++++++++
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 8577c7884fb9..2331bd55302c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -528,6 +528,7 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
 bool ovl_inuse_trylock(struct dentry *dentry);
 void ovl_inuse_unlock(struct dentry *dentry);
 bool ovl_is_inuse(struct dentry *dentry);
+void ovl_inuse_unlock_all(struct ovl_fs *ofs);
 bool ovl_need_index(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry);
 void ovl_nlink_end(struct dentry *dentry);
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 32c58fcfe52f..c102fb0ac481 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -735,12 +735,9 @@ void ovl_free_fs(struct ovl_fs *ofs)
 	iput(ofs->workdir_trap);
 	dput(ofs->whiteout);
 	dput(ofs->workdir);
-	if (ofs->workdir_locked)
-		ovl_inuse_unlock(ofs->workbasedir);
+	ovl_inuse_unlock_all(ofs);
 	dput(ofs->workbasedir);
 	path_put(&ofs->workbasepath);
-	if (ofs->upperdir_locked)
-		ovl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root);
 	path_put(&ofs->upperpath);
 
 	/* Reuse ofs->config.lowerdirs as a vfsmount array before freeing it */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index fb47711c8fb9..8153362caad1 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1502,13 +1502,23 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
 	return err;
 }
 
+static void ovl_kill_sb(struct super_block *sb)
+{
+	struct ovl_fs *ofs = OVL_FS(sb);
+
+	/* Nothing can modify them now, do not hold them across the sync */
+	if (ofs)
+		ovl_inuse_unlock_all(ofs);
+	kill_anon_super(sb);
+}
+
 struct file_system_type ovl_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "overlay",
 	.init_fs_context	= ovl_init_fs_context,
 	.parameters		= ovl_parameter_spec,
 	.fs_flags		= FS_USERNS_MOUNT | FS_VIRTUALIZED,
-	.kill_sb		= kill_anon_super,
+	.kill_sb		= ovl_kill_sb,
 };
 MODULE_ALIAS_FS("overlay");
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f95090f0d29c..817563215001 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1051,6 +1051,18 @@ bool ovl_is_inuse(struct dentry *dentry)
 	return inuse;
 }
 
+void ovl_inuse_unlock_all(struct ovl_fs *ofs)
+{
+	if (ofs->workdir_locked) {
+		ovl_inuse_unlock(ofs->workbasedir);
+		ofs->workdir_locked = false;
+	}
+	if (ofs->upperdir_locked) {
+		ovl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root);
+		ofs->upperdir_locked = false;
+	}
+}
+
 /*
  * Does this overlay dentry need to be indexed on copy up?
  */
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH vz10] ovl: release the upperdir/workdir in-use locks before the umount sync

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> The in-use locks on upperdir and workdir are released by ovl_free_fs()
> from ->put_super(). generic_shutdown_super() calls sync_filesystem()
> before that, and ovl_sync_fs() syncs the whole upper filesystem, so the
> locks stay held across a journal commit and a cache flush of the upper
> fs after the overlay is gone. With index=on a new overlay that uses the
> same upperdir, or uses it as a lowerdir, fails to mount in that window:
> 
>   overlayfs: upperdir is in-use as upperdir/workdir of another mount,
>   mount with '-o index=off' to override exclusive upperdir protection.
> 
> Userspace hits this when the last reference is not dropped by umount(2)
> itself. Docker with the containerd snapshotter mounts a snapshot on a
> thread with a private mount namespace for docker cp and for buildkit,
> [ ... ]
> Fixes: 2cac0c00a6cd ("ovl: get exclusive ownership on upper/work dirs")
> Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> Cc: Konstantin Khorenko <khorenko@virtuozzo.com>
> Feature: fix ms/overlayfs
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261106151641.3980601-1-den@openvz.org?part=1


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH vz10] ovl: release the upperdir/workdir in-use locks before the umount sync

[Pavel Tikhomirov]

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

On 11/6/26 16:16, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> The in-use locks on upperdir and workdir are released by ovl_free_fs()
> from ->put_super(). generic_shutdown_super() calls sync_filesystem()
> before that, and ovl_sync_fs() syncs the whole upper filesystem, so the
> locks stay held across a journal commit and a cache flush of the upper
> fs after the overlay is gone. With index=on a new overlay that uses the
> same upperdir, or uses it as a lowerdir, fails to mount in that window:
> 
>   overlayfs: upperdir is in-use as upperdir/workdir of another mount,
>   mount with '-o index=off' to override exclusive upperdir protection.
> 
> Userspace hits this when the last reference is not dropped by umount(2)
> itself. Docker with the containerd snapshotter mounts a snapshot on a
> thread with a private mount namespace for docker cp and for buildkit,
> and the superblock dies when that thread exits, after the reply to the
> client has gone out. The next request then gets EBUSY. The window grows
> with the flush latency of the upper fs and is there even if nothing was
> written through the overlay, as the mount itself dirties the upper.
> 
> ->kill_sb() is called once s_active drops to zero, with no mount and no
> open file left. Release the locks there, before generic_shutdown_super()
> runs. What the dying superblock still does to the upper fs, the sync and
> dropping its references to upper dentries and inodes, does not touch the
> contents of upperdir or workdir. ovl_free_fs() still releases the locks
> for a failed mount, so clear the *_locked flags to keep it from
> unlocking a second time and dropping the lock of a newer mount.
> 
> This takes the sync out of the window but does not close it: the final
> mntput of a dying mount namespace runs from task_work, after the exiting
> thread may have reported completion. Userspace that needs the
> directories free should umount(2) before saying the job is done.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-153253
> 
> Fixes: 2cac0c00a6cd ("ovl: get exclusive ownership on upper/work dirs")
> Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> Cc: Konstantin Khorenko <khorenko@virtuozzo.com>
> Feature: fix ms/overlayfs
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/params.c    |  5 +----
>  fs/overlayfs/super.c     | 12 +++++++++++-
>  fs/overlayfs/util.c      | 12 ++++++++++++
>  4 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 8577c7884fb9..2331bd55302c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -528,6 +528,7 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
>  bool ovl_inuse_trylock(struct dentry *dentry);
>  void ovl_inuse_unlock(struct dentry *dentry);
>  bool ovl_is_inuse(struct dentry *dentry);
> +void ovl_inuse_unlock_all(struct ovl_fs *ofs);
>  bool ovl_need_index(struct dentry *dentry);
>  int ovl_nlink_start(struct dentry *dentry);
>  void ovl_nlink_end(struct dentry *dentry);
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 32c58fcfe52f..c102fb0ac481 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -735,12 +735,9 @@ void ovl_free_fs(struct ovl_fs *ofs)
>  	iput(ofs->workdir_trap);
>  	dput(ofs->whiteout);
>  	dput(ofs->workdir);
> -	if (ofs->workdir_locked)
> -		ovl_inuse_unlock(ofs->workbasedir);
> +	ovl_inuse_unlock_all(ofs);
>  	dput(ofs->workbasedir);
>  	path_put(&ofs->workbasepath);
> -	if (ofs->upperdir_locked)
> -		ovl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root);
>  	path_put(&ofs->upperpath);
>  
>  	/* Reuse ofs->config.lowerdirs as a vfsmount array before freeing it */
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index fb47711c8fb9..8153362caad1 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1502,13 +1502,23 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
>  	return err;
>  }
>  
> +static void ovl_kill_sb(struct super_block *sb)
> +{
> +	struct ovl_fs *ofs = OVL_FS(sb);
> +
> +	/* Nothing can modify them now, do not hold them across the sync */
> +	if (ofs)
> +		ovl_inuse_unlock_all(ofs);
> +	kill_anon_super(sb);
> +}
> +
>  struct file_system_type ovl_fs_type = {
>  	.owner			= THIS_MODULE,
>  	.name			= "overlay",
>  	.init_fs_context	= ovl_init_fs_context,
>  	.parameters		= ovl_parameter_spec,
>  	.fs_flags		= FS_USERNS_MOUNT | FS_VIRTUALIZED,
> -	.kill_sb		= kill_anon_super,
> +	.kill_sb		= ovl_kill_sb,
>  };
>  MODULE_ALIAS_FS("overlay");
>  
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f95090f0d29c..817563215001 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1051,6 +1051,18 @@ bool ovl_is_inuse(struct dentry *dentry)
>  	return inuse;
>  }
>  
> +void ovl_inuse_unlock_all(struct ovl_fs *ofs)
> +{
> +	if (ofs->workdir_locked) {
> +		ovl_inuse_unlock(ofs->workbasedir);
> +		ofs->workdir_locked = false;
> +	}
> +	if (ofs->upperdir_locked) {
> +		ovl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root);
> +		ofs->upperdir_locked = false;
> +	}
> +}
> +
>  /*
>   * Does this overlay dentry need to be indexed on copy up?
>   */

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH vz10] ovl: release the upperdir/workdir in-use locks before the umount sync

[Denis V. Lunev]

On 11/6/26 16:16, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> From: Denis V. Lunev <den@openvz.org>
>
> The in-use locks on upperdir and workdir are released by ovl_free_fs()
> from ->put_super(). generic_shutdown_super() calls sync_filesystem()
> before that, and ovl_sync_fs() syncs the whole upper filesystem, so the
> locks stay held across a journal commit and a cache flush of the upper
> fs after the overlay is gone. With index=on a new overlay that uses the
> same upperdir, or uses it as a lowerdir, fails to mount in that window:
>
>   overlayfs: upperdir is in-use as upperdir/workdir of another mount,
>   mount with '-o index=off' to override exclusive upperdir protection.
>
> Userspace hits this when the last reference is not dropped by umount(2)
> itself. Docker with the containerd snapshotter mounts a snapshot on a
> thread with a private mount namespace for docker cp and for buildkit,
> and the superblock dies when that thread exits, after the reply to the
> client has gone out. The next request then gets EBUSY. The window grows
> with the flush latency of the upper fs and is there even if nothing was
> written through the overlay, as the mount itself dirties the upper.
>
> ->kill_sb() is called once s_active drops to zero, with no mount and no
> open file left. Release the locks there, before generic_shutdown_super()
> runs. What the dying superblock still does to the upper fs, the sync and
> dropping its references to upper dentries and inodes, does not touch the
> contents of upperdir or workdir. ovl_free_fs() still releases the locks
> for a failed mount, so clear the *_locked flags to keep it from
> unlocking a second time and dropping the lock of a newer mount.
>
> This takes the sync out of the window but does not close it: the final
> mntput of a dying mount namespace runs from task_work, after the exiting
> thread may have reported completion. Userspace that needs the
> directories free should umount(2) before saying the job is done.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-153253
>
> Fixes: 2cac0c00a6cd ("ovl: get exclusive ownership on upper/work dirs")
> Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> Cc: Konstantin Khorenko <khorenko@virtuozzo.com>
> Feature: fix ms/overlayfs
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/params.c    |  5 +----
>  fs/overlayfs/super.c     | 12 +++++++++++-
>  fs/overlayfs/util.c      | 12 ++++++++++++
>  4 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 8577c7884fb9..2331bd55302c 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -528,6 +528,7 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
>  bool ovl_inuse_trylock(struct dentry *dentry);
>  void ovl_inuse_unlock(struct dentry *dentry);
>  bool ovl_is_inuse(struct dentry *dentry);
> +void ovl_inuse_unlock_all(struct ovl_fs *ofs);
>  bool ovl_need_index(struct dentry *dentry);
>  int ovl_nlink_start(struct dentry *dentry);
>  void ovl_nlink_end(struct dentry *dentry);
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 32c58fcfe52f..c102fb0ac481 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -735,12 +735,9 @@ void ovl_free_fs(struct ovl_fs *ofs)
>  	iput(ofs->workdir_trap);
>  	dput(ofs->whiteout);
>  	dput(ofs->workdir);
> -	if (ofs->workdir_locked)
> -		ovl_inuse_unlock(ofs->workbasedir);
> +	ovl_inuse_unlock_all(ofs);
>  	dput(ofs->workbasedir);
>  	path_put(&ofs->workbasepath);
> -	if (ofs->upperdir_locked)
> -		ovl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root);
>  	path_put(&ofs->upperpath);
>  
>  	/* Reuse ofs->config.lowerdirs as a vfsmount array before freeing it */
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index fb47711c8fb9..8153362caad1 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1502,13 +1502,23 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
>  	return err;
>  }
>  
> +static void ovl_kill_sb(struct super_block *sb)
> +{
> +	struct ovl_fs *ofs = OVL_FS(sb);
> +
> +	/* Nothing can modify them now, do not hold them across the sync */
> +	if (ofs)
> +		ovl_inuse_unlock_all(ofs);
> +	kill_anon_super(sb);
> +}
> +
>  struct file_system_type ovl_fs_type = {
>  	.owner			= THIS_MODULE,
>  	.name			= "overlay",
>  	.init_fs_context	= ovl_init_fs_context,
>  	.parameters		= ovl_parameter_spec,
>  	.fs_flags		= FS_USERNS_MOUNT | FS_VIRTUALIZED,
> -	.kill_sb		= kill_anon_super,
> +	.kill_sb		= ovl_kill_sb,
>  };
>  MODULE_ALIAS_FS("overlay");
>  
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f95090f0d29c..817563215001 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1051,6 +1051,18 @@ bool ovl_is_inuse(struct dentry *dentry)
>  	return inuse;
>  }
>  
> +void ovl_inuse_unlock_all(struct ovl_fs *ofs)
> +{
> +	if (ofs->workdir_locked) {
> +		ovl_inuse_unlock(ofs->workbasedir);
> +		ofs->workdir_locked = false;
> +	}
> +	if (ofs->upperdir_locked) {
> +		ovl_inuse_unlock(ovl_upper_mnt(ofs)->mnt_root);
> +		ofs->upperdir_locked = false;
> +	}
> +}
> +
>  /*
>   * Does this overlay dentry need to be indexed on copy up?
>   */
merged mainstream


> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 5a15e5478a6b..82e270711dbc 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -849,11 +849,8 @@ void ovl_free_fs(struct ovl_fs *ofs)
>         iput(ofs->workdir_trap);
>         dput(ofs->whiteout);
>         dput(ofs->workdir);
> -       if (ofs->workdir_locked)
> -               ovl_inuse_unlock(ofs->workbasedir);
> +       ovl_inuse_unlock_all(ofs);

I moved this up to the start of ovl_free_fs() to be consistent with order
in ovl_kill_super.

Thanks,
Amir.


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel