[RFC,v3,1/2] mm: restrictedmem: Allow userspace to specify mount for memfd_restricted

  By default, the backing shmem file for a restrictedmem fd is created
on shmem's kernel space mount.

With this patch, an optional tmpfs mount can be specified via an fd,
which will be used as the mountpoint for backing the shmem file
associated with a restrictedmem fd.

This will help restrictedmem fds inherit the properties of the
provided tmpfs mounts, for example, hugepage allocation hints, NUMA
binding hints, etc.

Permissions for the fd passed to memfd_restricted() is modeled after
the openat() syscall, since both of these allow creation of a file
upon a mount/directory.

Permission to reference the mount the fd represents is checked upon fd
creation by other syscalls (e.g. fsmount(), open(), or open_tree(),
etc) and any process that can present memfd_restricted() with a valid
fd is expected to have obtained permission to use the mount
represented by the fd. This behavior is intended to parallel that of
the openat() syscall.

memfd_restricted() will check that the tmpfs superblock is
writable, and that the mount is also writable, before attempting to
create a restrictedmem file on the mount.

Signed-off-by: Ackerley Tng <ackerleytng@google.com>
---
 include/linux/syscalls.h           |  2 +-
 include/uapi/linux/restrictedmem.h |  8 ++++
 mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
 3 files changed, 77 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/restrictedmem.h

--
2.40.0.348.gf938b09366-goog
  

On 01.04.23 01:50, Ackerley Tng wrote:
> By default, the backing shmem file for a restrictedmem fd is created
> on shmem's kernel space mount.
> 
> With this patch, an optional tmpfs mount can be specified via an fd,
> which will be used as the mountpoint for backing the shmem file
> associated with a restrictedmem fd.
> 
> This will help restrictedmem fds inherit the properties of the
> provided tmpfs mounts, for example, hugepage allocation hints, NUMA
> binding hints, etc.
> 
> Permissions for the fd passed to memfd_restricted() is modeled after
> the openat() syscall, since both of these allow creation of a file
> upon a mount/directory.
> 
> Permission to reference the mount the fd represents is checked upon fd
> creation by other syscalls (e.g. fsmount(), open(), or open_tree(),
> etc) and any process that can present memfd_restricted() with a valid
> fd is expected to have obtained permission to use the mount
> represented by the fd. This behavior is intended to parallel that of
> the openat() syscall.
> 
> memfd_restricted() will check that the tmpfs superblock is
> writable, and that the mount is also writable, before attempting to
> create a restrictedmem file on the mount.
> 
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
>   include/linux/syscalls.h           |  2 +-
>   include/uapi/linux/restrictedmem.h |  8 ++++
>   mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
>   3 files changed, 77 insertions(+), 7 deletions(-)
>   create mode 100644 include/uapi/linux/restrictedmem.h
> 
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index f9e9e0c820c5..a23c4c385cd3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
>   asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
>   					    unsigned long home_node,
>   					    unsigned long flags);
> -asmlinkage long sys_memfd_restricted(unsigned int flags);
> +asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
> 
>   /*
>    * Architecture-specific system calls
> diff --git a/include/uapi/linux/restrictedmem.h b/include/uapi/linux/restrictedmem.h
> new file mode 100644
> index 000000000000..22d6f2285f6d
> --- /dev/null
> +++ b/include/uapi/linux/restrictedmem.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
> +#define _UAPI_LINUX_RESTRICTEDMEM_H
> +
> +/* flags for memfd_restricted */
> +#define RMFD_USERMNT		0x0001U

I wonder if we can come up with a more expressive prefix than RMFD. 
Sounds more like "rm fd" ;) Maybe it should better match the 
"memfd_restricted" syscall name, like "MEMFD_RSTD_USERMNT".


> +
> +#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */
> diff --git a/mm/restrictedmem.c b/mm/restrictedmem.c
> index c5d869d8c2d8..f7b62364a31a 100644
> --- a/mm/restrictedmem.c
> +++ b/mm/restrictedmem.c
> @@ -1,11 +1,12 @@
>   // SPDX-License-Identifier: GPL-2.0
> -#include "linux/sbitmap.h"

Looks like an unrelated change?

> +#include <linux/namei.h>
>   #include <linux/pagemap.h>
>   #include <linux/pseudo_fs.h>
>   #include <linux/shmem_fs.h>
>   #include <linux/syscalls.h>
>   #include <uapi/linux/falloc.h>
>   #include <uapi/linux/magic.h>
> +#include <uapi/linux/restrictedmem.h>
>   #include <linux/restrictedmem.h>
> 
>   struct restrictedmem {
> @@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
>   	return file;
>   }
> 
> -SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
> +static int restrictedmem_create(struct vfsmount *mount)
>   {
>   	struct file *file, *restricted_file;
>   	int fd, err;
> 
> -	if (flags)
> -		return -EINVAL;
> -
>   	fd = get_unused_fd_flags(0);
>   	if (fd < 0)
>   		return fd;
> 
> -	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> +	if (mount)
> +		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
> +	else
> +		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> +
>   	if (IS_ERR(file)) {
>   		err = PTR_ERR(file);
>   		goto err_fd;
> @@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
>   	return err;
>   }
> 
> +static bool is_shmem_mount(struct vfsmount *mnt)
> +{
> +	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
> +}
> +
> +static bool is_mount_root(struct file *file)
> +{
> +	return file->f_path.dentry == file->f_path.mnt->mnt_root;
> +}

I'd inline at least that function, pretty self-explaining.

> +
> +static int restrictedmem_create_on_user_mount(int mount_fd)
> +{
> +	int ret;
> +	struct fd f;
> +	struct vfsmount *mnt;
> +
> +	f = fdget_raw(mount_fd);
> +	if (!f.file)
> +		return -EBADF;
> +
> +	ret = -EINVAL;
> +	if (!is_mount_root(f.file))
> +		goto out;
> +
> +	mnt = f.file->f_path.mnt;
> +	if (!is_shmem_mount(mnt))
> +		goto out;
> +
> +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);
> +	if (ret)
> +		goto out;
> +
> +	ret = mnt_want_write(mnt);
> +	if (unlikely(ret))
> +		goto out;
> +
> +	ret = restrictedmem_create(mnt);
> +
> +	mnt_drop_write(mnt);
> +out:
> +	fdput(f);
> +
> +	return ret;
> +}
> +
> +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
> +{
> +	if (flags & ~RMFD_USERMNT)
> +		return -EINVAL;
> +
> +	if (flags == RMFD_USERMNT) {
> +		if (mount_fd < 0)
> +			return -EINVAL;
> +
> +		return restrictedmem_create_on_user_mount(mount_fd);
> +	} else {
> +		return restrictedmem_create(NULL);
> +	}


You can drop the else case:

if (flags == RMFD_USERMNT) {
	...
	return restrictedmem_create_on_user_mount(mount_fd);
}
return restrictedmem_create(NULL);


I do wonder if you want to properly check for a flag instead of 
comparing values. Results in a more natural way to deal with flags:

if (flags & RMFD_USERMNT) {

}

> +}
> +
>   int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
>   		       struct restrictedmem_notifier *notifier, bool exclusive)
>   {

The "memfd_restricted" vs. "restrictedmem" terminology is a bit 
unfortunate, but not your fault here.


I'm not a FS person, but it does look good to me.
  

On Fri, Mar 31, 2023 at 11:50:39PM +0000, Ackerley Tng wrote:
> By default, the backing shmem file for a restrictedmem fd is created
> on shmem's kernel space mount.
> 
> With this patch, an optional tmpfs mount can be specified via an fd,
> which will be used as the mountpoint for backing the shmem file
> associated with a restrictedmem fd.
> 
> This will help restrictedmem fds inherit the properties of the
> provided tmpfs mounts, for example, hugepage allocation hints, NUMA
> binding hints, etc.
> 
> Permissions for the fd passed to memfd_restricted() is modeled after
> the openat() syscall, since both of these allow creation of a file
> upon a mount/directory.
> 
> Permission to reference the mount the fd represents is checked upon fd
> creation by other syscalls (e.g. fsmount(), open(), or open_tree(),
> etc) and any process that can present memfd_restricted() with a valid
> fd is expected to have obtained permission to use the mount
> represented by the fd. This behavior is intended to parallel that of
> the openat() syscall.
> 
> memfd_restricted() will check that the tmpfs superblock is
> writable, and that the mount is also writable, before attempting to
> create a restrictedmem file on the mount.
> 
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
>  include/linux/syscalls.h           |  2 +-
>  include/uapi/linux/restrictedmem.h |  8 ++++
>  mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
>  3 files changed, 77 insertions(+), 7 deletions(-)
>  create mode 100644 include/uapi/linux/restrictedmem.h
> 
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index f9e9e0c820c5..a23c4c385cd3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
>  asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
>  					    unsigned long home_node,
>  					    unsigned long flags);
> -asmlinkage long sys_memfd_restricted(unsigned int flags);
> +asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
> 
>  /*
>   * Architecture-specific system calls
> diff --git a/include/uapi/linux/restrictedmem.h b/include/uapi/linux/restrictedmem.h
> new file mode 100644
> index 000000000000..22d6f2285f6d
> --- /dev/null
> +++ b/include/uapi/linux/restrictedmem.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
> +#define _UAPI_LINUX_RESTRICTEDMEM_H
> +
> +/* flags for memfd_restricted */
> +#define RMFD_USERMNT		0x0001U
> +
> +#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */
> diff --git a/mm/restrictedmem.c b/mm/restrictedmem.c
> index c5d869d8c2d8..f7b62364a31a 100644
> --- a/mm/restrictedmem.c
> +++ b/mm/restrictedmem.c
> @@ -1,11 +1,12 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include "linux/sbitmap.h"
> +#include <linux/namei.h>
>  #include <linux/pagemap.h>
>  #include <linux/pseudo_fs.h>
>  #include <linux/shmem_fs.h>
>  #include <linux/syscalls.h>
>  #include <uapi/linux/falloc.h>
>  #include <uapi/linux/magic.h>
> +#include <uapi/linux/restrictedmem.h>
>  #include <linux/restrictedmem.h>
> 
>  struct restrictedmem {
> @@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
>  	return file;
>  }
> 
> -SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
> +static int restrictedmem_create(struct vfsmount *mount)
>  {
>  	struct file *file, *restricted_file;
>  	int fd, err;
> 
> -	if (flags)
> -		return -EINVAL;
> -
>  	fd = get_unused_fd_flags(0);
>  	if (fd < 0)
>  		return fd;
> 
> -	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> +	if (mount)
> +		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
> +	else
> +		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> +
>  	if (IS_ERR(file)) {
>  		err = PTR_ERR(file);
>  		goto err_fd;
> @@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
>  	return err;
>  }
> 
> +static bool is_shmem_mount(struct vfsmount *mnt)
> +{
> +	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
> +}
> +
> +static bool is_mount_root(struct file *file)
> +{
> +	return file->f_path.dentry == file->f_path.mnt->mnt_root;
> +}
> +
> +static int restrictedmem_create_on_user_mount(int mount_fd)
> +{
> +	int ret;
> +	struct fd f;
> +	struct vfsmount *mnt;
> +
> +	f = fdget_raw(mount_fd);
> +	if (!f.file)
> +		return -EBADF;
> +
> +	ret = -EINVAL;
> +	if (!is_mount_root(f.file))
> +		goto out;
> +
> +	mnt = f.file->f_path.mnt;
> +	if (!is_shmem_mount(mnt))
> +		goto out;
> +
> +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

Why MAY_EXEC?

> +	if (ret)
> +		goto out;
> +
> +	ret = mnt_want_write(mnt);
> +	if (unlikely(ret))
> +		goto out;
> +
> +	ret = restrictedmem_create(mnt);
> +
> +	mnt_drop_write(mnt);
> +out:
> +	fdput(f);
> +
> +	return ret;
> +}

We need review from fs folks. Look mostly sensible, but I have no
experience in fs.

> +
> +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
> +{
> +	if (flags & ~RMFD_USERMNT)
> +		return -EINVAL;
> +
> +	if (flags == RMFD_USERMNT) {
> +		if (mount_fd < 0)
> +			return -EINVAL;
> +
> +		return restrictedmem_create_on_user_mount(mount_fd);
> +	} else {
> +		return restrictedmem_create(NULL);
> +	}

Maybe restructure with single restrictedmem_create() call?

	struct vfsmount *mnt = NULL;

	if (flags == RMFD_USERMNT) {
		...
		mnt = ...();
	}

	return restrictedmem_create(mnt);
> +}
> +
>  int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
>  		       struct restrictedmem_notifier *notifier, bool exclusive)
>  {
> --
> 2.40.0.348.gf938b09366-goog
  

On Fri, Mar 31, 2023 at 11:50:39PM +0000, Ackerley Tng wrote:
> By default, the backing shmem file for a restrictedmem fd is created
> on shmem's kernel space mount.
> 
> With this patch, an optional tmpfs mount can be specified via an fd,
> which will be used as the mountpoint for backing the shmem file
> associated with a restrictedmem fd.
> 
> This will help restrictedmem fds inherit the properties of the
> provided tmpfs mounts, for example, hugepage allocation hints, NUMA
> binding hints, etc.
> 
> Permissions for the fd passed to memfd_restricted() is modeled after
> the openat() syscall, since both of these allow creation of a file
> upon a mount/directory.
> 
> Permission to reference the mount the fd represents is checked upon fd
> creation by other syscalls (e.g. fsmount(), open(), or open_tree(),
> etc) and any process that can present memfd_restricted() with a valid
> fd is expected to have obtained permission to use the mount
> represented by the fd. This behavior is intended to parallel that of
> the openat() syscall.
> 
> memfd_restricted() will check that the tmpfs superblock is
> writable, and that the mount is also writable, before attempting to
> create a restrictedmem file on the mount.
> 
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
>  include/linux/syscalls.h           |  2 +-
>  include/uapi/linux/restrictedmem.h |  8 ++++
>  mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
>  3 files changed, 77 insertions(+), 7 deletions(-)
>  create mode 100644 include/uapi/linux/restrictedmem.h
> 
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index f9e9e0c820c5..a23c4c385cd3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
>  asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
>  					    unsigned long home_node,
>  					    unsigned long flags);
> -asmlinkage long sys_memfd_restricted(unsigned int flags);
> +asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
> 
>  /*
>   * Architecture-specific system calls
> diff --git a/include/uapi/linux/restrictedmem.h b/include/uapi/linux/restrictedmem.h
> new file mode 100644
> index 000000000000..22d6f2285f6d
> --- /dev/null
> +++ b/include/uapi/linux/restrictedmem.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
> +#define _UAPI_LINUX_RESTRICTEDMEM_H
> +
> +/* flags for memfd_restricted */
> +#define RMFD_USERMNT		0x0001U
> +
> +#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */
> diff --git a/mm/restrictedmem.c b/mm/restrictedmem.c
> index c5d869d8c2d8..f7b62364a31a 100644
> --- a/mm/restrictedmem.c
> +++ b/mm/restrictedmem.c
> @@ -1,11 +1,12 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include "linux/sbitmap.h"
> +#include <linux/namei.h>
>  #include <linux/pagemap.h>
>  #include <linux/pseudo_fs.h>
>  #include <linux/shmem_fs.h>
>  #include <linux/syscalls.h>
>  #include <uapi/linux/falloc.h>
>  #include <uapi/linux/magic.h>
> +#include <uapi/linux/restrictedmem.h>
>  #include <linux/restrictedmem.h>
> 
>  struct restrictedmem {
> @@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
>  	return file;
>  }
> 
> -SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
> +static int restrictedmem_create(struct vfsmount *mount)
>  {
>  	struct file *file, *restricted_file;
>  	int fd, err;
> 
> -	if (flags)
> -		return -EINVAL;
> -
>  	fd = get_unused_fd_flags(0);

Any reasons the file descriptors aren't O_CLOEXEC by default? I don't
see any reasons why we should introduce new fdtypes that aren't
O_CLOEXEC by default. The "don't mix-and-match" train has already left
the station anyway as we do have seccomp noitifer fds and pidfds both of
which are O_CLOEXEC by default.

>  	if (fd < 0)
>  		return fd;
> 
> -	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> +	if (mount)
> +		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
> +	else
> +		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> +
>  	if (IS_ERR(file)) {
>  		err = PTR_ERR(file);
>  		goto err_fd;
> @@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
>  	return err;
>  }
> 
> +static bool is_shmem_mount(struct vfsmount *mnt)
> +{
> +	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;

This can just be if (mnt->mnt_sb->s_magic == TMPFS_MAGIC).

> +}
> +
> +static bool is_mount_root(struct file *file)
> +{
> +	return file->f_path.dentry == file->f_path.mnt->mnt_root;

mount -t tmpfs tmpfs /mnt
touch /mnt/bla
touch /mnt/ble
mount --bind /mnt/bla /mnt/ble
fd = open("/mnt/ble")
fd_restricted = memfd_restricted(fd)

IOW, this doesn't restrict it to the tmpfs root. It only restricts it to
paths that refer to the root of any tmpfs mount. To exclude bind-mounts
that aren't bind-mounts of the whole filesystem you want:

path->dentry == path->mnt->mnt_root && 
path->mnt->mnt_root == path->mnt->mnt_sb->s_root

> +}
> +
> +static int restrictedmem_create_on_user_mount(int mount_fd)
> +{
> +	int ret;
> +	struct fd f;
> +	struct vfsmount *mnt;
> +
> +	f = fdget_raw(mount_fd);
> +	if (!f.file)
> +		return -EBADF;
> +
> +	ret = -EINVAL;
> +	if (!is_mount_root(f.file))
> +		goto out;
> +
> +	mnt = f.file->f_path.mnt;
> +	if (!is_shmem_mount(mnt))
> +		goto out;
> +
> +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

With the current semantics you're asking whether you have write
permissions on the /mnt/ble file in order to get answer to the question
whether you're allowed to create an unlinked restricted memory file.
That doesn't make much sense afaict.

> +	if (ret)
> +		goto out;
> +
> +	ret = mnt_want_write(mnt);
> +	if (unlikely(ret))
> +		goto out;
> +
> +	ret = restrictedmem_create(mnt);
> +
> +	mnt_drop_write(mnt);
> +out:
> +	fdput(f);
> +
> +	return ret;
> +}
> +
> +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
> +{
> +	if (flags & ~RMFD_USERMNT)
> +		return -EINVAL;
> +
> +	if (flags == RMFD_USERMNT) {

Why do you even need this flag? It seems that @mount_fd being < 0 is
sufficient to indicate that a new restricted memory fd is supposed to be
created in the system instance.

> +		if (mount_fd < 0)
> +			return -EINVAL;
> +
> +		return restrictedmem_create_on_user_mount(mount_fd);
> +	} else {
> +		return restrictedmem_create(NULL);
> +	}
> +}

I have to say that I'm very confused by all of this the more I look at it.

Effectively memfd restricted functions as a wrapper filesystem around
the tmpfs filesystem. This is basically a weird overlay filesystem.
You're allocating tmpfs files that you stash in restrictedmem files. 
I have to say that this seems very hacky. I didn't get this at all at
first.

So what does the caller get if they call statx() on a restricted memfd?
Do they get the device number of the tmpfs mount and the inode numbers
of the tmpfs mount? Because it looks like they would:

static int restrictedmem_getattr(struct user_namespace *mnt_userns,
				 const struct path *path, struct kstat *stat,
				 u32 request_mask, unsigned int query_flags)
{
	struct inode *inode = d_inode(path->dentry);
	struct restrictedmem *rm = inode->i_mapping->private_data;
	struct file *memfd = rm->memfd;

	return memfd->f_inode->i_op->getattr(mnt_userns, path, stat,
					     request_mask, query_flags);

That @memfd would be a struct file allocated in a tmpfs instance, no? So
you'd be calling the inode operation of the tmpfs file meaning that
struct kstat will be filled up with the info from the tmpfs instance.

But then if I call statfs() and check the fstype I would get
RESTRICTEDMEM_MAGIC, no? This is... unorthodox?

I'm honestly puzzled and this sounds really strange. There must be a
better way to implement all of this.

Shouldn't you try and make this a part of tmpfs proper? Make a really
separate filesystem and add a memfs library that both tmpfs and
restrictedmemfs can use? Add a mount option to tmpfs that makes it a
restricted tmpfs?
  

On Tue, Apr 04, 2023 at 03:53:13PM +0200, Christian Brauner wrote:
> On Fri, Mar 31, 2023 at 11:50:39PM +0000, Ackerley Tng wrote:
> > By default, the backing shmem file for a restrictedmem fd is created
> > on shmem's kernel space mount.
> > 
> > With this patch, an optional tmpfs mount can be specified via an fd,
> > which will be used as the mountpoint for backing the shmem file
> > associated with a restrictedmem fd.
> > 
> > This will help restrictedmem fds inherit the properties of the
> > provided tmpfs mounts, for example, hugepage allocation hints, NUMA
> > binding hints, etc.
> > 
> > Permissions for the fd passed to memfd_restricted() is modeled after
> > the openat() syscall, since both of these allow creation of a file
> > upon a mount/directory.
> > 
> > Permission to reference the mount the fd represents is checked upon fd
> > creation by other syscalls (e.g. fsmount(), open(), or open_tree(),
> > etc) and any process that can present memfd_restricted() with a valid
> > fd is expected to have obtained permission to use the mount
> > represented by the fd. This behavior is intended to parallel that of
> > the openat() syscall.
> > 
> > memfd_restricted() will check that the tmpfs superblock is
> > writable, and that the mount is also writable, before attempting to
> > create a restrictedmem file on the mount.
> > 
> > Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> > ---
> >  include/linux/syscalls.h           |  2 +-
> >  include/uapi/linux/restrictedmem.h |  8 ++++
> >  mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
> >  3 files changed, 77 insertions(+), 7 deletions(-)
> >  create mode 100644 include/uapi/linux/restrictedmem.h
> > 
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index f9e9e0c820c5..a23c4c385cd3 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
> >  asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
> >  					    unsigned long home_node,
> >  					    unsigned long flags);
> > -asmlinkage long sys_memfd_restricted(unsigned int flags);
> > +asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
> > 
> >  /*
> >   * Architecture-specific system calls
> > diff --git a/include/uapi/linux/restrictedmem.h b/include/uapi/linux/restrictedmem.h
> > new file mode 100644
> > index 000000000000..22d6f2285f6d
> > --- /dev/null
> > +++ b/include/uapi/linux/restrictedmem.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
> > +#define _UAPI_LINUX_RESTRICTEDMEM_H
> > +
> > +/* flags for memfd_restricted */
> > +#define RMFD_USERMNT		0x0001U
> > +
> > +#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */
> > diff --git a/mm/restrictedmem.c b/mm/restrictedmem.c
> > index c5d869d8c2d8..f7b62364a31a 100644
> > --- a/mm/restrictedmem.c
> > +++ b/mm/restrictedmem.c
> > @@ -1,11 +1,12 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -#include "linux/sbitmap.h"
> > +#include <linux/namei.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/pseudo_fs.h>
> >  #include <linux/shmem_fs.h>
> >  #include <linux/syscalls.h>
> >  #include <uapi/linux/falloc.h>
> >  #include <uapi/linux/magic.h>
> > +#include <uapi/linux/restrictedmem.h>
> >  #include <linux/restrictedmem.h>
> > 
> >  struct restrictedmem {
> > @@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
> >  	return file;
> >  }
> > 
> > -SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
> > +static int restrictedmem_create(struct vfsmount *mount)
> >  {
> >  	struct file *file, *restricted_file;
> >  	int fd, err;
> > 
> > -	if (flags)
> > -		return -EINVAL;
> > -
> >  	fd = get_unused_fd_flags(0);
> 
> Any reasons the file descriptors aren't O_CLOEXEC by default? I don't
> see any reasons why we should introduce new fdtypes that aren't
> O_CLOEXEC by default. The "don't mix-and-match" train has already left
> the station anyway as we do have seccomp noitifer fds and pidfds both of
> which are O_CLOEXEC by default.
> 
> >  	if (fd < 0)
> >  		return fd;
> > 
> > -	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> > +	if (mount)
> > +		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
> > +	else
> > +		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> > +
> >  	if (IS_ERR(file)) {
> >  		err = PTR_ERR(file);
> >  		goto err_fd;
> > @@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
> >  	return err;
> >  }
> > 
> > +static bool is_shmem_mount(struct vfsmount *mnt)
> > +{
> > +	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
> 
> This can just be if (mnt->mnt_sb->s_magic == TMPFS_MAGIC).
> 
> > +}
> > +
> > +static bool is_mount_root(struct file *file)
> > +{
> > +	return file->f_path.dentry == file->f_path.mnt->mnt_root;
> 
> mount -t tmpfs tmpfs /mnt
> touch /mnt/bla
> touch /mnt/ble
> mount --bind /mnt/bla /mnt/ble
> fd = open("/mnt/ble")
> fd_restricted = memfd_restricted(fd)
> 
> IOW, this doesn't restrict it to the tmpfs root. It only restricts it to
> paths that refer to the root of any tmpfs mount. To exclude bind-mounts
> that aren't bind-mounts of the whole filesystem you want:
> 
> path->dentry == path->mnt->mnt_root && 
> path->mnt->mnt_root == path->mnt->mnt_sb->s_root
> 
> > +}
> > +
> > +static int restrictedmem_create_on_user_mount(int mount_fd)
> > +{
> > +	int ret;
> > +	struct fd f;
> > +	struct vfsmount *mnt;
> > +
> > +	f = fdget_raw(mount_fd);
> > +	if (!f.file)
> > +		return -EBADF;
> > +
> > +	ret = -EINVAL;
> > +	if (!is_mount_root(f.file))
> > +		goto out;
> > +
> > +	mnt = f.file->f_path.mnt;
> > +	if (!is_shmem_mount(mnt))
> > +		goto out;
> > +
> > +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);
> 
> With the current semantics you're asking whether you have write
> permissions on the /mnt/ble file in order to get answer to the question
> whether you're allowed to create an unlinked restricted memory file.
> That doesn't make much sense afaict.
> 
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = mnt_want_write(mnt);
> > +	if (unlikely(ret))
> > +		goto out;
> > +
> > +	ret = restrictedmem_create(mnt);
> > +
> > +	mnt_drop_write(mnt);
> > +out:
> > +	fdput(f);
> > +
> > +	return ret;
> > +}
> > +
> > +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
> > +{
> > +	if (flags & ~RMFD_USERMNT)
> > +		return -EINVAL;
> > +
> > +	if (flags == RMFD_USERMNT) {
> 
> Why do you even need this flag? It seems that @mount_fd being < 0 is
> sufficient to indicate that a new restricted memory fd is supposed to be
> created in the system instance.
> 
> > +		if (mount_fd < 0)
> > +			return -EINVAL;
> > +
> > +		return restrictedmem_create_on_user_mount(mount_fd);
> > +	} else {
> > +		return restrictedmem_create(NULL);
> > +	}
> > +}
> 
> I have to say that I'm very confused by all of this the more I look at it.
> 
> Effectively memfd restricted functions as a wrapper filesystem around
> the tmpfs filesystem. This is basically a weird overlay filesystem.
> You're allocating tmpfs files that you stash in restrictedmem files. 
> I have to say that this seems very hacky. I didn't get this at all at
> first.
> 
> So what does the caller get if they call statx() on a restricted memfd?
> Do they get the device number of the tmpfs mount and the inode numbers
> of the tmpfs mount? Because it looks like they would:
> 
> static int restrictedmem_getattr(struct user_namespace *mnt_userns,
> 				 const struct path *path, struct kstat *stat,
> 				 u32 request_mask, unsigned int query_flags)
> {
> 	struct inode *inode = d_inode(path->dentry);
> 	struct restrictedmem *rm = inode->i_mapping->private_data;
> 	struct file *memfd = rm->memfd;
> 
> 	return memfd->f_inode->i_op->getattr(mnt_userns, path, stat,

This is pretty broken btw, because @path refers to a restrictedmem path
which you're passing to a tmpfs iop...

I see that in

	return memfd->f_inode->i_op->getattr(mnt_userns, &memfd->f_path, stat,
					     request_mask, query_flags);

this if fixed but still, this is... not great.

> 					     request_mask, query_flags);
> 
> That @memfd would be a struct file allocated in a tmpfs instance, no? So
> you'd be calling the inode operation of the tmpfs file meaning that
> struct kstat will be filled up with the info from the tmpfs instance.
> 
> But then if I call statfs() and check the fstype I would get
> RESTRICTEDMEM_MAGIC, no? This is... unorthodox?
> 
> I'm honestly puzzled and this sounds really strange. There must be a
> better way to implement all of this.
> 
> Shouldn't you try and make this a part of tmpfs proper? Make a really
> separate filesystem and add a memfs library that both tmpfs and
> restrictedmemfs can use? Add a mount option to tmpfs that makes it a
> restricted tmpfs?
  

Thanks again for your review!

Christian Brauner <brauner@kernel.org> writes:
> On Tue, Apr 04, 2023 at 03:53:13PM +0200, Christian Brauner wrote:
>> On Fri, Mar 31, 2023 at 11:50:39PM +0000, Ackerley Tng wrote:
>> >
>> > ...
>> >
>> > -SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
>> > +static int restrictedmem_create(struct vfsmount *mount)
>> >  {
>> >  	struct file *file, *restricted_file;
>> >  	int fd, err;
>> >
>> > -	if (flags)
>> > -		return -EINVAL;
>> > -
>> >  	fd = get_unused_fd_flags(0);

>> Any reasons the file descriptors aren't O_CLOEXEC by default? I don't
>> see any reasons why we should introduce new fdtypes that aren't
>> O_CLOEXEC by default. The "don't mix-and-match" train has already left
>> the station anyway as we do have seccomp noitifer fds and pidfds both of
>> which are O_CLOEXEC by default.


Thanks for pointing this out. I agree with using O_CLOEXEC, but didn’t
notice this before. Let us discuss this under the original series at
[1].

>> >  	if (fd < 0)
>> >  		return fd;
>> >
>> > -	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
>> > +	if (mount)
>> > +		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0,  
>> VM_NORESERVE);
>> > +	else
>> > +		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
>> > +
>> >  	if (IS_ERR(file)) {
>> >  		err = PTR_ERR(file);
>> >  		goto err_fd;
>> > @@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int,  
>> flags)
>> >  	return err;
>> >  }
>> >
>> > +static bool is_shmem_mount(struct vfsmount *mnt)
>> > +{
>> > +	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;

>> This can just be if (mnt->mnt_sb->s_magic == TMPFS_MAGIC).


Will simplify this in the next revision.

>> > +}
>> > +
>> > +static bool is_mount_root(struct file *file)
>> > +{
>> > +	return file->f_path.dentry == file->f_path.mnt->mnt_root;

>> mount -t tmpfs tmpfs /mnt
>> touch /mnt/bla
>> touch /mnt/ble
>> mount --bind /mnt/bla /mnt/ble
>> fd = open("/mnt/ble")
>> fd_restricted = memfd_restricted(fd)

>> IOW, this doesn't restrict it to the tmpfs root. It only restricts it to
>> paths that refer to the root of any tmpfs mount. To exclude bind-mounts
>> that aren't bind-mounts of the whole filesystem you want:

>> path->dentry == path->mnt->mnt_root &&
>> path->mnt->mnt_root == path->mnt->mnt_sb->s_root


Will adopt this in the next revision and add a selftest to check
this. Thanks for pointing this out!

>> > +}
>> > +
>> > +static int restrictedmem_create_on_user_mount(int mount_fd)
>> > +{
>> > +	int ret;
>> > +	struct fd f;
>> > +	struct vfsmount *mnt;
>> > +
>> > +	f = fdget_raw(mount_fd);
>> > +	if (!f.file)
>> > +		return -EBADF;
>> > +
>> > +	ret = -EINVAL;
>> > +	if (!is_mount_root(f.file))
>> > +		goto out;
>> > +
>> > +	mnt = f.file->f_path.mnt;
>> > +	if (!is_shmem_mount(mnt))
>> > +		goto out;
>> > +
>> > +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

>> With the current semantics you're asking whether you have write
>> permissions on the /mnt/ble file in order to get answer to the question
>> whether you're allowed to create an unlinked restricted memory file.
>> That doesn't make much sense afaict.


That's true. Since mnt_want_write() already checks for write permissions
and this syscall creates an unlinked file on the mount, we don't have to
check permissions on the file then. Will remove this in the next
revision!

>> > +	if (ret)
>> > +		goto out;
>> > +
>> > +	ret = mnt_want_write(mnt);
>> > +	if (unlikely(ret))
>> > +		goto out;
>> > +
>> > +	ret = restrictedmem_create(mnt);
>> > +
>> > +	mnt_drop_write(mnt);
>> > +out:
>> > +	fdput(f);
>> > +
>> > +	return ret;
>> > +}
>> > +
>> > +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
>> > +{
>> > +	if (flags & ~RMFD_USERMNT)
>> > +		return -EINVAL;
>> > +
>> > +	if (flags == RMFD_USERMNT) {

>> Why do you even need this flag? It seems that @mount_fd being < 0 is
>> sufficient to indicate that a new restricted memory fd is supposed to be
>> created in the system instance.


I'm hoping to have this patch series merged after Chao's patch series
introduces the memfd_restricted() syscall [1].

This flag is necessary to indicate the validity of the second argument.

With this flag, we can definitively return an error if the fd is
invalid, which I think is a better experience for the userspace
programmer than if we just silently default to the kernel mount when the
fd provided is invalid.

>> > +		if (mount_fd < 0)
>> > +			return -EINVAL;
>> > +
>> > +		return restrictedmem_create_on_user_mount(mount_fd);
>> > +	} else {
>> > +		return restrictedmem_create(NULL);
>> > +	}
>> > +}

>> I have to say that I'm very confused by all of this the more I look at  
>> it.

>> Effectively memfd restricted functions as a wrapper filesystem around
>> the tmpfs filesystem. This is basically a weird overlay filesystem.
>> You're allocating tmpfs files that you stash in restrictedmem files.
>> I have to say that this seems very hacky. I didn't get this at all at
>> first.

>> So what does the caller get if they call statx() on a restricted memfd?
>> Do they get the device number of the tmpfs mount and the inode numbers
>> of the tmpfs mount? Because it looks like they would:

>> static int restrictedmem_getattr(struct user_namespace *mnt_userns,
>> 				 const struct path *path, struct kstat *stat,
>> 				 u32 request_mask, unsigned int query_flags)
>> {
>> 	struct inode *inode = d_inode(path->dentry);
>> 	struct restrictedmem *rm = inode->i_mapping->private_data;
>> 	struct file *memfd = rm->memfd;

>> 	return memfd->f_inode->i_op->getattr(mnt_userns, path, stat,

> This is pretty broken btw, because @path refers to a restrictedmem path
> which you're passing to a tmpfs iop...

> I see that in

> 	return memfd->f_inode->i_op->getattr(mnt_userns, &memfd->f_path, stat,
> 					     request_mask, query_flags);

> this if fixed but still, this is... not great.


Thanks, this will be fixed in the next revision by rebasing on Chao's
latest code.

>> 					     request_mask, query_flags);

>> That @memfd would be a struct file allocated in a tmpfs instance, no? So
>> you'd be calling the inode operation of the tmpfs file meaning that
>> struct kstat will be filled up with the info from the tmpfs instance.

>> But then if I call statfs() and check the fstype I would get
>> RESTRICTEDMEM_MAGIC, no? This is... unorthodox?

>> I'm honestly puzzled and this sounds really strange. There must be a
>> better way to implement all of this.

>> Shouldn't you try and make this a part of tmpfs proper? Make a really
>> separate filesystem and add a memfs library that both tmpfs and
>> restrictedmemfs can use? Add a mount option to tmpfs that makes it a
>> restricted tmpfs?

This was discussed earlier in the patch series introducing
memfd_restricted and this approach was taken to better manage ownership
of required functionalities between two subsystems. Please see
discussion beginning [2]

[1] ->  
https://lore.kernel.org/lkml/20221202061347.1070246-1-chao.p.peng@linux.intel.com/T/.
[2] ->  
https://lore.kernel.org/lkml/ff5c5b97-acdf-9745-ebe5-c6609dd6322e@google.com/
  

Thanks for your review!

David Hildenbrand <david@redhat.com> writes:

> On 01.04.23 01:50, Ackerley Tng wrote:

>> ...

>> diff --git a/include/uapi/linux/restrictedmem.h  
>> b/include/uapi/linux/restrictedmem.h
>> new file mode 100644
>> index 000000000000..22d6f2285f6d
>> --- /dev/null
>> +++ b/include/uapi/linux/restrictedmem.h
>> @@ -0,0 +1,8 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
>> +#define _UAPI_LINUX_RESTRICTEDMEM_H
>> +
>> +/* flags for memfd_restricted */
>> +#define RMFD_USERMNT		0x0001U

> I wonder if we can come up with a more expressive prefix than RMFD.
> Sounds more like "rm fd" ;) Maybe it should better match the
> "memfd_restricted" syscall name, like "MEMFD_RSTD_USERMNT".


RMFD did actually sound vulgar, I'm good with MEMFD_RSTD_USERMNT!

>> +
>> +#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */
>> diff --git a/mm/restrictedmem.c b/mm/restrictedmem.c
>> index c5d869d8c2d8..f7b62364a31a 100644
>> --- a/mm/restrictedmem.c
>> +++ b/mm/restrictedmem.c
>> @@ -1,11 +1,12 @@
>>    // SPDX-License-Identifier: GPL-2.0
>> -#include "linux/sbitmap.h"

> Looks like an unrelated change?


Will remove this in the next revision.

>> +#include <linux/namei.h>
>>    #include <linux/pagemap.h>
>>    #include <linux/pseudo_fs.h>
>>    #include <linux/shmem_fs.h>
>>    #include <linux/syscalls.h>
>>    #include <uapi/linux/falloc.h>
>>    #include <uapi/linux/magic.h>
>> +#include <uapi/linux/restrictedmem.h>
>>    #include <linux/restrictedmem.h>

>>    struct restrictedmem {
>> @@ -189,19 +190,20 @@ static struct file  
>> *restrictedmem_file_create(struct file *memfd)
>>    	return file;
>>    }

>> -SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
>> +static int restrictedmem_create(struct vfsmount *mount)
>>    {
>>    	struct file *file, *restricted_file;
>>    	int fd, err;

>> -	if (flags)
>> -		return -EINVAL;
>> -
>>    	fd = get_unused_fd_flags(0);
>>    	if (fd < 0)
>>    		return fd;

>> -	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
>> +	if (mount)
>> +		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0,  
>> VM_NORESERVE);
>> +	else
>> +		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
>> +
>>    	if (IS_ERR(file)) {
>>    		err = PTR_ERR(file);
>>    		goto err_fd;
>> @@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int,  
>> flags)
>>    	return err;
>>    }

>> +static bool is_shmem_mount(struct vfsmount *mnt)
>> +{
>> +	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
>> +}
>> +
>> +static bool is_mount_root(struct file *file)
>> +{
>> +	return file->f_path.dentry == file->f_path.mnt->mnt_root;
>> +}

> I'd inline at least that function, pretty self-explaining.


Will inline this in the next revision.

>> +
>> +static int restrictedmem_create_on_user_mount(int mount_fd)
>> +{
>> +	int ret;
>> +	struct fd f;
>> +	struct vfsmount *mnt;
>> +
>> +	f = fdget_raw(mount_fd);
>> +	if (!f.file)
>> +		return -EBADF;
>> +
>> +	ret = -EINVAL;
>> +	if (!is_mount_root(f.file))
>> +		goto out;
>> +
>> +	mnt = f.file->f_path.mnt;
>> +	if (!is_shmem_mount(mnt))
>> +		goto out;
>> +
>> +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = mnt_want_write(mnt);
>> +	if (unlikely(ret))
>> +		goto out;
>> +
>> +	ret = restrictedmem_create(mnt);
>> +
>> +	mnt_drop_write(mnt);
>> +out:
>> +	fdput(f);
>> +
>> +	return ret;
>> +}
>> +
>> +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
>> +{
>> +	if (flags & ~RMFD_USERMNT)
>> +		return -EINVAL;
>> +
>> +	if (flags == RMFD_USERMNT) {
>> +		if (mount_fd < 0)
>> +			return -EINVAL;
>> +
>> +		return restrictedmem_create_on_user_mount(mount_fd);
>> +	} else {
>> +		return restrictedmem_create(NULL);
>> +	}


> You can drop the else case:

> if (flags == RMFD_USERMNT) {
> 	...
> 	return restrictedmem_create_on_user_mount(mount_fd);
> }
> return restrictedmem_create(NULL);


I'll be refactoring this to adopt Kirill's suggestion of using a single
restrictedmem_create(mnt) call.


> I do wonder if you want to properly check for a flag instead of
> comparing values. Results in a more natural way to deal with flags:

> if (flags & RMFD_USERMNT) {

> }


Will use this in the next revision.

>> +}
>> +
>>    int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
>>    		       struct restrictedmem_notifier *notifier, bool exclusive)
>>    {

> The "memfd_restricted" vs. "restrictedmem" terminology is a bit
> unfortunate, but not your fault here.


> I'm not a FS person, but it does look good to me.
  

Thanks for reviewing these patches!

"Kirill A. Shutemov" <kirill@shutemov.name> writes:

> On Fri, Mar 31, 2023 at 11:50:39PM +0000, Ackerley Tng wrote:

>> ...

>> +static int restrictedmem_create_on_user_mount(int mount_fd)
>> +{
>> +	int ret;
>> +	struct fd f;
>> +	struct vfsmount *mnt;
>> +
>> +	f = fdget_raw(mount_fd);
>> +	if (!f.file)
>> +		return -EBADF;
>> +
>> +	ret = -EINVAL;
>> +	if (!is_mount_root(f.file))
>> +		goto out;
>> +
>> +	mnt = f.file->f_path.mnt;
>> +	if (!is_shmem_mount(mnt))
>> +		goto out;
>> +
>> +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

> Why MAY_EXEC?


Christian pointed out that this check does not make sense, I'll be
removing the entire check in the next revision.

>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = mnt_want_write(mnt);
>> +	if (unlikely(ret))
>> +		goto out;
>> +
>> +	ret = restrictedmem_create(mnt);
>> +
>> +	mnt_drop_write(mnt);
>> +out:
>> +	fdput(f);
>> +
>> +	return ret;
>> +}

> We need review from fs folks. Look mostly sensible, but I have no
> experience in fs.

>> +
>> +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
>> +{
>> +	if (flags & ~RMFD_USERMNT)
>> +		return -EINVAL;
>> +
>> +	if (flags == RMFD_USERMNT) {
>> +		if (mount_fd < 0)
>> +			return -EINVAL;
>> +
>> +		return restrictedmem_create_on_user_mount(mount_fd);
>> +	} else {
>> +		return restrictedmem_create(NULL);
>> +	}

> Maybe restructure with single restrictedmem_create() call?

> 	struct vfsmount *mnt = NULL;

> 	if (flags == RMFD_USERMNT) {
> 		...
> 		mnt = ...();
> 	}

> 	return restrictedmem_create(mnt);

Will do so in the next revision.

>> +}
>> +
>>   int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
>>   		       struct restrictedmem_notifier *notifier, bool exclusive)
>>   {
>> --
>> 2.40.0.348.gf938b09366-goog
  

On Wed, Apr 05, 2023 at 09:58:44PM +0000, Ackerley Tng wrote:
> 
> Thanks again for your review!
> 
> Christian Brauner <brauner@kernel.org> writes:
> > On Tue, Apr 04, 2023 at 03:53:13PM +0200, Christian Brauner wrote:
> > > On Fri, Mar 31, 2023 at 11:50:39PM +0000, Ackerley Tng wrote:
> > > >
> > > > ...
> > > >
> > > > -SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
> > > > +static int restrictedmem_create(struct vfsmount *mount)
> > > >  {
> > > >  	struct file *file, *restricted_file;
> > > >  	int fd, err;
> > > >
> > > > -	if (flags)
> > > > -		return -EINVAL;
> > > > -
> > > >  	fd = get_unused_fd_flags(0);
> 
> > > Any reasons the file descriptors aren't O_CLOEXEC by default? I don't
> > > see any reasons why we should introduce new fdtypes that aren't
> > > O_CLOEXEC by default. The "don't mix-and-match" train has already left
> > > the station anyway as we do have seccomp noitifer fds and pidfds both of
> > > which are O_CLOEXEC by default.
> 
> 
> Thanks for pointing this out. I agree with using O_CLOEXEC, but didn’t
> notice this before. Let us discuss this under the original series at
> [1].
> 
> > > >  	if (fd < 0)
> > > >  		return fd;
> > > >
> > > > -	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> > > > +	if (mount)
> > > > +		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem",
> > > 0, VM_NORESERVE);
> > > > +	else
> > > > +		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
> > > > +
> > > >  	if (IS_ERR(file)) {
> > > >  		err = PTR_ERR(file);
> > > >  		goto err_fd;
> > > > @@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned
> > > int, flags)
> > > >  	return err;
> > > >  }
> > > >
> > > > +static bool is_shmem_mount(struct vfsmount *mnt)
> > > > +{
> > > > +	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
> 
> > > This can just be if (mnt->mnt_sb->s_magic == TMPFS_MAGIC).
> 
> 
> Will simplify this in the next revision.
> 
> > > > +}
> > > > +
> > > > +static bool is_mount_root(struct file *file)
> > > > +{
> > > > +	return file->f_path.dentry == file->f_path.mnt->mnt_root;
> 
> > > mount -t tmpfs tmpfs /mnt
> > > touch /mnt/bla
> > > touch /mnt/ble
> > > mount --bind /mnt/bla /mnt/ble
> > > fd = open("/mnt/ble")
> > > fd_restricted = memfd_restricted(fd)
> 
> > > IOW, this doesn't restrict it to the tmpfs root. It only restricts it to
> > > paths that refer to the root of any tmpfs mount. To exclude bind-mounts
> > > that aren't bind-mounts of the whole filesystem you want:
> 
> > > path->dentry == path->mnt->mnt_root &&
> > > path->mnt->mnt_root == path->mnt->mnt_sb->s_root
> 
> 
> Will adopt this in the next revision and add a selftest to check
> this. Thanks for pointing this out!
> 
> > > > +}
> > > > +
> > > > +static int restrictedmem_create_on_user_mount(int mount_fd)
> > > > +{
> > > > +	int ret;
> > > > +	struct fd f;
> > > > +	struct vfsmount *mnt;
> > > > +
> > > > +	f = fdget_raw(mount_fd);
> > > > +	if (!f.file)
> > > > +		return -EBADF;
> > > > +
> > > > +	ret = -EINVAL;
> > > > +	if (!is_mount_root(f.file))
> > > > +		goto out;
> > > > +
> > > > +	mnt = f.file->f_path.mnt;
> > > > +	if (!is_shmem_mount(mnt))
> > > > +		goto out;
> > > > +
> > > > +	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);
> 
> > > With the current semantics you're asking whether you have write
> > > permissions on the /mnt/ble file in order to get answer to the question
> > > whether you're allowed to create an unlinked restricted memory file.
> > > That doesn't make much sense afaict.
> 
> 
> That's true. Since mnt_want_write() already checks for write permissions
> and this syscall creates an unlinked file on the mount, we don't have to
> check permissions on the file then. Will remove this in the next
> revision!
> 
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	ret = mnt_want_write(mnt);
> > > > +	if (unlikely(ret))
> > > > +		goto out;
> > > > +
> > > > +	ret = restrictedmem_create(mnt);
> > > > +
> > > > +	mnt_drop_write(mnt);
> > > > +out:
> > > > +	fdput(f);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
> > > > +{
> > > > +	if (flags & ~RMFD_USERMNT)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (flags == RMFD_USERMNT) {
> 
> > > Why do you even need this flag? It seems that @mount_fd being < 0 is
> > > sufficient to indicate that a new restricted memory fd is supposed to be
> > > created in the system instance.
> 
> 
> I'm hoping to have this patch series merged after Chao's patch series
> introduces the memfd_restricted() syscall [1].

I'm curious, is there an LSFMM session for this?
  

Christian Brauner <brauner@kernel.org> writes:

> On Wed, Apr 05, 2023 at 09:58:44PM +0000, Ackerley Tng wrote:

>> ...

>> > > Why do you even need this flag? It seems that @mount_fd being < 0 is
>> > > sufficient to indicate that a new restricted memory fd is supposed  
>> to be
>> > > created in the system instance.


>> I'm hoping to have this patch series merged after Chao's patch series
>> introduces the memfd_restricted() syscall [1].

> I'm curious, is there an LSFMM session for this?

As far as I know, there is no LSFMM session for this.
  

On Thu, Apr 13, 2023, Ackerley Tng wrote:
> Christian Brauner <brauner@kernel.org> writes:
> > I'm curious, is there an LSFMM session for this?
> 
> As far as I know, there is no LSFMM session for this.

Correct, no LSFMM session.  In hindsight, that's obviously something we should
have pursued :-(