[PATCH RFC v2 0/2] ovl: add override_creds mount option

[PATCH RFC v2 0/2] ovl: add override_creds mount option

[Christian Brauner]

Hey,

Currently overlayfs uses the mounter's credentials for it's
override_creds() calls. That provides a consistent permission model.

This patches allows a caller to instruct overlayfs to use its
credentials instead. The caller must be located in the same user
namespace as the user namespace the overlayfs instance will be mounted
in. This provides a consistent and simple security model.

With this it is possible to e.g., mount an overlayfs instance where the
mounter must have CAP_SYS_ADMIN but the credentials used for
override_creds() have dropped CAP_SYS_ADMIN. It also allows the usage of
custom fs{g,u}id different from the callers and other tweaks.

I'm marking this as RFC since I've written this down pretty quickly and
I'm not sure I've thought enough about all the possible pitfalls. I
think overall the concept is sound but there might be additional changes
needed in ovl_fill_super(). Right now I'm just calling override_creds()
when creating the index and work directories.

Thanks!
Christian

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Changes in v2:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v1: https://lore.kernel.org/r/20250214-work-overlayfs-v1-0-465d1867d3d4@kernel.org

---
Christian Brauner (2):
      ovl: allow to specify override credentials
      selftests/ovl: add selftests for "override_creds"

 fs/overlayfs/params.c                              | 22 ++++++
 fs/overlayfs/super.c                               | 11 ++-
 .../filesystems/overlayfs/set_layers_via_fds.c     | 89 ++++++++++++++++++++++
 3 files changed, 121 insertions(+), 1 deletion(-)
---
base-commit: 7a54947e727b6df840780a66c970395ed9734ebe
change-id: 20250214-work-overlayfs-dfcfc4cd7ebd

[PATCH RFC v2 1/2] ovl: allow to specify override credentials

[Christian Brauner]

Currently overlayfs uses the mounter's credentials for it's
override_creds() calls. That provides a consistent permission model.

This patches allows a caller to instruct overlayfs to use its
credentials instead. The caller must be located in the same user
namespace as the user namespace the overlayfs instance will be mounted
in. This provides a consistent and simple security model.

With this it is possible to e.g., mount an overlayfs instance where the
mounter must have CAP_SYS_ADMIN but the credentials used for
override_creds() have dropped CAP_SYS_ADMIN. It also allows the usage of
custom fs{g,u}id different from the callers and other tweaks.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/params.c | 22 ++++++++++++++++++++++
 fs/overlayfs/super.c  | 11 ++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 1115c22deca0..f2bc8acf6bf1 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -59,6 +59,7 @@ enum ovl_opt {
 	Opt_metacopy,
 	Opt_verity,
 	Opt_volatile,
+	Opt_override_creds,
 };
 
 static const struct constant_table ovl_parameter_bool[] = {
@@ -155,6 +156,7 @@ const struct fs_parameter_spec ovl_parameter_spec[] = {
 	fsparam_enum("metacopy",            Opt_metacopy, ovl_parameter_bool),
 	fsparam_enum("verity",              Opt_verity, ovl_parameter_verity),
 	fsparam_flag("volatile",            Opt_volatile),
+	fsparam_flag_no("override_creds",   Opt_override_creds),
 	{}
 };
 
@@ -662,6 +664,26 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	case Opt_userxattr:
 		config->userxattr = true;
 		break;
+	case Opt_override_creds: {
+		const struct cred *cred = ofs->creator_cred;
+
+		if (!result.negated) {
+			if (fc->user_ns != current_user_ns()) {
+				err = -EINVAL;
+				break;
+			}
+
+			ofs->creator_cred = prepare_creds();
+			if (!ofs->creator_cred) {
+				err = -EINVAL;
+				break;
+			}
+		} else {
+			ofs->creator_cred = NULL;
+		}
+		put_cred(cred);
+		break;
+	}
 	default:
 		pr_err("unrecognized mount option \"%s\" or missing value\n",
 		       param->key);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 86ae6f6da36b..a85071fe18fd 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1305,6 +1305,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
 	struct ovl_fs_context *ctx = fc->fs_private;
+	const struct cred *old_cred = NULL;
 	struct dentry *root_dentry;
 	struct ovl_entry *oe;
 	struct ovl_layer *layers;
@@ -1318,10 +1319,15 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
 	sb->s_d_op = &ovl_dentry_operations;
 
 	err = -ENOMEM;
-	ofs->creator_cred = cred = prepare_creds();
+	if (!ofs->creator_cred)
+		ofs->creator_cred = cred = prepare_creds();
+	else
+		cred = (struct cred *)ofs->creator_cred;
 	if (!cred)
 		goto out_err;
 
+	old_cred = ovl_override_creds(sb);
+
 	err = ovl_fs_params_verify(ctx, &ofs->config);
 	if (err)
 		goto out_err;
@@ -1481,6 +1487,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	sb->s_root = root_dentry;
 
+	ovl_revert_creds(old_cred);
 	return 0;
 
 out_free_oe:
@@ -1488,6 +1495,8 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
 out_err:
 	ovl_free_fs(ofs);
 	sb->s_fs_info = NULL;
+	if (old_cred)
+		ovl_revert_creds(old_cred);
 	return err;
 }
 

-- 
2.47.2

Re: [PATCH RFC v2 1/2] ovl: allow to specify override credentials

[Amir Goldstein]

On Mon, Feb 17, 2025 at 11:20 AM Christian Brauner <brauner@kernel.org> wrote:
>
> Currently overlayfs uses the mounter's credentials for it's
> override_creds() calls. That provides a consistent permission model.
>
> This patches allows a caller to instruct overlayfs to use its
> credentials instead. The caller must be located in the same user
> namespace as the user namespace the overlayfs instance will be mounted
> in. This provides a consistent and simple security model.
>
> With this it is possible to e.g., mount an overlayfs instance where the
> mounter must have CAP_SYS_ADMIN but the credentials used for
> override_creds() have dropped CAP_SYS_ADMIN. It also allows the usage of
> custom fs{g,u}id different from the callers and other tweaks.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

For the code:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

For non-RFC patch please add something to the
'Permission model' section in overlayfs.rst

This section refers to the "mounting task" which implies that the
"mounting task" credentials are constant throughout the mount
(which made sense until the new mount API).

I would add a [*] near "mounting task" and write a footnote that
"mounting task" credentials is referring to the mounting task
credentials at the time of mount() system call or at the some of
executing FSCONFIG_CMD_CREATE fsconfig() command or
(since kernel xxx) at the time of setting the mount option
"override_creds" using the fsconfig() system call.

Feel free to rephrase as you see fit.

Thanks,
Amir.


> ---
>  fs/overlayfs/params.c | 22 ++++++++++++++++++++++
>  fs/overlayfs/super.c  | 11 ++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 1115c22deca0..f2bc8acf6bf1 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -59,6 +59,7 @@ enum ovl_opt {
>         Opt_metacopy,
>         Opt_verity,
>         Opt_volatile,
> +       Opt_override_creds,
>  };
>
>  static const struct constant_table ovl_parameter_bool[] = {
> @@ -155,6 +156,7 @@ const struct fs_parameter_spec ovl_parameter_spec[] = {
>         fsparam_enum("metacopy",            Opt_metacopy, ovl_parameter_bool),
>         fsparam_enum("verity",              Opt_verity, ovl_parameter_verity),
>         fsparam_flag("volatile",            Opt_volatile),
> +       fsparam_flag_no("override_creds",   Opt_override_creds),
>         {}
>  };
>
> @@ -662,6 +664,26 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
>         case Opt_userxattr:
>                 config->userxattr = true;
>                 break;
> +       case Opt_override_creds: {
> +               const struct cred *cred = ofs->creator_cred;
> +
> +               if (!result.negated) {
> +                       if (fc->user_ns != current_user_ns()) {
> +                               err = -EINVAL;
> +                               break;
> +                       }
> +
> +                       ofs->creator_cred = prepare_creds();
> +                       if (!ofs->creator_cred) {
> +                               err = -EINVAL;
> +                               break;
> +                       }
> +               } else {
> +                       ofs->creator_cred = NULL;
> +               }
> +               put_cred(cred);
> +               break;
> +       }
>         default:
>                 pr_err("unrecognized mount option \"%s\" or missing value\n",
>                        param->key);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 86ae6f6da36b..a85071fe18fd 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1305,6 +1305,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>         struct ovl_fs *ofs = sb->s_fs_info;
>         struct ovl_fs_context *ctx = fc->fs_private;
> +       const struct cred *old_cred = NULL;
>         struct dentry *root_dentry;
>         struct ovl_entry *oe;
>         struct ovl_layer *layers;
> @@ -1318,10 +1319,15 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
>         sb->s_d_op = &ovl_dentry_operations;
>
>         err = -ENOMEM;
> -       ofs->creator_cred = cred = prepare_creds();
> +       if (!ofs->creator_cred)
> +               ofs->creator_cred = cred = prepare_creds();
> +       else
> +               cred = (struct cred *)ofs->creator_cred;
>         if (!cred)
>                 goto out_err;
>
> +       old_cred = ovl_override_creds(sb);
> +
>         err = ovl_fs_params_verify(ctx, &ofs->config);
>         if (err)
>                 goto out_err;
> @@ -1481,6 +1487,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
>
>         sb->s_root = root_dentry;
>
> +       ovl_revert_creds(old_cred);
>         return 0;
>
>  out_free_oe:
> @@ -1488,6 +1495,8 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
>  out_err:
>         ovl_free_fs(ofs);
>         sb->s_fs_info = NULL;
> +       if (old_cred)
> +               ovl_revert_creds(old_cred);
>         return err;
>  }
>
>
> --
> 2.47.2
>

[PATCH RFC v2 2/2] selftests/ovl: add selftests for "override_creds"

[Christian Brauner]

Add a simple test to verify that the new "override_creds" option works.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 .../filesystems/overlayfs/set_layers_via_fds.c     | 89 ++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
index e65d95d97846..6c9f4df5df8d 100644
--- a/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
+++ b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
@@ -6,11 +6,13 @@
 #include <sched.h>
 #include <stdio.h>
 #include <string.h>
+#include <sys/fsuid.h>
 #include <sys/stat.h>
 #include <sys/mount.h>
 #include <unistd.h>
 
 #include "../../kselftest_harness.h"
+#include "../../pidfd/pidfd.h"
 #include "log.h"
 #include "wrappers.h"
 
@@ -409,4 +411,91 @@ TEST_F(set_layers_via_fds, set_layers_via_detached_mount_fds)
 	ASSERT_EQ(fclose(f_mountinfo), 0);
 }
 
+TEST_F(set_layers_via_fds, set_override_creds)
+{
+	int fd_context, fd_tmpfs, fd_overlay;
+	int layer_fds[] = { [0 ... 3] = -EBADF };
+	pid_t pid;
+	int pidfd;
+
+	ASSERT_EQ(unshare(CLONE_NEWNS), 0);
+	ASSERT_EQ(sys_mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL), 0);
+
+	fd_context = sys_fsopen("tmpfs", 0);
+	ASSERT_GE(fd_context, 0);
+
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
+	fd_tmpfs = sys_fsmount(fd_context, 0, 0);
+	ASSERT_GE(fd_tmpfs, 0);
+	ASSERT_EQ(close(fd_context), 0);
+
+	ASSERT_EQ(mkdirat(fd_tmpfs, "w", 0755), 0);
+	ASSERT_EQ(mkdirat(fd_tmpfs, "u", 0755), 0);
+	ASSERT_EQ(mkdirat(fd_tmpfs, "l1", 0755), 0);
+	ASSERT_EQ(mkdirat(fd_tmpfs, "l2", 0755), 0);
+
+	layer_fds[0] = openat(fd_tmpfs, "w", O_DIRECTORY);
+	ASSERT_GE(layer_fds[0], 0);
+
+	layer_fds[1] = openat(fd_tmpfs, "u", O_DIRECTORY);
+	ASSERT_GE(layer_fds[1], 0);
+
+	layer_fds[2] = openat(fd_tmpfs, "l1", O_DIRECTORY);
+	ASSERT_GE(layer_fds[2], 0);
+
+	layer_fds[3] = openat(fd_tmpfs, "l2", O_DIRECTORY);
+	ASSERT_GE(layer_fds[3], 0);
+
+	ASSERT_EQ(sys_move_mount(fd_tmpfs, "", -EBADF, "/tmp", MOVE_MOUNT_F_EMPTY_PATH), 0);
+	ASSERT_EQ(close(fd_tmpfs), 0);
+
+	fd_context = sys_fsopen("overlay", 0);
+	ASSERT_GE(fd_context, 0);
+
+	ASSERT_NE(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir", NULL, layer_fds[2]), 0);
+
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "workdir",   NULL, layer_fds[0]), 0);
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "upperdir",  NULL, layer_fds[1]), 0);
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[2]), 0);
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[3]), 0);
+
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_STRING, "metacopy", "on", 0), 0);
+
+	pid = create_child(&pidfd, CLONE_NEWUSER);
+	EXPECT_GE(pid, 0);
+	if (pid == 0) {
+		if (!sys_fsconfig(fd_context, FSCONFIG_SET_FLAG, "override_creds", NULL, 0)) {
+			TH_LOG("sys_fsconfig should have failed");
+			_exit(EXIT_FAILURE);
+		}
+
+		_exit(EXIT_SUCCESS);
+	}
+	EXPECT_EQ(sys_waitid(P_PID, pid, NULL, WEXITED), 0);
+	EXPECT_EQ(close(pidfd), 0);
+
+	pid = create_child(&pidfd, 0);
+	EXPECT_GE(pid, 0);
+	if (pid == 0) {
+		if (sys_fsconfig(fd_context, FSCONFIG_SET_FLAG, "override_creds", NULL, 0)) {
+			TH_LOG("sys_fsconfig should have succeeded");
+			_exit(EXIT_FAILURE);
+		}
+
+		_exit(EXIT_SUCCESS);
+	}
+	EXPECT_EQ(sys_waitid(P_PID, pid, NULL, WEXITED), 0);
+	EXPECT_EQ(close(pidfd), 0);
+
+	ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
+
+	fd_overlay = sys_fsmount(fd_context, 0, 0);
+	ASSERT_GE(fd_overlay, 0);
+
+	ASSERT_EQ(sys_move_mount(fd_overlay, "", -EBADF, "/set_layers_via_fds", MOVE_MOUNT_F_EMPTY_PATH), 0);
+
+	ASSERT_EQ(close(fd_context), 0);
+	ASSERT_EQ(close(fd_overlay), 0);
+}
+
 TEST_HARNESS_MAIN

-- 
2.47.2

Re: [PATCH RFC v2 2/2] selftests/ovl: add selftests for "override_creds"

[Amir Goldstein]

On Mon, Feb 17, 2025 at 11:20 AM Christian Brauner <brauner@kernel.org> wrote:
>
> Add a simple test to verify that the new "override_creds" option works.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  .../filesystems/overlayfs/set_layers_via_fds.c     | 89 ++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>
> diff --git a/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
> index e65d95d97846..6c9f4df5df8d 100644
> --- a/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
> +++ b/tools/testing/selftests/filesystems/overlayfs/set_layers_via_fds.c
> @@ -6,11 +6,13 @@
>  #include <sched.h>
>  #include <stdio.h>
>  #include <string.h>
> +#include <sys/fsuid.h>
>  #include <sys/stat.h>
>  #include <sys/mount.h>
>  #include <unistd.h>
>
>  #include "../../kselftest_harness.h"
> +#include "../../pidfd/pidfd.h"
>  #include "log.h"
>  #include "wrappers.h"
>
> @@ -409,4 +411,91 @@ TEST_F(set_layers_via_fds, set_layers_via_detached_mount_fds)
>         ASSERT_EQ(fclose(f_mountinfo), 0);
>  }
>
> +TEST_F(set_layers_via_fds, set_override_creds)
> +{
> +       int fd_context, fd_tmpfs, fd_overlay;
> +       int layer_fds[] = { [0 ... 3] = -EBADF };
> +       pid_t pid;
> +       int pidfd;
> +
> +       ASSERT_EQ(unshare(CLONE_NEWNS), 0);
> +       ASSERT_EQ(sys_mount(NULL, "/", NULL, MS_SLAVE | MS_REC, NULL), 0);
> +
> +       fd_context = sys_fsopen("tmpfs", 0);
> +       ASSERT_GE(fd_context, 0);
> +
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
> +       fd_tmpfs = sys_fsmount(fd_context, 0, 0);
> +       ASSERT_GE(fd_tmpfs, 0);
> +       ASSERT_EQ(close(fd_context), 0);
> +
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "w", 0755), 0);
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "u", 0755), 0);
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "l1", 0755), 0);
> +       ASSERT_EQ(mkdirat(fd_tmpfs, "l2", 0755), 0);
> +
> +       layer_fds[0] = openat(fd_tmpfs, "w", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[0], 0);
> +
> +       layer_fds[1] = openat(fd_tmpfs, "u", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[1], 0);
> +
> +       layer_fds[2] = openat(fd_tmpfs, "l1", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[2], 0);
> +
> +       layer_fds[3] = openat(fd_tmpfs, "l2", O_DIRECTORY);
> +       ASSERT_GE(layer_fds[3], 0);
> +
> +       ASSERT_EQ(sys_move_mount(fd_tmpfs, "", -EBADF, "/tmp", MOVE_MOUNT_F_EMPTY_PATH), 0);
> +       ASSERT_EQ(close(fd_tmpfs), 0);
> +
> +       fd_context = sys_fsopen("overlay", 0);
> +       ASSERT_GE(fd_context, 0);
> +
> +       ASSERT_NE(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir", NULL, layer_fds[2]), 0);
> +
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "workdir",   NULL, layer_fds[0]), 0);
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "upperdir",  NULL, layer_fds[1]), 0);
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[2]), 0);
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_FD, "lowerdir+", NULL, layer_fds[3]), 0);
> +
> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_SET_STRING, "metacopy", "on", 0), 0);
> +
> +       pid = create_child(&pidfd, CLONE_NEWUSER);
> +       EXPECT_GE(pid, 0);
> +       if (pid == 0) {
> +               if (!sys_fsconfig(fd_context, FSCONFIG_SET_FLAG, "override_creds", NULL, 0)) {
> +                       TH_LOG("sys_fsconfig should have failed");
> +                       _exit(EXIT_FAILURE);
> +               }
> +
> +               _exit(EXIT_SUCCESS);
> +       }
> +       EXPECT_EQ(sys_waitid(P_PID, pid, NULL, WEXITED), 0);
> +       EXPECT_EQ(close(pidfd), 0);
> +
> +       pid = create_child(&pidfd, 0);
> +       EXPECT_GE(pid, 0);
> +       if (pid == 0) {
> +               if (sys_fsconfig(fd_context, FSCONFIG_SET_FLAG, "override_creds", NULL, 0)) {
> +                       TH_LOG("sys_fsconfig should have succeeded");
> +                       _exit(EXIT_FAILURE);
> +               }
> +
> +               _exit(EXIT_SUCCESS);
> +       }
> +       EXPECT_EQ(sys_waitid(P_PID, pid, NULL, WEXITED), 0);
> +       EXPECT_EQ(close(pidfd), 0);
> +

Suggest to verify functionality:
execute FSCONFIG_CMD_CREATE from a child which removes CAP_MKNOD...

> +       ASSERT_EQ(sys_fsconfig(fd_context, FSCONFIG_CMD_CREATE, NULL, NULL, 0), 0);
> +
> +       fd_overlay = sys_fsmount(fd_context, 0, 0);
> +       ASSERT_GE(fd_overlay, 0);
> +
> +       ASSERT_EQ(sys_move_mount(fd_overlay, "", -EBADF, "/set_layers_via_fds", MOVE_MOUNT_F_EMPTY_PATH), 0);
> +

... and verify that you can mknod(2) on ovl mount.

Other than that you may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.