[PATCH 0/6] memfd: add sealing to hugetlb-backed memory

[PATCH 0/6] memfd: add sealing to hugetlb-backed memory

[Marc-André Lureau]

Hi,

Recently, Mike Kravetz added hugetlbfs support to memfd. However, he
didn't add sealing support. One of the reasons to use memfd is to have
shared memory sealing when doing IPC or sharing memory with another
process with some extra safety. qemu uses shared memory & hugetables
with vhost-user (used by dpdk), so it is reasonable to use memfd
now instead for convenience and security reasons.

Thanks!

RFC->v1:
- split rfc patch, after early review feedback
- added patch for memfd-test changes
- fix build with hugetlbfs disabled
- small code and commit messages improvements

Marc-André Lureau (6):
  shmem: unexport shmem_add_seals()/shmem_get_seals()
  shmem: rename functions that are memfd-related
  hugetlb: expose hugetlbfs_inode_info in header
  hugetlbfs: implement memfd sealing
  shmem: add sealing support to hugetlb-backed memfd
  memfd-tests: test hugetlbfs sealing

 fs/fcntl.c                                 |   2 +-
 fs/hugetlbfs/inode.c                       |  39 +++++---
 include/linux/hugetlb.h                    |  11 +++
 include/linux/shmem_fs.h                   |   6 +-
 mm/shmem.c                                 |  59 +++++++-----
 tools/testing/selftests/memfd/memfd_test.c | 150 +++--------------------------
 6 files changed, 89 insertions(+), 178 deletions(-)

-- 
2.15.0.rc0.40.gaefcc5f6f

[PATCH 1/6] shmem: unexport shmem_add_seals()/shmem_get_seals()

[Marc-André Lureau]

The functions are called through shmem_fcntl() only.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/linux/shmem_fs.h | 2 --
 mm/shmem.c               | 6 ++----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index b6c3540e07bc..557d0c3b6eca 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -109,8 +109,6 @@ extern void shmem_uncharge(struct inode *inode, long pages);
 
 #ifdef CONFIG_TMPFS
 
-extern int shmem_add_seals(struct file *file, unsigned int seals);
-extern int shmem_get_seals(struct file *file);
 extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
 
 #else
diff --git a/mm/shmem.c b/mm/shmem.c
index 07a1d22807be..37260c5e12fa 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space *mapping)
 		     F_SEAL_GROW | \
 		     F_SEAL_WRITE)
 
-int shmem_add_seals(struct file *file, unsigned int seals)
+static int shmem_add_seals(struct file *file, unsigned int seals)
 {
 	struct inode *inode = file_inode(file);
 	struct shmem_inode_info *info = SHMEM_I(inode);
@@ -2791,16 +2791,14 @@ int shmem_add_seals(struct file *file, unsigned int seals)
 	inode_unlock(inode);
 	return error;
 }
-EXPORT_SYMBOL_GPL(shmem_add_seals);
 
-int shmem_get_seals(struct file *file)
+static int shmem_get_seals(struct file *file)
 {
 	if (file->f_op != &shmem_file_operations)
 		return -EINVAL;
 
 	return SHMEM_I(file_inode(file))->seals;
 }
-EXPORT_SYMBOL_GPL(shmem_get_seals);
 
 long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-- 
2.15.0.rc0.40.gaefcc5f6f

[PATCH 2/6] shmem: rename functions that are memfd-related

[Marc-André Lureau]

Those functions are called for memfd files, backed by shmem or
hugetlb (the next patches will handle hugetlb).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 fs/fcntl.c               |  2 +-
 include/linux/shmem_fs.h |  4 ++--
 mm/shmem.c               | 10 +++++-----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 448a1119f0be..752c23743616 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -417,7 +417,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 		break;
 	case F_ADD_SEALS:
 	case F_GET_SEALS:
-		err = shmem_fcntl(filp, cmd, arg);
+		err = memfd_fcntl(filp, cmd, arg);
 		break;
 	case F_GET_RW_HINT:
 	case F_SET_RW_HINT:
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index 557d0c3b6eca..0dac8c0f4aa4 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -109,11 +109,11 @@ extern void shmem_uncharge(struct inode *inode, long pages);
 
 #ifdef CONFIG_TMPFS
 
-extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
+extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
 
 #else
 
-static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned long a)
+static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned long a)
 {
 	return -EINVAL;
 }
diff --git a/mm/shmem.c b/mm/shmem.c
index 37260c5e12fa..b7811979611f 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space *mapping)
 		     F_SEAL_GROW | \
 		     F_SEAL_WRITE)
 
-static int shmem_add_seals(struct file *file, unsigned int seals)
+static int memfd_add_seals(struct file *file, unsigned int seals)
 {
 	struct inode *inode = file_inode(file);
 	struct shmem_inode_info *info = SHMEM_I(inode);
@@ -2792,7 +2792,7 @@ static int shmem_add_seals(struct file *file, unsigned int seals)
 	return error;
 }
 
-static int shmem_get_seals(struct file *file)
+static int memfd_get_seals(struct file *file)
 {
 	if (file->f_op != &shmem_file_operations)
 		return -EINVAL;
@@ -2800,7 +2800,7 @@ static int shmem_get_seals(struct file *file)
 	return SHMEM_I(file_inode(file))->seals;
 }
 
-long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
+long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long error;
 
@@ -2810,10 +2810,10 @@ long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (arg > UINT_MAX)
 			return -EINVAL;
 
-		error = shmem_add_seals(file, arg);
+		error = memfd_add_seals(file, arg);
 		break;
 	case F_GET_SEALS:
-		error = shmem_get_seals(file);
+		error = memfd_get_seals(file);
 		break;
 	default:
 		error = -EINVAL;
-- 
2.15.0.rc0.40.gaefcc5f6f

[PATCH 3/6] hugetlb: expose hugetlbfs_inode_info in header

[Marc-André Lureau]

The following patch is going to access hugetlbfs_inode_info field from
mm/shmem.c.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 fs/hugetlbfs/inode.c    | 10 ----------
 include/linux/hugetlb.h | 10 ++++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 59073e9f01a4..ea7b10357ac4 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -55,16 +55,6 @@ struct hugetlbfs_config {
 	umode_t			mode;
 };
 
-struct hugetlbfs_inode_info {
-	struct shared_policy policy;
-	struct inode vfs_inode;
-};
-
-static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
-{
-	return container_of(inode, struct hugetlbfs_inode_info, vfs_inode);
-}
-
 int sysctl_hugetlb_shm_group;
 
 enum {
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 8bbbd37ab105..f78daf54897d 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -278,6 +278,16 @@ static inline struct hugetlbfs_sb_info *HUGETLBFS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
+struct hugetlbfs_inode_info {
+	struct shared_policy policy;
+	struct inode vfs_inode;
+};
+
+static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
+{
+	return container_of(inode, struct hugetlbfs_inode_info, vfs_inode);
+}
+
 extern const struct file_operations hugetlbfs_file_operations;
 extern const struct vm_operations_struct hugetlb_vm_ops;
 struct file *hugetlb_file_setup(const char *name, size_t size, vm_flags_t acct,
-- 
2.15.0.rc0.40.gaefcc5f6f

[PATCH 4/6] hugetlbfs: implement memfd sealing

[Marc-André Lureau]

Implements memfd sealing, similar to shmem:
- WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
  memfd_add_seals(). write() doesn't exist for hugetlbfs.
- SHRINK: added similar check as shmem_setattr()
- GROW: added similar check as shmem_setattr() & shmem_fallocate()

Except write() operation that doesn't exist with hugetlbfs, that
should make sealing as close as it can be to shmem support.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 fs/hugetlbfs/inode.c    | 29 +++++++++++++++++++++++++++--
 include/linux/hugetlb.h |  1 +
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index ea7b10357ac4..62d70b1b1ab9 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -510,8 +510,16 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 
 	if (hole_end > hole_start) {
 		struct address_space *mapping = inode->i_mapping;
+		struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
 
 		inode_lock(inode);
+
+		/* protected by i_mutex */
+		if (info->seals & F_SEAL_WRITE) {
+			inode_unlock(inode);
+			return -EPERM;
+		}
+
 		i_mmap_lock_write(mapping);
 		if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
 			hugetlb_vmdelete_list(&mapping->i_mmap,
@@ -529,6 +537,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 				loff_t len)
 {
 	struct inode *inode = file_inode(file);
+	struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
 	struct address_space *mapping = inode->i_mapping;
 	struct hstate *h = hstate_inode(inode);
 	struct vm_area_struct pseudo_vma;
@@ -560,6 +569,11 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 	if (error)
 		goto out;
 
+	if ((info->seals & F_SEAL_GROW) && offset + len > inode->i_size) {
+		error = -EPERM;
+		goto out;
+	}
+
 	/*
 	 * Initialize a pseudo vma as this is required by the huge page
 	 * allocation routines.  If NUMA is configured, use page index
@@ -650,6 +664,7 @@ static int hugetlbfs_setattr(struct dentry *dentry, struct iattr *attr)
 	struct hstate *h = hstate_inode(inode);
 	int error;
 	unsigned int ia_valid = attr->ia_valid;
+	struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
 
 	BUG_ON(!inode);
 
@@ -658,10 +673,17 @@ static int hugetlbfs_setattr(struct dentry *dentry, struct iattr *attr)
 		return error;
 
 	if (ia_valid & ATTR_SIZE) {
+		loff_t oldsize = inode->i_size;
+		loff_t newsize = attr->ia_size;
+
 		error = -EINVAL;
-		if (attr->ia_size & ~huge_page_mask(h))
+		if (newsize & ~huge_page_mask(h))
 			return -EINVAL;
-		error = hugetlb_vmtruncate(inode, attr->ia_size);
+		/* protected by i_mutex */
+		if ((newsize < oldsize && (info->seals & F_SEAL_SHRINK)) ||
+		    (newsize > oldsize && (info->seals & F_SEAL_GROW)))
+			return -EPERM;
+		error = hugetlb_vmtruncate(inode, newsize);
 		if (error)
 			return error;
 	}
@@ -713,6 +735,8 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 
 	inode = new_inode(sb);
 	if (inode) {
+		struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
+
 		inode->i_ino = get_next_ino();
 		inode_init_owner(inode, dir, mode);
 		lockdep_set_class(&inode->i_mapping->i_mmap_rwsem,
@@ -720,6 +744,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 		inode->i_mapping->a_ops = &hugetlbfs_aops;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
 		inode->i_mapping->private_data = resv_map;
+		info->seals = F_SEAL_SEAL;
 		switch (mode & S_IFMT) {
 		default:
 			init_special_inode(inode, mode, dev);
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index f78daf54897d..128ef10902f3 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -281,6 +281,7 @@ static inline struct hugetlbfs_sb_info *HUGETLBFS_SB(struct super_block *sb)
 struct hugetlbfs_inode_info {
 	struct shared_policy policy;
 	struct inode vfs_inode;
+	unsigned int seals;
 };
 
 static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
-- 
2.15.0.rc0.40.gaefcc5f6f

[PATCH 5/6] shmem: add sealing support to hugetlb-backed memfd

[Marc-André Lureau]

Adapt add_seals()/get_seals() to work with hugetbfs-backed memory.

Teach memfd_create() to allow sealing operations on MFD_HUGETLB.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 mm/shmem.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index b7811979611f..b7c59d993c19 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2717,6 +2717,19 @@ static int shmem_wait_for_pins(struct address_space *mapping)
 	return error;
 }
 
+static unsigned int *memfd_get_seals(struct file *file)
+{
+	if (file->f_op == &shmem_file_operations)
+		return &SHMEM_I(file_inode(file))->seals;
+
+#ifdef CONFIG_HUGETLBFS
+	if (file->f_op == &hugetlbfs_file_operations)
+		return &HUGETLBFS_I(file_inode(file))->seals;
+#endif
+
+	return NULL;
+}
+
 #define F_ALL_SEALS (F_SEAL_SEAL | \
 		     F_SEAL_SHRINK | \
 		     F_SEAL_GROW | \
@@ -2725,7 +2738,7 @@ static int shmem_wait_for_pins(struct address_space *mapping)
 static int memfd_add_seals(struct file *file, unsigned int seals)
 {
 	struct inode *inode = file_inode(file);
-	struct shmem_inode_info *info = SHMEM_I(inode);
+	unsigned int *file_seals;
 	int error;
 
 	/*
@@ -2758,8 +2771,6 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
 	 * other file types.
 	 */
 
-	if (file->f_op != &shmem_file_operations)
-		return -EINVAL;
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EPERM;
 	if (seals & ~(unsigned int)F_ALL_SEALS)
@@ -2767,12 +2778,18 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
 
 	inode_lock(inode);
 
-	if (info->seals & F_SEAL_SEAL) {
+	file_seals = memfd_get_seals(file);
+	if (!file_seals) {
+		error = -EINVAL;
+		goto unlock;
+	}
+
+	if (*file_seals & F_SEAL_SEAL) {
 		error = -EPERM;
 		goto unlock;
 	}
 
-	if ((seals & F_SEAL_WRITE) && !(info->seals & F_SEAL_WRITE)) {
+	if ((seals & F_SEAL_WRITE) && !(*file_seals & F_SEAL_WRITE)) {
 		error = mapping_deny_writable(file->f_mapping);
 		if (error)
 			goto unlock;
@@ -2784,7 +2801,7 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
 		}
 	}
 
-	info->seals |= seals;
+	*file_seals |= seals;
 	error = 0;
 
 unlock:
@@ -2792,12 +2809,11 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
 	return error;
 }
 
-static int memfd_get_seals(struct file *file)
+static int memfd_fcntl_get_seals(struct file *file)
 {
-	if (file->f_op != &shmem_file_operations)
-		return -EINVAL;
+	unsigned int *seals = memfd_get_seals(file);
 
-	return SHMEM_I(file_inode(file))->seals;
+	return seals ? *seals : -EINVAL;
 }
 
 long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
@@ -2813,7 +2829,7 @@ long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 		error = memfd_add_seals(file, arg);
 		break;
 	case F_GET_SEALS:
-		error = memfd_get_seals(file);
+		error = memfd_fcntl_get_seals(file);
 		break;
 	default:
 		error = -EINVAL;
@@ -3657,7 +3673,7 @@ SYSCALL_DEFINE2(memfd_create,
 		const char __user *, uname,
 		unsigned int, flags)
 {
-	struct shmem_inode_info *info;
+	unsigned int *file_seals;
 	struct file *file;
 	int fd, error;
 	char *name;
@@ -3667,9 +3683,6 @@ SYSCALL_DEFINE2(memfd_create,
 		if (flags & ~(unsigned int)MFD_ALL_FLAGS)
 			return -EINVAL;
 	} else {
-		/* Sealing not supported in hugetlbfs (MFD_HUGETLB) */
-		if (flags & MFD_ALLOW_SEALING)
-			return -EINVAL;
 		/* Allow huge page size encoding in flags. */
 		if (flags & ~(unsigned int)(MFD_ALL_FLAGS |
 				(MFD_HUGE_MASK << MFD_HUGE_SHIFT)))
@@ -3722,12 +3735,8 @@ SYSCALL_DEFINE2(memfd_create,
 	file->f_flags |= O_RDWR | O_LARGEFILE;
 
 	if (flags & MFD_ALLOW_SEALING) {
-		/*
-		 * flags check at beginning of function ensures
-		 * this is not a hugetlbfs (MFD_HUGETLB) file.
-		 */
-		info = SHMEM_I(file_inode(file));
-		info->seals &= ~F_SEAL_SEAL;
+		file_seals = memfd_get_seals(file);
+		*file_seals &= ~F_SEAL_SEAL;
 	}
 
 	fd_install(fd, file);
-- 
2.15.0.rc0.40.gaefcc5f6f

[PATCH 6/6] memfd-tests: test hugetlbfs sealing

[Marc-André Lureau]

Remove most of the special-casing of hugetlbfs now that sealing
is supported.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tools/testing/selftests/memfd/memfd_test.c | 150 +++--------------------------
 1 file changed, 15 insertions(+), 135 deletions(-)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index f94c6d1fb46f..f5028f800107 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -512,6 +512,10 @@ static void mfd_assert_grow_write(int fd)
 	static char *buf;
 	ssize_t l;
 
+	/* hugetlbfs does not support write */
+	if (hugetlbfs_test)
+		return;
+
 	buf = malloc(mfd_def_size * 8);
 	if (!buf) {
 		printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
@@ -532,6 +536,10 @@ static void mfd_fail_grow_write(int fd)
 	static char *buf;
 	ssize_t l;
 
+	/* hugetlbfs does not support write */
+	if (hugetlbfs_test)
+		return;
+
 	buf = malloc(mfd_def_size * 8);
 	if (!buf) {
 		printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
@@ -626,18 +634,13 @@ static void test_create(void)
 	fd = mfd_assert_new("", 0, MFD_CLOEXEC);
 	close(fd);
 
-	if (!hugetlbfs_test) {
-		/* verify MFD_ALLOW_SEALING is allowed */
-		fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
-		close(fd);
-
-		/* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
-		fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
-		close(fd);
-	} else {
-		/* sealing is not supported on hugetlbfs */
-		mfd_fail_new("", MFD_ALLOW_SEALING);
-	}
+	/* verify MFD_ALLOW_SEALING is allowed */
+	fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
+	close(fd);
+
+	/* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
+	fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
+	close(fd);
 }
 
 /*
@@ -648,10 +651,6 @@ static void test_basic(void)
 {
 	int fd;
 
-	/* hugetlbfs does not contain sealing support */
-	if (hugetlbfs_test)
-		return;
-
 	printf("%s BASIC\n", MEMFD_STR);
 
 	fd = mfd_assert_new("kern_memfd_basic",
@@ -696,28 +695,6 @@ static void test_basic(void)
 	close(fd);
 }
 
-/*
- * hugetlbfs doesn't support seals or write, so just verify grow and shrink
- * on a hugetlbfs file created via memfd_create.
- */
-static void test_hugetlbfs_grow_shrink(void)
-{
-	int fd;
-
-	printf("%s HUGETLBFS-GROW-SHRINK\n", MEMFD_STR);
-
-	fd = mfd_assert_new("kern_memfd_seal_write",
-			    mfd_def_size,
-			    MFD_CLOEXEC);
-
-	mfd_assert_read(fd);
-	mfd_assert_write(fd);
-	mfd_assert_shrink(fd);
-	mfd_assert_grow(fd);
-
-	close(fd);
-}
-
 /*
  * Test SEAL_WRITE
  * Test whether SEAL_WRITE actually prevents modifications.
@@ -726,13 +703,6 @@ static void test_seal_write(void)
 {
 	int fd;
 
-	/*
-	 * hugetlbfs does not contain sealing or write support.  Just test
-	 * basic grow and shrink via test_hugetlbfs_grow_shrink.
-	 */
-	if (hugetlbfs_test)
-		return test_hugetlbfs_grow_shrink();
-
 	printf("%s SEAL-WRITE\n", MEMFD_STR);
 
 	fd = mfd_assert_new("kern_memfd_seal_write",
@@ -759,10 +729,6 @@ static void test_seal_shrink(void)
 {
 	int fd;
 
-	/* hugetlbfs does not contain sealing support */
-	if (hugetlbfs_test)
-		return;
-
 	printf("%s SEAL-SHRINK\n", MEMFD_STR);
 
 	fd = mfd_assert_new("kern_memfd_seal_shrink",
@@ -789,10 +755,6 @@ static void test_seal_grow(void)
 {
 	int fd;
 
-	/* hugetlbfs does not contain sealing support */
-	if (hugetlbfs_test)
-		return;
-
 	printf("%s SEAL-GROW\n", MEMFD_STR);
 
 	fd = mfd_assert_new("kern_memfd_seal_grow",
@@ -819,10 +781,6 @@ static void test_seal_resize(void)
 {
 	int fd;
 
-	/* hugetlbfs does not contain sealing support */
-	if (hugetlbfs_test)
-		return;
-
 	printf("%s SEAL-RESIZE\n", MEMFD_STR);
 
 	fd = mfd_assert_new("kern_memfd_seal_resize",
@@ -841,32 +799,6 @@ static void test_seal_resize(void)
 	close(fd);
 }
 
-/*
- * hugetlbfs does not support seals.  Basic test to dup the memfd created
- * fd and perform some basic operations on it.
- */
-static void hugetlbfs_dup(char *b_suffix)
-{
-	int fd, fd2;
-
-	printf("%s HUGETLBFS-DUP %s\n", MEMFD_STR, b_suffix);
-
-	fd = mfd_assert_new("kern_memfd_share_dup",
-			    mfd_def_size,
-			    MFD_CLOEXEC);
-
-	fd2 = mfd_assert_dup(fd);
-
-	mfd_assert_read(fd);
-	mfd_assert_write(fd);
-
-	mfd_assert_shrink(fd2);
-	mfd_assert_grow(fd2);
-
-	close(fd2);
-	close(fd);
-}
-
 /*
  * Test sharing via dup()
  * Test that seals are shared between dupped FDs and they're all equal.
@@ -875,15 +807,6 @@ static void test_share_dup(char *banner, char *b_suffix)
 {
 	int fd, fd2;
 
-	/*
-	 * hugetlbfs does not contain sealing support.  Perform some
-	 * basic testing on dup'ed fd instead via hugetlbfs_dup.
-	 */
-	if (hugetlbfs_test) {
-		hugetlbfs_dup(b_suffix);
-		return;
-	}
-
 	printf("%s %s %s\n", MEMFD_STR, banner, b_suffix);
 
 	fd = mfd_assert_new("kern_memfd_share_dup",
@@ -926,10 +849,6 @@ static void test_share_mmap(char *banner, char *b_suffix)
 	int fd;
 	void *p;
 
-	/* hugetlbfs does not contain sealing support */
-	if (hugetlbfs_test)
-		return;
-
 	printf("%s %s %s\n", MEMFD_STR,  banner, b_suffix);
 
 	fd = mfd_assert_new("kern_memfd_share_mmap",
@@ -954,32 +873,6 @@ static void test_share_mmap(char *banner, char *b_suffix)
 	close(fd);
 }
 
-/*
- * Basic test to make sure we can open the hugetlbfs fd via /proc and
- * perform some simple operations on it.
- */
-static void hugetlbfs_proc_open(char *b_suffix)
-{
-	int fd, fd2;
-
-	printf("%s HUGETLBFS-PROC-OPEN %s\n", MEMFD_STR, b_suffix);
-
-	fd = mfd_assert_new("kern_memfd_share_open",
-			    mfd_def_size,
-			    MFD_CLOEXEC);
-
-	fd2 = mfd_assert_open(fd, O_RDWR, 0);
-
-	mfd_assert_read(fd);
-	mfd_assert_write(fd);
-
-	mfd_assert_shrink(fd2);
-	mfd_assert_grow(fd2);
-
-	close(fd2);
-	close(fd);
-}
-
 /*
  * Test sealing with open(/proc/self/fd/%d)
  * Via /proc we can get access to a separate file-context for the same memfd.
@@ -990,15 +883,6 @@ static void test_share_open(char *banner, char *b_suffix)
 {
 	int fd, fd2;
 
-	/*
-	 * hugetlbfs does not contain sealing support.  So test basic
-	 * functionality of using /proc fd via hugetlbfs_proc_open
-	 */
-	if (hugetlbfs_test) {
-		hugetlbfs_proc_open(b_suffix);
-		return;
-	}
-
 	printf("%s %s %s\n", MEMFD_STR, banner, b_suffix);
 
 	fd = mfd_assert_new("kern_memfd_share_open",
@@ -1042,10 +926,6 @@ static void test_share_fork(char *banner, char *b_suffix)
 	int fd;
 	pid_t pid;
 
-	/* hugetlbfs does not contain sealing support */
-	if (hugetlbfs_test)
-		return;
-
 	printf("%s %s %s\n", MEMFD_STR, banner, b_suffix);
 
 	fd = mfd_assert_new("kern_memfd_share_fork",
-- 
2.15.0.rc0.40.gaefcc5f6f

Re: [PATCH 1/6] shmem: unexport shmem_add_seals()/shmem_get_seals()

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> The functions are called through shmem_fcntl() only.

And no danger in removing the EXPORTs as the routines only work
with shmem file structs.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

> ---
>  include/linux/shmem_fs.h | 2 --
>  mm/shmem.c               | 6 ++----
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index b6c3540e07bc..557d0c3b6eca 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -109,8 +109,6 @@ extern void shmem_uncharge(struct inode *inode, long pages);
>  
>  #ifdef CONFIG_TMPFS
>  
> -extern int shmem_add_seals(struct file *file, unsigned int seals);
> -extern int shmem_get_seals(struct file *file);
>  extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
>  
>  #else
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 07a1d22807be..37260c5e12fa 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space *mapping)
>  		     F_SEAL_GROW | \
>  		     F_SEAL_WRITE)
>  
> -int shmem_add_seals(struct file *file, unsigned int seals)
> +static int shmem_add_seals(struct file *file, unsigned int seals)
>  {
>  	struct inode *inode = file_inode(file);
>  	struct shmem_inode_info *info = SHMEM_I(inode);
> @@ -2791,16 +2791,14 @@ int shmem_add_seals(struct file *file, unsigned int seals)
>  	inode_unlock(inode);
>  	return error;
>  }
> -EXPORT_SYMBOL_GPL(shmem_add_seals);
>  
> -int shmem_get_seals(struct file *file)
> +static int shmem_get_seals(struct file *file)
>  {
>  	if (file->f_op != &shmem_file_operations)
>  		return -EINVAL;
>  
>  	return SHMEM_I(file_inode(file))->seals;
>  }
> -EXPORT_SYMBOL_GPL(shmem_get_seals);
>  
>  long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
> 

Re: [PATCH 2/6] shmem: rename functions that are memfd-related

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> Those functions are called for memfd files, backed by shmem or
> hugetlb (the next patches will handle hugetlb).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  fs/fcntl.c               |  2 +-
>  include/linux/shmem_fs.h |  4 ++--
>  mm/shmem.c               | 10 +++++-----
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 448a1119f0be..752c23743616 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -417,7 +417,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
>  		break;
>  	case F_ADD_SEALS:
>  	case F_GET_SEALS:
> -		err = shmem_fcntl(filp, cmd, arg);
> +		err = memfd_fcntl(filp, cmd, arg);
>  		break;
>  	case F_GET_RW_HINT:
>  	case F_SET_RW_HINT:
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 557d0c3b6eca..0dac8c0f4aa4 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -109,11 +109,11 @@ extern void shmem_uncharge(struct inode *inode, long pages);
>  
>  #ifdef CONFIG_TMPFS
>  
> -extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
> +extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
>  
>  #else
>  
> -static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned long a)
> +static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned long a)
>  {
>  	return -EINVAL;
>  }

Do we want memfd_fcntl() to work for hugetlbfs if CONFIG_TMPFS is not
defined?  I admit that having CONFIG_HUGETLBFS defined without CONFIG_TMPFS
is unlikely, but I think possible.  Based on the above #ifdef/#else, I
think hugetlbfs seals will not work if CONFIG_TMPFS is not defined.

-- 
Mike Kravetz

> diff --git a/mm/shmem.c b/mm/shmem.c
> index 37260c5e12fa..b7811979611f 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space *mapping)
>  		     F_SEAL_GROW | \
>  		     F_SEAL_WRITE)
>  
> -static int shmem_add_seals(struct file *file, unsigned int seals)
> +static int memfd_add_seals(struct file *file, unsigned int seals)
>  {
>  	struct inode *inode = file_inode(file);
>  	struct shmem_inode_info *info = SHMEM_I(inode);
> @@ -2792,7 +2792,7 @@ static int shmem_add_seals(struct file *file, unsigned int seals)
>  	return error;
>  }
>  
> -static int shmem_get_seals(struct file *file)
> +static int memfd_get_seals(struct file *file)
>  {
>  	if (file->f_op != &shmem_file_operations)
>  		return -EINVAL;
> @@ -2800,7 +2800,7 @@ static int shmem_get_seals(struct file *file)
>  	return SHMEM_I(file_inode(file))->seals;
>  }
>  
> -long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> +long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	long error;
>  
> @@ -2810,10 +2810,10 @@ long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>  		if (arg > UINT_MAX)
>  			return -EINVAL;
>  
> -		error = shmem_add_seals(file, arg);
> +		error = memfd_add_seals(file, arg);
>  		break;
>  	case F_GET_SEALS:
> -		error = shmem_get_seals(file);
> +		error = memfd_get_seals(file);
>  		break;
>  	default:
>  		error = -EINVAL;
> 

Re: [PATCH 3/6] hugetlb: expose hugetlbfs_inode_info in header

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> The following patch is going to access hugetlbfs_inode_info field from
> mm/shmem.c.

The code looks fine.  However, I would prefer something different for the
commit message.  Perhaps something like:

hugetlbfs inode information will need to be accessed by code in mm/shmem.c
for file sealing operations.  Move inode information definition from .c
file to header for needed access.

-- 
Mike Kravetz

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  fs/hugetlbfs/inode.c    | 10 ----------
>  include/linux/hugetlb.h | 10 ++++++++++
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 59073e9f01a4..ea7b10357ac4 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -55,16 +55,6 @@ struct hugetlbfs_config {
>  	umode_t			mode;
>  };
>  
> -struct hugetlbfs_inode_info {
> -	struct shared_policy policy;
> -	struct inode vfs_inode;
> -};
> -
> -static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
> -{
> -	return container_of(inode, struct hugetlbfs_inode_info, vfs_inode);
> -}
> -
>  int sysctl_hugetlb_shm_group;
>  
>  enum {
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 8bbbd37ab105..f78daf54897d 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -278,6 +278,16 @@ static inline struct hugetlbfs_sb_info *HUGETLBFS_SB(struct super_block *sb)
>  	return sb->s_fs_info;
>  }
>  
> +struct hugetlbfs_inode_info {
> +	struct shared_policy policy;
> +	struct inode vfs_inode;
> +};
> +
> +static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
> +{
> +	return container_of(inode, struct hugetlbfs_inode_info, vfs_inode);
> +}
> +
>  extern const struct file_operations hugetlbfs_file_operations;
>  extern const struct vm_operations_struct hugetlb_vm_ops;
>  struct file *hugetlb_file_setup(const char *name, size_t size, vm_flags_t acct,
> 

Re: [PATCH 4/6] hugetlbfs: implement memfd sealing

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> Implements memfd sealing, similar to shmem:
> - WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
>   memfd_add_seals(). write() doesn't exist for hugetlbfs.
> - SHRINK: added similar check as shmem_setattr()
> - GROW: added similar check as shmem_setattr() & shmem_fallocate()
> 
> Except write() operation that doesn't exist with hugetlbfs, that
> should make sealing as close as it can be to shmem support.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Looks fine to me,
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

> ---
>  fs/hugetlbfs/inode.c    | 29 +++++++++++++++++++++++++++--
>  include/linux/hugetlb.h |  1 +
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index ea7b10357ac4..62d70b1b1ab9 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -510,8 +510,16 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
>  
>  	if (hole_end > hole_start) {
>  		struct address_space *mapping = inode->i_mapping;
> +		struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
>  
>  		inode_lock(inode);
> +
> +		/* protected by i_mutex */
> +		if (info->seals & F_SEAL_WRITE) {
> +			inode_unlock(inode);
> +			return -EPERM;
> +		}
> +
>  		i_mmap_lock_write(mapping);
>  		if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
>  			hugetlb_vmdelete_list(&mapping->i_mmap,
> @@ -529,6 +537,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  				loff_t len)
>  {
>  	struct inode *inode = file_inode(file);
> +	struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
>  	struct address_space *mapping = inode->i_mapping;
>  	struct hstate *h = hstate_inode(inode);
>  	struct vm_area_struct pseudo_vma;
> @@ -560,6 +569,11 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  	if (error)
>  		goto out;
>  
> +	if ((info->seals & F_SEAL_GROW) && offset + len > inode->i_size) {
> +		error = -EPERM;
> +		goto out;
> +	}
> +
>  	/*
>  	 * Initialize a pseudo vma as this is required by the huge page
>  	 * allocation routines.  If NUMA is configured, use page index
> @@ -650,6 +664,7 @@ static int hugetlbfs_setattr(struct dentry *dentry, struct iattr *attr)
>  	struct hstate *h = hstate_inode(inode);
>  	int error;
>  	unsigned int ia_valid = attr->ia_valid;
> +	struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
>  
>  	BUG_ON(!inode);
>  
> @@ -658,10 +673,17 @@ static int hugetlbfs_setattr(struct dentry *dentry, struct iattr *attr)
>  		return error;
>  
>  	if (ia_valid & ATTR_SIZE) {
> +		loff_t oldsize = inode->i_size;
> +		loff_t newsize = attr->ia_size;
> +
>  		error = -EINVAL;
> -		if (attr->ia_size & ~huge_page_mask(h))
> +		if (newsize & ~huge_page_mask(h))
>  			return -EINVAL;
> -		error = hugetlb_vmtruncate(inode, attr->ia_size);
> +		/* protected by i_mutex */
> +		if ((newsize < oldsize && (info->seals & F_SEAL_SHRINK)) ||
> +		    (newsize > oldsize && (info->seals & F_SEAL_GROW)))
> +			return -EPERM;
> +		error = hugetlb_vmtruncate(inode, newsize);
>  		if (error)
>  			return error;
>  	}
> @@ -713,6 +735,8 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
>  
>  	inode = new_inode(sb);
>  	if (inode) {
> +		struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
> +
>  		inode->i_ino = get_next_ino();
>  		inode_init_owner(inode, dir, mode);
>  		lockdep_set_class(&inode->i_mapping->i_mmap_rwsem,
> @@ -720,6 +744,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
>  		inode->i_mapping->a_ops = &hugetlbfs_aops;
>  		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
>  		inode->i_mapping->private_data = resv_map;
> +		info->seals = F_SEAL_SEAL;
>  		switch (mode & S_IFMT) {
>  		default:
>  			init_special_inode(inode, mode, dev);
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index f78daf54897d..128ef10902f3 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -281,6 +281,7 @@ static inline struct hugetlbfs_sb_info *HUGETLBFS_SB(struct super_block *sb)
>  struct hugetlbfs_inode_info {
>  	struct shared_policy policy;
>  	struct inode vfs_inode;
> +	unsigned int seals;
>  };
>  
>  static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
> 

Re: [PATCH 5/6] shmem: add sealing support to hugetlb-backed memfd

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> Adapt add_seals()/get_seals() to work with hugetbfs-backed memory.
> 
> Teach memfd_create() to allow sealing operations on MFD_HUGETLB.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  mm/shmem.c | 51 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b7811979611f..b7c59d993c19 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2717,6 +2717,19 @@ static int shmem_wait_for_pins(struct address_space *mapping)
>  	return error;
>  }
>  
> +static unsigned int *memfd_get_seals(struct file *file)

I would have named this something like 'memfd_file_seal_ptr', and not
changed the name of memfd_get_seals below.  Just my preference, and it
does not carry as much weight as Hugh who originally write this code.

> +{
> +	if (file->f_op == &shmem_file_operations)
> +		return &SHMEM_I(file_inode(file))->seals;
> +
> +#ifdef CONFIG_HUGETLBFS
> +	if (file->f_op == &hugetlbfs_file_operations)
> +		return &HUGETLBFS_I(file_inode(file))->seals;
> +#endif
> +
> +	return NULL;
> +}
> +

As mentioned in patch 2, I think this code will need to be restructured
so that hugetlbfs file sealing will work even is CONFIG_TMPFS is not
defined.  The above routine is behind #ifdef CONFIG_TMPFS.

In general the code looks fine, but this config issue needs to be addressed.
-- 
Mike Kravetz

>  #define F_ALL_SEALS (F_SEAL_SEAL | \
>  		     F_SEAL_SHRINK | \
>  		     F_SEAL_GROW | \
> @@ -2725,7 +2738,7 @@ static int shmem_wait_for_pins(struct address_space *mapping)
>  static int memfd_add_seals(struct file *file, unsigned int seals)
>  {
>  	struct inode *inode = file_inode(file);
> -	struct shmem_inode_info *info = SHMEM_I(inode);
> +	unsigned int *file_seals;
>  	int error;
>  
>  	/*
> @@ -2758,8 +2771,6 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>  	 * other file types.
>  	 */
>  
> -	if (file->f_op != &shmem_file_operations)
> -		return -EINVAL;
>  	if (!(file->f_mode & FMODE_WRITE))
>  		return -EPERM;
>  	if (seals & ~(unsigned int)F_ALL_SEALS)
> @@ -2767,12 +2778,18 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>  
>  	inode_lock(inode);
>  
> -	if (info->seals & F_SEAL_SEAL) {
> +	file_seals = memfd_get_seals(file);
> +	if (!file_seals) {
> +		error = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (*file_seals & F_SEAL_SEAL) {
>  		error = -EPERM;
>  		goto unlock;
>  	}
>  
> -	if ((seals & F_SEAL_WRITE) && !(info->seals & F_SEAL_WRITE)) {
> +	if ((seals & F_SEAL_WRITE) && !(*file_seals & F_SEAL_WRITE)) {
>  		error = mapping_deny_writable(file->f_mapping);
>  		if (error)
>  			goto unlock;
> @@ -2784,7 +2801,7 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>  		}
>  	}
>  
> -	info->seals |= seals;
> +	*file_seals |= seals;
>  	error = 0;
>  
>  unlock:
> @@ -2792,12 +2809,11 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>  	return error;
>  }
>  
> -static int memfd_get_seals(struct file *file)
> +static int memfd_fcntl_get_seals(struct file *file)
>  {
> -	if (file->f_op != &shmem_file_operations)
> -		return -EINVAL;
> +	unsigned int *seals = memfd_get_seals(file);
>  
> -	return SHMEM_I(file_inode(file))->seals;
> +	return seals ? *seals : -EINVAL;
>  }
>  
>  long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> @@ -2813,7 +2829,7 @@ long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>  		error = memfd_add_seals(file, arg);
>  		break;
>  	case F_GET_SEALS:
> -		error = memfd_get_seals(file);
> +		error = memfd_fcntl_get_seals(file);
>  		break;
>  	default:
>  		error = -EINVAL;
> @@ -3657,7 +3673,7 @@ SYSCALL_DEFINE2(memfd_create,
>  		const char __user *, uname,
>  		unsigned int, flags)
>  {
> -	struct shmem_inode_info *info;
> +	unsigned int *file_seals;
>  	struct file *file;
>  	int fd, error;
>  	char *name;
> @@ -3667,9 +3683,6 @@ SYSCALL_DEFINE2(memfd_create,
>  		if (flags & ~(unsigned int)MFD_ALL_FLAGS)
>  			return -EINVAL;
>  	} else {
> -		/* Sealing not supported in hugetlbfs (MFD_HUGETLB) */
> -		if (flags & MFD_ALLOW_SEALING)
> -			return -EINVAL;
>  		/* Allow huge page size encoding in flags. */
>  		if (flags & ~(unsigned int)(MFD_ALL_FLAGS |
>  				(MFD_HUGE_MASK << MFD_HUGE_SHIFT)))
> @@ -3722,12 +3735,8 @@ SYSCALL_DEFINE2(memfd_create,
>  	file->f_flags |= O_RDWR | O_LARGEFILE;
>  
>  	if (flags & MFD_ALLOW_SEALING) {
> -		/*
> -		 * flags check at beginning of function ensures
> -		 * this is not a hugetlbfs (MFD_HUGETLB) file.
> -		 */
> -		info = SHMEM_I(file_inode(file));
> -		info->seals &= ~F_SEAL_SEAL;
> +		file_seals = memfd_get_seals(file);
> +		*file_seals &= ~F_SEAL_SEAL;
>  	}
>  
>  	fd_install(fd, file);
> 

Re: [PATCH 2/6] shmem: rename functions that are memfd-related

[Marc-André Lureau]

Hi

----- Original Message -----
> On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> > Those functions are called for memfd files, backed by shmem or
> > hugetlb (the next patches will handle hugetlb).
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  fs/fcntl.c               |  2 +-
> >  include/linux/shmem_fs.h |  4 ++--
> >  mm/shmem.c               | 10 +++++-----
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index 448a1119f0be..752c23743616 100644
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -417,7 +417,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned
> > long arg,
> >  		break;
> >  	case F_ADD_SEALS:
> >  	case F_GET_SEALS:
> > -		err = shmem_fcntl(filp, cmd, arg);
> > +		err = memfd_fcntl(filp, cmd, arg);
> >  		break;
> >  	case F_GET_RW_HINT:
> >  	case F_SET_RW_HINT:
> > diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> > index 557d0c3b6eca..0dac8c0f4aa4 100644
> > --- a/include/linux/shmem_fs.h
> > +++ b/include/linux/shmem_fs.h
> > @@ -109,11 +109,11 @@ extern void shmem_uncharge(struct inode *inode, long
> > pages);
> >  
> >  #ifdef CONFIG_TMPFS
> >  
> > -extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long
> > arg);
> > +extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long
> > arg);
> >  
> >  #else
> >  
> > -static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned
> > long a)
> > +static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned
> > long a)
> >  {
> >  	return -EINVAL;
> >  }
> 
> Do we want memfd_fcntl() to work for hugetlbfs if CONFIG_TMPFS is not
> defined?  I admit that having CONFIG_HUGETLBFS defined without CONFIG_TMPFS
> is unlikely, but I think possible.  Based on the above #ifdef/#else, I
> think hugetlbfs seals will not work if CONFIG_TMPFS is not defined.

Good point, memfd_create() will not exists either.

I think this is a separate concern, and preexisting from this patch series though.

Ack the function renaming part?

> --
> Mike Kravetz
> 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 37260c5e12fa..b7811979611f 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space
> > *mapping)
> >  		     F_SEAL_GROW | \
> >  		     F_SEAL_WRITE)
> >  
> > -static int shmem_add_seals(struct file *file, unsigned int seals)
> > +static int memfd_add_seals(struct file *file, unsigned int seals)
> >  {
> >  	struct inode *inode = file_inode(file);
> >  	struct shmem_inode_info *info = SHMEM_I(inode);
> > @@ -2792,7 +2792,7 @@ static int shmem_add_seals(struct file *file,
> > unsigned int seals)
> >  	return error;
> >  }
> >  
> > -static int shmem_get_seals(struct file *file)
> > +static int memfd_get_seals(struct file *file)
> >  {
> >  	if (file->f_op != &shmem_file_operations)
> >  		return -EINVAL;
> > @@ -2800,7 +2800,7 @@ static int shmem_get_seals(struct file *file)
> >  	return SHMEM_I(file_inode(file))->seals;
> >  }
> >  
> > -long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> > +long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> >  {
> >  	long error;
> >  
> > @@ -2810,10 +2810,10 @@ long shmem_fcntl(struct file *file, unsigned int
> > cmd, unsigned long arg)
> >  		if (arg > UINT_MAX)
> >  			return -EINVAL;
> >  
> > -		error = shmem_add_seals(file, arg);
> > +		error = memfd_add_seals(file, arg);
> >  		break;
> >  	case F_GET_SEALS:
> > -		error = shmem_get_seals(file);
> > +		error = memfd_get_seals(file);
> >  		break;
> >  	default:
> >  		error = -EINVAL;
> > 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH 5/6] shmem: add sealing support to hugetlb-backed memfd

[Marc-André Lureau]

Hi

----- Original Message -----
> On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> > Adapt add_seals()/get_seals() to work with hugetbfs-backed memory.
> > 
> > Teach memfd_create() to allow sealing operations on MFD_HUGETLB.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  mm/shmem.c | 51 ++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 30 insertions(+), 21 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index b7811979611f..b7c59d993c19 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2717,6 +2717,19 @@ static int shmem_wait_for_pins(struct address_space
> > *mapping)
> >  	return error;
> >  }
> >  
> > +static unsigned int *memfd_get_seals(struct file *file)
> 
> I would have named this something like 'memfd_file_seal_ptr', and not
> changed the name of memfd_get_seals below.  Just my preference, and it
> does not carry as much weight as Hugh who originally write this code.
> 

agreed and changed, thanks

> > +{
> > +	if (file->f_op == &shmem_file_operations)
> > +		return &SHMEM_I(file_inode(file))->seals;
> > +
> > +#ifdef CONFIG_HUGETLBFS
> > +	if (file->f_op == &hugetlbfs_file_operations)
> > +		return &HUGETLBFS_I(file_inode(file))->seals;
> > +#endif
> > +
> > +	return NULL;
> > +}
> > +
> 
> As mentioned in patch 2, I think this code will need to be restructured
> so that hugetlbfs file sealing will work even is CONFIG_TMPFS is not
> defined.  The above routine is behind #ifdef CONFIG_TMPFS.
> 
> In general the code looks fine, but this config issue needs to be addressed.

See discussion in patch 2

> --
> Mike Kravetz
> 
> >  #define F_ALL_SEALS (F_SEAL_SEAL | \
> >  		     F_SEAL_SHRINK | \
> >  		     F_SEAL_GROW | \
> > @@ -2725,7 +2738,7 @@ static int shmem_wait_for_pins(struct address_space
> > *mapping)
> >  static int memfd_add_seals(struct file *file, unsigned int seals)
> >  {
> >  	struct inode *inode = file_inode(file);
> > -	struct shmem_inode_info *info = SHMEM_I(inode);
> > +	unsigned int *file_seals;
> >  	int error;
> >  
> >  	/*
> > @@ -2758,8 +2771,6 @@ static int memfd_add_seals(struct file *file,
> > unsigned int seals)
> >  	 * other file types.
> >  	 */
> >  
> > -	if (file->f_op != &shmem_file_operations)
> > -		return -EINVAL;
> >  	if (!(file->f_mode & FMODE_WRITE))
> >  		return -EPERM;
> >  	if (seals & ~(unsigned int)F_ALL_SEALS)
> > @@ -2767,12 +2778,18 @@ static int memfd_add_seals(struct file *file,
> > unsigned int seals)
> >  
> >  	inode_lock(inode);
> >  
> > -	if (info->seals & F_SEAL_SEAL) {
> > +	file_seals = memfd_get_seals(file);
> > +	if (!file_seals) {
> > +		error = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	if (*file_seals & F_SEAL_SEAL) {
> >  		error = -EPERM;
> >  		goto unlock;
> >  	}
> >  
> > -	if ((seals & F_SEAL_WRITE) && !(info->seals & F_SEAL_WRITE)) {
> > +	if ((seals & F_SEAL_WRITE) && !(*file_seals & F_SEAL_WRITE)) {
> >  		error = mapping_deny_writable(file->f_mapping);
> >  		if (error)
> >  			goto unlock;
> > @@ -2784,7 +2801,7 @@ static int memfd_add_seals(struct file *file,
> > unsigned int seals)
> >  		}
> >  	}
> >  
> > -	info->seals |= seals;
> > +	*file_seals |= seals;
> >  	error = 0;
> >  
> >  unlock:
> > @@ -2792,12 +2809,11 @@ static int memfd_add_seals(struct file *file,
> > unsigned int seals)
> >  	return error;
> >  }
> >  
> > -static int memfd_get_seals(struct file *file)
> > +static int memfd_fcntl_get_seals(struct file *file)
> >  {
> > -	if (file->f_op != &shmem_file_operations)
> > -		return -EINVAL;
> > +	unsigned int *seals = memfd_get_seals(file);
> >  
> > -	return SHMEM_I(file_inode(file))->seals;
> > +	return seals ? *seals : -EINVAL;
> >  }
> >  
> >  long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> > @@ -2813,7 +2829,7 @@ long memfd_fcntl(struct file *file, unsigned int cmd,
> > unsigned long arg)
> >  		error = memfd_add_seals(file, arg);
> >  		break;
> >  	case F_GET_SEALS:
> > -		error = memfd_get_seals(file);
> > +		error = memfd_fcntl_get_seals(file);
> >  		break;
> >  	default:
> >  		error = -EINVAL;
> > @@ -3657,7 +3673,7 @@ SYSCALL_DEFINE2(memfd_create,
> >  		const char __user *, uname,
> >  		unsigned int, flags)
> >  {
> > -	struct shmem_inode_info *info;
> > +	unsigned int *file_seals;
> >  	struct file *file;
> >  	int fd, error;
> >  	char *name;
> > @@ -3667,9 +3683,6 @@ SYSCALL_DEFINE2(memfd_create,
> >  		if (flags & ~(unsigned int)MFD_ALL_FLAGS)
> >  			return -EINVAL;
> >  	} else {
> > -		/* Sealing not supported in hugetlbfs (MFD_HUGETLB) */
> > -		if (flags & MFD_ALLOW_SEALING)
> > -			return -EINVAL;
> >  		/* Allow huge page size encoding in flags. */
> >  		if (flags & ~(unsigned int)(MFD_ALL_FLAGS |
> >  				(MFD_HUGE_MASK << MFD_HUGE_SHIFT)))
> > @@ -3722,12 +3735,8 @@ SYSCALL_DEFINE2(memfd_create,
> >  	file->f_flags |= O_RDWR | O_LARGEFILE;
> >  
> >  	if (flags & MFD_ALLOW_SEALING) {
> > -		/*
> > -		 * flags check at beginning of function ensures
> > -		 * this is not a hugetlbfs (MFD_HUGETLB) file.
> > -		 */
> > -		info = SHMEM_I(file_inode(file));
> > -		info->seals &= ~F_SEAL_SEAL;
> > +		file_seals = memfd_get_seals(file);
> > +		*file_seals &= ~F_SEAL_SEAL;
> >  	}
> >  
> >  	fd_install(fd, file);
> > 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH 3/6] hugetlb: expose hugetlbfs_inode_info in header

[Marc-André Lureau]

Hi

----- Original Message -----
> On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> > The following patch is going to access hugetlbfs_inode_info field from
> > mm/shmem.c.
> 
> The code looks fine.  However, I would prefer something different for the
> commit message.  Perhaps something like:
> 
> hugetlbfs inode information will need to be accessed by code in mm/shmem.c
> for file sealing operations.  Move inode information definition from .c
> file to header for needed access.

Ok, Does the patch get your Reviewed-by tag with that change?

thanks

Re: [PATCH 2/6] shmem: rename functions that are memfd-related

[Mike Kravetz]

On 11/03/2017 09:02 AM, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
>>> Those functions are called for memfd files, backed by shmem or
>>> hugetlb (the next patches will handle hugetlb).
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  fs/fcntl.c               |  2 +-
>>>  include/linux/shmem_fs.h |  4 ++--
>>>  mm/shmem.c               | 10 +++++-----
>>>  3 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/fcntl.c b/fs/fcntl.c
>>> index 448a1119f0be..752c23743616 100644
>>> --- a/fs/fcntl.c
>>> +++ b/fs/fcntl.c
>>> @@ -417,7 +417,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned
>>> long arg,
>>>  		break;
>>>  	case F_ADD_SEALS:
>>>  	case F_GET_SEALS:
>>> -		err = shmem_fcntl(filp, cmd, arg);
>>> +		err = memfd_fcntl(filp, cmd, arg);
>>>  		break;
>>>  	case F_GET_RW_HINT:
>>>  	case F_SET_RW_HINT:
>>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>>> index 557d0c3b6eca..0dac8c0f4aa4 100644
>>> --- a/include/linux/shmem_fs.h
>>> +++ b/include/linux/shmem_fs.h
>>> @@ -109,11 +109,11 @@ extern void shmem_uncharge(struct inode *inode, long
>>> pages);
>>>  
>>>  #ifdef CONFIG_TMPFS
>>>  
>>> -extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long
>>> arg);
>>> +extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long
>>> arg);
>>>  
>>>  #else
>>>  
>>> -static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned
>>> long a)
>>> +static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned
>>> long a)
>>>  {
>>>  	return -EINVAL;
>>>  }
>>
>> Do we want memfd_fcntl() to work for hugetlbfs if CONFIG_TMPFS is not
>> defined?  I admit that having CONFIG_HUGETLBFS defined without CONFIG_TMPFS
>> is unlikely, but I think possible.  Based on the above #ifdef/#else, I
>> think hugetlbfs seals will not work if CONFIG_TMPFS is not defined.
> 
> Good point, memfd_create() will not exists either.
> 
> I think this is a separate concern, and preexisting from this patch series though.

Ah yes.  I should have addressed this when adding hugetlbfs memfd_create
support.

Of course, one 'simple' way to address this would be to make CONFIG_HUGETLBFS
depend on CONFIG_TMPFS.  Not sure what people think about this?

> Ack the function renaming part?

Yes, the remaining code looks fine to me.

-- 
Mike Kravetz

> 
>> --
>> Mike Kravetz
>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 37260c5e12fa..b7811979611f 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space
>>> *mapping)
>>>  		     F_SEAL_GROW | \
>>>  		     F_SEAL_WRITE)
>>>  
>>> -static int shmem_add_seals(struct file *file, unsigned int seals)
>>> +static int memfd_add_seals(struct file *file, unsigned int seals)
>>>  {
>>>  	struct inode *inode = file_inode(file);
>>>  	struct shmem_inode_info *info = SHMEM_I(inode);
>>> @@ -2792,7 +2792,7 @@ static int shmem_add_seals(struct file *file,
>>> unsigned int seals)
>>>  	return error;
>>>  }
>>>  
>>> -static int shmem_get_seals(struct file *file)
>>> +static int memfd_get_seals(struct file *file)
>>>  {
>>>  	if (file->f_op != &shmem_file_operations)
>>>  		return -EINVAL;
>>> @@ -2800,7 +2800,7 @@ static int shmem_get_seals(struct file *file)
>>>  	return SHMEM_I(file_inode(file))->seals;
>>>  }
>>>  
>>> -long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>>> +long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>>>  {
>>>  	long error;
>>>  
>>> @@ -2810,10 +2810,10 @@ long shmem_fcntl(struct file *file, unsigned int
>>> cmd, unsigned long arg)
>>>  		if (arg > UINT_MAX)
>>>  			return -EINVAL;
>>>  
>>> -		error = shmem_add_seals(file, arg);
>>> +		error = memfd_add_seals(file, arg);
>>>  		break;
>>>  	case F_GET_SEALS:
>>> -		error = shmem_get_seals(file);
>>> +		error = memfd_get_seals(file);
>>>  		break;
>>>  	default:
>>>  		error = -EINVAL;
>>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>

Re: [PATCH 3/6] hugetlb: expose hugetlbfs_inode_info in header

[Mike Kravetz]

On 11/03/2017 09:14 AM, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
>>> The following patch is going to access hugetlbfs_inode_info field from
>>> mm/shmem.c.
>>
>> The code looks fine.  However, I would prefer something different for the
>> commit message.  Perhaps something like:
>>
>> hugetlbfs inode information will need to be accessed by code in mm/shmem.c
>> for file sealing operations.  Move inode information definition from .c
>> file to header for needed access.
> 
> Ok, Does the patch get your Reviewed-by tag with that change?
> 
> thanks
> 

Yes, you can add
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

with an updated commit message.
-- 
Mike Kravetz

Re: [PATCH 2/6] shmem: rename functions that are memfd-related

[Marc-André Lureau]

Hi

----- Original Message -----
> On 11/03/2017 09:02 AM, Marc-André Lureau wrote:
> > Hi
> > 
> > ----- Original Message -----
> >> On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> >>> Those functions are called for memfd files, backed by shmem or
> >>> hugetlb (the next patches will handle hugetlb).
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>> ---
> >>>  fs/fcntl.c               |  2 +-
> >>>  include/linux/shmem_fs.h |  4 ++--
> >>>  mm/shmem.c               | 10 +++++-----
> >>>  3 files changed, 8 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/fs/fcntl.c b/fs/fcntl.c
> >>> index 448a1119f0be..752c23743616 100644
> >>> --- a/fs/fcntl.c
> >>> +++ b/fs/fcntl.c
> >>> @@ -417,7 +417,7 @@ static long do_fcntl(int fd, unsigned int cmd,
> >>> unsigned
> >>> long arg,
> >>>  		break;
> >>>  	case F_ADD_SEALS:
> >>>  	case F_GET_SEALS:
> >>> -		err = shmem_fcntl(filp, cmd, arg);
> >>> +		err = memfd_fcntl(filp, cmd, arg);
> >>>  		break;
> >>>  	case F_GET_RW_HINT:
> >>>  	case F_SET_RW_HINT:
> >>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> >>> index 557d0c3b6eca..0dac8c0f4aa4 100644
> >>> --- a/include/linux/shmem_fs.h
> >>> +++ b/include/linux/shmem_fs.h
> >>> @@ -109,11 +109,11 @@ extern void shmem_uncharge(struct inode *inode,
> >>> long
> >>> pages);
> >>>  
> >>>  #ifdef CONFIG_TMPFS
> >>>  
> >>> -extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned
> >>> long
> >>> arg);
> >>> +extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned
> >>> long
> >>> arg);
> >>>  
> >>>  #else
> >>>  
> >>> -static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned
> >>> long a)
> >>> +static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned
> >>> long a)
> >>>  {
> >>>  	return -EINVAL;
> >>>  }
> >>
> >> Do we want memfd_fcntl() to work for hugetlbfs if CONFIG_TMPFS is not
> >> defined?  I admit that having CONFIG_HUGETLBFS defined without
> >> CONFIG_TMPFS
> >> is unlikely, but I think possible.  Based on the above #ifdef/#else, I
> >> think hugetlbfs seals will not work if CONFIG_TMPFS is not defined.
> > 
> > Good point, memfd_create() will not exists either.
> > 
> > I think this is a separate concern, and preexisting from this patch series
> > though.
> 
> Ah yes.  I should have addressed this when adding hugetlbfs memfd_create
> support.
> 
> Of course, one 'simple' way to address this would be to make CONFIG_HUGETLBFS
> depend on CONFIG_TMPFS.  Not sure what people think about this?
> 

I can't say much about that. But compiling the hugetlb seal support while TPMFS/memfd is disabled should not break anything. You won't be able to add seals, that's it.

I suppose memfd could be splitted off TPMFS, and depend on either HUGETLBFS || TPMFS?

> > Ack the function renaming part?
> 
> Yes, the remaining code looks fine to me.

Should I add your Review-by: for this patch then?

> 
> --
> Mike Kravetz
> 
> > 
> >> --
> >> Mike Kravetz
> >>
> >>> diff --git a/mm/shmem.c b/mm/shmem.c
> >>> index 37260c5e12fa..b7811979611f 100644
> >>> --- a/mm/shmem.c
> >>> +++ b/mm/shmem.c
> >>> @@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space
> >>> *mapping)
> >>>  		     F_SEAL_GROW | \
> >>>  		     F_SEAL_WRITE)
> >>>  
> >>> -static int shmem_add_seals(struct file *file, unsigned int seals)
> >>> +static int memfd_add_seals(struct file *file, unsigned int seals)
> >>>  {
> >>>  	struct inode *inode = file_inode(file);
> >>>  	struct shmem_inode_info *info = SHMEM_I(inode);
> >>> @@ -2792,7 +2792,7 @@ static int shmem_add_seals(struct file *file,
> >>> unsigned int seals)
> >>>  	return error;
> >>>  }
> >>>  
> >>> -static int shmem_get_seals(struct file *file)
> >>> +static int memfd_get_seals(struct file *file)
> >>>  {
> >>>  	if (file->f_op != &shmem_file_operations)
> >>>  		return -EINVAL;
> >>> @@ -2800,7 +2800,7 @@ static int shmem_get_seals(struct file *file)
> >>>  	return SHMEM_I(file_inode(file))->seals;
> >>>  }
> >>>  
> >>> -long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> >>> +long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> >>>  {
> >>>  	long error;
> >>>  
> >>> @@ -2810,10 +2810,10 @@ long shmem_fcntl(struct file *file, unsigned int
> >>> cmd, unsigned long arg)
> >>>  		if (arg > UINT_MAX)
> >>>  			return -EINVAL;
> >>>  
> >>> -		error = shmem_add_seals(file, arg);
> >>> +		error = memfd_add_seals(file, arg);
> >>>  		break;
> >>>  	case F_GET_SEALS:
> >>> -		error = shmem_get_seals(file);
> >>> +		error = memfd_get_seals(file);
> >>>  		break;
> >>>  	default:
> >>>  		error = -EINVAL;
> >>>
> >>
> >> --
> >> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> >> the body to majordomo@kvack.org.  For more info on Linux MM,
> >> see: http://www.linux-mm.org/ .
> >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >>
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH 4/6] hugetlbfs: implement memfd sealing

[David Herrmann]

Hi

On Tue, Oct 31, 2017 at 7:40 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Implements memfd sealing, similar to shmem:
> - WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
>   memfd_add_seals(). write() doesn't exist for hugetlbfs.
> - SHRINK: added similar check as shmem_setattr()
> - GROW: added similar check as shmem_setattr() & shmem_fallocate()
>
> Except write() operation that doesn't exist with hugetlbfs, that
> should make sealing as close as it can be to shmem support.

SEAL, SHRINK, and GROW look fine to me.

Regarding WRITE you need to make sure there are no page references
left around. For instance, on shmem any process might trigger the
kernel to GUP mapped shmem pages for asynchronous IO, then unmap the
file and request F_SEAL_WRITE. In this case the seal must be rejected
*iff* the pages are still pinned. shmem does this by requiring the
page-refcounts to be 0. Preferably there would be some better
infrastructure that tells us whether someone operates on those pages,
but this does not exist right now. See shmem_wait_for_pins() for
details.

I have little knowledge on how hugetlbs integrate with the page-cache
and radix-tree, hence I'd prefer if someone can explicitly ACK that
shmem_wait_for_pins() is suitable for hugetlbfs.

Otherwise, this series looks good to me (minus the #ifdef mess..).

Thanks
David

Re: [PATCH 4/6] hugetlbfs: implement memfd sealing

[Mike Kravetz]

On 11/03/2017 10:03 AM, David Herrmann wrote:
> Hi
> 
> On Tue, Oct 31, 2017 at 7:40 PM, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>> Implements memfd sealing, similar to shmem:
>> - WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
>>   memfd_add_seals(). write() doesn't exist for hugetlbfs.
>> - SHRINK: added similar check as shmem_setattr()
>> - GROW: added similar check as shmem_setattr() & shmem_fallocate()
>>
>> Except write() operation that doesn't exist with hugetlbfs, that
>> should make sealing as close as it can be to shmem support.
> 
> SEAL, SHRINK, and GROW look fine to me.
> 
> Regarding WRITE

The commit message may not be clear.  However, hugetlbfs does not support
the write system call (or aio).  The only way to modify contents of a
hugetlbfs file is via mmap or hole punch/truncate.  So, we do not really
need to worry about those special (a)io cases for hugetlbfs.

-- 
Mike Kravetz

>                 you need to make sure there are no page references
> left around. For instance, on shmem any process might trigger the
> kernel to GUP mapped shmem pages for asynchronous IO, then unmap the
> file and request F_SEAL_WRITE. In this case the seal must be rejected
> *iff* the pages are still pinned. shmem does this by requiring the
> page-refcounts to be 0. Preferably there would be some better
> infrastructure that tells us whether someone operates on those pages,
> but this does not exist right now. See shmem_wait_for_pins() for
> details.
> 
> I have little knowledge on how hugetlbs integrate with the page-cache
> and radix-tree, hence I'd prefer if someone can explicitly ACK that
> shmem_wait_for_pins() is suitable for hugetlbfs.
> 
> Otherwise, this series looks good to me (minus the #ifdef mess..).
> 
> Thanks
> David
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=ilto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH 4/6] hugetlbfs: implement memfd sealing

[David Herrmann]

Hi

On Fri, Nov 3, 2017 at 6:12 PM, Mike Kravetz <mike.kravetz@oracle.com> wrote:
> On 11/03/2017 10:03 AM, David Herrmann wrote:
>> Hi
>>
>> On Tue, Oct 31, 2017 at 7:40 PM, Marc-André Lureau
>> <marcandre.lureau@redhat.com> wrote:
>>> Implements memfd sealing, similar to shmem:
>>> - WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
>>>   memfd_add_seals(). write() doesn't exist for hugetlbfs.
>>> - SHRINK: added similar check as shmem_setattr()
>>> - GROW: added similar check as shmem_setattr() & shmem_fallocate()
>>>
>>> Except write() operation that doesn't exist with hugetlbfs, that
>>> should make sealing as close as it can be to shmem support.
>>
>> SEAL, SHRINK, and GROW look fine to me.
>>
>> Regarding WRITE
>
> The commit message may not be clear.  However, hugetlbfs does not support
> the write system call (or aio).  The only way to modify contents of a
> hugetlbfs file is via mmap or hole punch/truncate.  So, we do not really
> need to worry about those special (a)io cases for hugetlbfs.

This is not about the write(2) syscall. Please consider this scenario
about shmem:

You create a memfd via memfd_create() and map it writable. You now
call another kernel syscall that takes as input _any mapped page
range_. You pass your mapped memfd-addresses to it. Those syscalls
tend to use get_user_pages() to pin arbitrary user-mapped pages, as
such this also affects shmem. In this case, those pages might stay
mapped even if you munmap() your memfd!

One example of this is using AIO-read() on any other file that
supports it, passing your mapped memfd as buffer to _read into_. The
operations supported on the memfd are irrelevant here.
The selftests contain a FUSE-based test for this, since FUSE allows
user-space to GUP pages for an arbitrary amount of time.

The original fix for this is:

    commit 05f65b5c70909ef686f865f0a85406d74d75f70f
    Author: David Herrmann <dh.herrmann@gmail.com>
    Date:   Fri Aug 8 14:25:36 2014 -0700

        shm: wait for pins to be released when sealing

Please have a look at this. Your patches use shmem_add_seals() almost
unchanged, and as such you call into shmem_wait_for_pins() on
hugetlbfs. I would really like to see an explicit ACK that this works
on hugetlbfs.

Thanks
David

Re: [PATCH 4/6] hugetlbfs: implement memfd sealing

[Mike Kravetz]

On 11/03/2017 10:41 AM, David Herrmann wrote:
> Hi
> 
> On Fri, Nov 3, 2017 at 6:12 PM, Mike Kravetz <mike.kravetz@oracle.com> wrote:
>> On 11/03/2017 10:03 AM, David Herrmann wrote:
>>> Hi
>>>
>>> On Tue, Oct 31, 2017 at 7:40 PM, Marc-André Lureau
>>> <marcandre.lureau@redhat.com> wrote:
>>>> Implements memfd sealing, similar to shmem:
>>>> - WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
>>>>   memfd_add_seals(). write() doesn't exist for hugetlbfs.
>>>> - SHRINK: added similar check as shmem_setattr()
>>>> - GROW: added similar check as shmem_setattr() & shmem_fallocate()
>>>>
>>>> Except write() operation that doesn't exist with hugetlbfs, that
>>>> should make sealing as close as it can be to shmem support.
>>>
>>> SEAL, SHRINK, and GROW look fine to me.
>>>
>>> Regarding WRITE
>>
>> The commit message may not be clear.  However, hugetlbfs does not support
>> the write system call (or aio).  The only way to modify contents of a
>> hugetlbfs file is via mmap or hole punch/truncate.  So, we do not really
>> need to worry about those special (a)io cases for hugetlbfs.
> 
> This is not about the write(2) syscall. Please consider this scenario
> about shmem:
> 
> You create a memfd via memfd_create() and map it writable. You now
> call another kernel syscall that takes as input _any mapped page
> range_. You pass your mapped memfd-addresses to it. Those syscalls
> tend to use get_user_pages() to pin arbitrary user-mapped pages, as
> such this also affects shmem. In this case, those pages might stay
> mapped even if you munmap() your memfd!
> 
> One example of this is using AIO-read() on any other file that
> supports it, passing your mapped memfd as buffer to _read into_. The
> operations supported on the memfd are irrelevant here.
> The selftests contain a FUSE-based test for this, since FUSE allows
> user-space to GUP pages for an arbitrary amount of time.
> 
> The original fix for this is:
> 
>     commit 05f65b5c70909ef686f865f0a85406d74d75f70f
>     Author: David Herrmann <dh.herrmann@gmail.com>
>     Date:   Fri Aug 8 14:25:36 2014 -0700
> 
>         shm: wait for pins to be released when sealing
> 
> Please have a look at this. Your patches use shmem_add_seals() almost
> unchanged, and as such you call into shmem_wait_for_pins() on
> hugetlbfs. I would really like to see an explicit ACK that this works
> on hugetlbfs.

Thanks for the explanation.  I missed that in your first reply.  I'll
look into this for hugetlbfs.

-- 
Mike Kravetz

> 
> Thanks
> David
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=ilto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH 2/6] shmem: rename functions that are memfd-related

[Mike Kravetz]

On 11/03/2017 09:36 AM, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 11/03/2017 09:02 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> ----- Original Message -----
>>>> On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
>>>>> Those functions are called for memfd files, backed by shmem or
>>>>> hugetlb (the next patches will handle hugetlb).
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>  fs/fcntl.c               |  2 +-
>>>>>  include/linux/shmem_fs.h |  4 ++--
>>>>>  mm/shmem.c               | 10 +++++-----
>>>>>  3 files changed, 8 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/fcntl.c b/fs/fcntl.c
>>>>> index 448a1119f0be..752c23743616 100644
>>>>> --- a/fs/fcntl.c
>>>>> +++ b/fs/fcntl.c
>>>>> @@ -417,7 +417,7 @@ static long do_fcntl(int fd, unsigned int cmd,
>>>>> unsigned
>>>>> long arg,
>>>>>  		break;
>>>>>  	case F_ADD_SEALS:
>>>>>  	case F_GET_SEALS:
>>>>> -		err = shmem_fcntl(filp, cmd, arg);
>>>>> +		err = memfd_fcntl(filp, cmd, arg);
>>>>>  		break;
>>>>>  	case F_GET_RW_HINT:
>>>>>  	case F_SET_RW_HINT:
>>>>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>>>>> index 557d0c3b6eca..0dac8c0f4aa4 100644
>>>>> --- a/include/linux/shmem_fs.h
>>>>> +++ b/include/linux/shmem_fs.h
>>>>> @@ -109,11 +109,11 @@ extern void shmem_uncharge(struct inode *inode,
>>>>> long
>>>>> pages);
>>>>>  
>>>>>  #ifdef CONFIG_TMPFS
>>>>>  
>>>>> -extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned
>>>>> long
>>>>> arg);
>>>>> +extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned
>>>>> long
>>>>> arg);
>>>>>  
>>>>>  #else
>>>>>  
>>>>> -static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned
>>>>> long a)
>>>>> +static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned
>>>>> long a)
>>>>>  {
>>>>>  	return -EINVAL;
>>>>>  }
>>>>
>>>> Do we want memfd_fcntl() to work for hugetlbfs if CONFIG_TMPFS is not
>>>> defined?  I admit that having CONFIG_HUGETLBFS defined without
>>>> CONFIG_TMPFS
>>>> is unlikely, but I think possible.  Based on the above #ifdef/#else, I
>>>> think hugetlbfs seals will not work if CONFIG_TMPFS is not defined.
>>>
>>> Good point, memfd_create() will not exists either.
>>>
>>> I think this is a separate concern, and preexisting from this patch series
>>> though.
>>
>> Ah yes.  I should have addressed this when adding hugetlbfs memfd_create
>> support.
>>
>> Of course, one 'simple' way to address this would be to make CONFIG_HUGETLBFS
>> depend on CONFIG_TMPFS.  Not sure what people think about this?
>>
> 
> I can't say much about that. But compiling the hugetlb seal support while TPMFS/memfd is disabled should not break anything. You won't be able to add seals, that's it.
> 

Correct.  But, if someone did create such a config AND wanted hugetlbfs
seal support they would be out of luck.  I really can't imagine systems
where tmpfs would be disabled and hugetlbfs would be enabled and users
would want hugetlbfs file sealing.  That is why I threw out the possibility
of making hugetlbfs depend on tmpfs.

> I suppose memfd could be splitted off TPMFS, and depend on either HUGETLBFS || TPMFS?

Yes, that would be the ideal solution.  I just hate to go through the code
churn for a config combination that may never be used.  However, this really
would be the right thing to do.

> 
>>> Ack the function renaming part?
>>
>> Yes, the remaining code looks fine to me.
> 
> Should I add your Review-by: for this patch then?

Yes

-- 
Mike Kravetz

> 
>>
>> --
>> Mike Kravetz
>>
>>>
>>>> --
>>>> Mike Kravetz
>>>>
>>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>>> index 37260c5e12fa..b7811979611f 100644
>>>>> --- a/mm/shmem.c
>>>>> +++ b/mm/shmem.c
>>>>> @@ -2722,7 +2722,7 @@ static int shmem_wait_for_pins(struct address_space
>>>>> *mapping)
>>>>>  		     F_SEAL_GROW | \
>>>>>  		     F_SEAL_WRITE)
>>>>>  
>>>>> -static int shmem_add_seals(struct file *file, unsigned int seals)
>>>>> +static int memfd_add_seals(struct file *file, unsigned int seals)
>>>>>  {
>>>>>  	struct inode *inode = file_inode(file);
>>>>>  	struct shmem_inode_info *info = SHMEM_I(inode);
>>>>> @@ -2792,7 +2792,7 @@ static int shmem_add_seals(struct file *file,
>>>>> unsigned int seals)
>>>>>  	return error;
>>>>>  }
>>>>>  
>>>>> -static int shmem_get_seals(struct file *file)
>>>>> +static int memfd_get_seals(struct file *file)
>>>>>  {
>>>>>  	if (file->f_op != &shmem_file_operations)
>>>>>  		return -EINVAL;
>>>>> @@ -2800,7 +2800,7 @@ static int shmem_get_seals(struct file *file)
>>>>>  	return SHMEM_I(file_inode(file))->seals;
>>>>>  }
>>>>>  
>>>>> -long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>>>>> +long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>>>>>  {
>>>>>  	long error;
>>>>>  
>>>>> @@ -2810,10 +2810,10 @@ long shmem_fcntl(struct file *file, unsigned int
>>>>> cmd, unsigned long arg)
>>>>>  		if (arg > UINT_MAX)
>>>>>  			return -EINVAL;
>>>>>  
>>>>> -		error = shmem_add_seals(file, arg);
>>>>> +		error = memfd_add_seals(file, arg);
>>>>>  		break;
>>>>>  	case F_GET_SEALS:
>>>>> -		error = shmem_get_seals(file);
>>>>> +		error = memfd_get_seals(file);
>>>>>  		break;
>>>>>  	default:
>>>>>  		error = -EINVAL;
>>>>>
>>>>
>>>> --
>>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>>> the body to majordomo@kvack.org.  For more info on Linux MM,
>>>> see: http://www.linux-mm.org/ .
>>>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>

Re: [PATCH 4/6] hugetlbfs: implement memfd sealing

[Mike Kravetz]

On 11/03/2017 10:56 AM, Mike Kravetz wrote:
> On 11/03/2017 10:41 AM, David Herrmann wrote:
>> Hi
>>
>> On Fri, Nov 3, 2017 at 6:12 PM, Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>> On 11/03/2017 10:03 AM, David Herrmann wrote:
>>>> Hi
>>>>
>>>> On Tue, Oct 31, 2017 at 7:40 PM, Marc-André Lureau
>>>> <marcandre.lureau@redhat.com> wrote:
>>>>> Implements memfd sealing, similar to shmem:
>>>>> - WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
>>>>>   memfd_add_seals(). write() doesn't exist for hugetlbfs.
>>>>> - SHRINK: added similar check as shmem_setattr()
>>>>> - GROW: added similar check as shmem_setattr() & shmem_fallocate()
>>>>>
>>>>> Except write() operation that doesn't exist with hugetlbfs, that
>>>>> should make sealing as close as it can be to shmem support.
>>>>
>>>> SEAL, SHRINK, and GROW look fine to me.
>>>>
>>>> Regarding WRITE
>>>
>>> The commit message may not be clear.  However, hugetlbfs does not support
>>> the write system call (or aio).  The only way to modify contents of a
>>> hugetlbfs file is via mmap or hole punch/truncate.  So, we do not really
>>> need to worry about those special (a)io cases for hugetlbfs.
>>
>> This is not about the write(2) syscall. Please consider this scenario
>> about shmem:
>>
>> You create a memfd via memfd_create() and map it writable. You now
>> call another kernel syscall that takes as input _any mapped page
>> range_. You pass your mapped memfd-addresses to it. Those syscalls
>> tend to use get_user_pages() to pin arbitrary user-mapped pages, as
>> such this also affects shmem. In this case, those pages might stay
>> mapped even if you munmap() your memfd!
>>
>> One example of this is using AIO-read() on any other file that
>> supports it, passing your mapped memfd as buffer to _read into_. The
>> operations supported on the memfd are irrelevant here.
>> The selftests contain a FUSE-based test for this, since FUSE allows
>> user-space to GUP pages for an arbitrary amount of time.
>>
>> The original fix for this is:
>>
>>     commit 05f65b5c70909ef686f865f0a85406d74d75f70f
>>     Author: David Herrmann <dh.herrmann@gmail.com>
>>     Date:   Fri Aug 8 14:25:36 2014 -0700
>>
>>         shm: wait for pins to be released when sealing
>>
>> Please have a look at this. Your patches use shmem_add_seals() almost
>> unchanged, and as such you call into shmem_wait_for_pins() on
>> hugetlbfs. I would really like to see an explicit ACK that this works
>> on hugetlbfs.
> 
> Thanks for the explanation.  I missed that in your first reply.  I'll
> look into this for hugetlbfs.

I reviewed the routines in the above commit and did not see anything that
would prevent them from working properly with hugetlbfs.  I modified the
fuse test to use hugetlbfs based mapping.  I also instrumented the above
routines and verified that tags were set/checked/cleared as designed for
hugetlb pages.  So, that is an ACK on working with hugetlbfs.

This does bring up the point that the fuse seals test should also be
modified to work with hugetlbfs as part of this series.

-- 
Mike Kravetz

Re: [PATCH 6/6] memfd-tests: test hugetlbfs sealing

[Mike Kravetz]

On 10/31/2017 11:40 AM, Marc-André Lureau wrote:
> Remove most of the special-casing of hugetlbfs now that sealing
> is supported.

The changes below look fine.  Just a couple issues.

While discussing patch 4 with David, I realized that we should modify/expand
the fuse seals test to also verify proper functionality with hugetlbfs.  I
think this is a requirement.

The output of run_tests.sh looks something like:
opening: ./mnt/memfd
fuse: DONE
memfd: CREATE
memfd: BASIC
memfd: SEAL-WRITE
memfd: SEAL-SHRINK
memfd: SEAL-GROW
memfd: SEAL-RESIZE
memfd: SHARE-DUP 
memfd: SHARE-MMAP 
memfd: SHARE-OPEN 
memfd: SHARE-FORK 
memfd: SHARE-DUP (shared file-table)
memfd: SHARE-MMAP (shared file-table)
memfd: SHARE-OPEN (shared file-table)
memfd: SHARE-FORK (shared file-table)
memfd: DONE
memfd: CREATE
memfd: BASIC
memfd: SEAL-WRITE
memfd: SEAL-SHRINK
memfd: SEAL-GROW
memfd: SEAL-RESIZE
memfd: SHARE-DUP 
memfd: SHARE-MMAP 
memfd: SHARE-OPEN 
memfd: SHARE-FORK 
memfd: SHARE-DUP (shared file-table)
memfd: SHARE-MMAP (shared file-table)
memfd: SHARE-OPEN (shared file-table)
memfd: SHARE-FORK (shared file-table)
memfd: DONE

It might be nice to distinguish testing of tmpfs and hugetlbfs.  The
string '#define MEMFD_STR "memfd:"' is prepended to the output lines.
Perhaps we could do something like:
#define MEMFD_STR       "memfd:"
#define MEMFD_HUGE_STR	"memfd-hugetlb"
char *memfd_str;

and then in main,
if (hugetlbfs_test)
	memfd_str = MEMFD_HUGE_STR;
else
	memfd_str = MEMFD_STR;

then prepend output strings with memfd_str.

This is just a suggestion and optional.

-- 
Mike Kravetz

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tools/testing/selftests/memfd/memfd_test.c | 150 +++--------------------------
>  1 file changed, 15 insertions(+), 135 deletions(-)
> 
> diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
> index f94c6d1fb46f..f5028f800107 100644
> --- a/tools/testing/selftests/memfd/memfd_test.c
> +++ b/tools/testing/selftests/memfd/memfd_test.c
> @@ -512,6 +512,10 @@ static void mfd_assert_grow_write(int fd)
>  	static char *buf;
>  	ssize_t l;
>  
> +	/* hugetlbfs does not support write */
> +	if (hugetlbfs_test)
> +		return;
> +
>  	buf = malloc(mfd_def_size * 8);
>  	if (!buf) {
>  		printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
> @@ -532,6 +536,10 @@ static void mfd_fail_grow_write(int fd)
>  	static char *buf;
>  	ssize_t l;
>  
> +	/* hugetlbfs does not support write */
> +	if (hugetlbfs_test)
> +		return;
> +
>  	buf = malloc(mfd_def_size * 8);
>  	if (!buf) {
>  		printf("malloc(%d) failed: %m\n", mfd_def_size * 8);
> @@ -626,18 +634,13 @@ static void test_create(void)
>  	fd = mfd_assert_new("", 0, MFD_CLOEXEC);
>  	close(fd);
>  
> -	if (!hugetlbfs_test) {
> -		/* verify MFD_ALLOW_SEALING is allowed */
> -		fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
> -		close(fd);
> -
> -		/* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
> -		fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> -		close(fd);
> -	} else {
> -		/* sealing is not supported on hugetlbfs */
> -		mfd_fail_new("", MFD_ALLOW_SEALING);
> -	}
> +	/* verify MFD_ALLOW_SEALING is allowed */
> +	fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING);
> +	close(fd);
> +
> +	/* verify MFD_ALLOW_SEALING | MFD_CLOEXEC is allowed */
> +	fd = mfd_assert_new("", 0, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +	close(fd);
>  }
>  
>  /*
> @@ -648,10 +651,6 @@ static void test_basic(void)
>  {
>  	int fd;
>  
> -	/* hugetlbfs does not contain sealing support */
> -	if (hugetlbfs_test)
> -		return;
> -
>  	printf("%s BASIC\n", MEMFD_STR);
>  
>  	fd = mfd_assert_new("kern_memfd_basic",
> @@ -696,28 +695,6 @@ static void test_basic(void)
>  	close(fd);
>  }
>  
> -/*
> - * hugetlbfs doesn't support seals or write, so just verify grow and shrink
> - * on a hugetlbfs file created via memfd_create.
> - */
> -static void test_hugetlbfs_grow_shrink(void)
> -{
> -	int fd;
> -
> -	printf("%s HUGETLBFS-GROW-SHRINK\n", MEMFD_STR);
> -
> -	fd = mfd_assert_new("kern_memfd_seal_write",
> -			    mfd_def_size,
> -			    MFD_CLOEXEC);
> -
> -	mfd_assert_read(fd);
> -	mfd_assert_write(fd);
> -	mfd_assert_shrink(fd);
> -	mfd_assert_grow(fd);
> -
> -	close(fd);
> -}
> -
>  /*
>   * Test SEAL_WRITE
>   * Test whether SEAL_WRITE actually prevents modifications.
> @@ -726,13 +703,6 @@ static void test_seal_write(void)
>  {
>  	int fd;
>  
> -	/*
> -	 * hugetlbfs does not contain sealing or write support.  Just test
> -	 * basic grow and shrink via test_hugetlbfs_grow_shrink.
> -	 */
> -	if (hugetlbfs_test)
> -		return test_hugetlbfs_grow_shrink();
> -
>  	printf("%s SEAL-WRITE\n", MEMFD_STR);
>  
>  	fd = mfd_assert_new("kern_memfd_seal_write",
> @@ -759,10 +729,6 @@ static void test_seal_shrink(void)
>  {
>  	int fd;
>  
> -	/* hugetlbfs does not contain sealing support */
> -	if (hugetlbfs_test)
> -		return;
> -
>  	printf("%s SEAL-SHRINK\n", MEMFD_STR);
>  
>  	fd = mfd_assert_new("kern_memfd_seal_shrink",
> @@ -789,10 +755,6 @@ static void test_seal_grow(void)
>  {
>  	int fd;
>  
> -	/* hugetlbfs does not contain sealing support */
> -	if (hugetlbfs_test)
> -		return;
> -
>  	printf("%s SEAL-GROW\n", MEMFD_STR);
>  
>  	fd = mfd_assert_new("kern_memfd_seal_grow",
> @@ -819,10 +781,6 @@ static void test_seal_resize(void)
>  {
>  	int fd;
>  
> -	/* hugetlbfs does not contain sealing support */
> -	if (hugetlbfs_test)
> -		return;
> -
>  	printf("%s SEAL-RESIZE\n", MEMFD_STR);
>  
>  	fd = mfd_assert_new("kern_memfd_seal_resize",
> @@ -841,32 +799,6 @@ static void test_seal_resize(void)
>  	close(fd);
>  }
>  
> -/*
> - * hugetlbfs does not support seals.  Basic test to dup the memfd created
> - * fd and perform some basic operations on it.
> - */
> -static void hugetlbfs_dup(char *b_suffix)
> -{
> -	int fd, fd2;
> -
> -	printf("%s HUGETLBFS-DUP %s\n", MEMFD_STR, b_suffix);
> -
> -	fd = mfd_assert_new("kern_memfd_share_dup",
> -			    mfd_def_size,
> -			    MFD_CLOEXEC);
> -
> -	fd2 = mfd_assert_dup(fd);
> -
> -	mfd_assert_read(fd);
> -	mfd_assert_write(fd);
> -
> -	mfd_assert_shrink(fd2);
> -	mfd_assert_grow(fd2);
> -
> -	close(fd2);
> -	close(fd);
> -}
> -
>  /*
>   * Test sharing via dup()
>   * Test that seals are shared between dupped FDs and they're all equal.
> @@ -875,15 +807,6 @@ static void test_share_dup(char *banner, char *b_suffix)
>  {
>  	int fd, fd2;
>  
> -	/*
> -	 * hugetlbfs does not contain sealing support.  Perform some
> -	 * basic testing on dup'ed fd instead via hugetlbfs_dup.
> -	 */
> -	if (hugetlbfs_test) {
> -		hugetlbfs_dup(b_suffix);
> -		return;
> -	}
> -
>  	printf("%s %s %s\n", MEMFD_STR, banner, b_suffix);
>  
>  	fd = mfd_assert_new("kern_memfd_share_dup",
> @@ -926,10 +849,6 @@ static void test_share_mmap(char *banner, char *b_suffix)
>  	int fd;
>  	void *p;
>  
> -	/* hugetlbfs does not contain sealing support */
> -	if (hugetlbfs_test)
> -		return;
> -
>  	printf("%s %s %s\n", MEMFD_STR,  banner, b_suffix);
>  
>  	fd = mfd_assert_new("kern_memfd_share_mmap",
> @@ -954,32 +873,6 @@ static void test_share_mmap(char *banner, char *b_suffix)
>  	close(fd);
>  }
>  
> -/*
> - * Basic test to make sure we can open the hugetlbfs fd via /proc and
> - * perform some simple operations on it.
> - */
> -static void hugetlbfs_proc_open(char *b_suffix)
> -{
> -	int fd, fd2;
> -
> -	printf("%s HUGETLBFS-PROC-OPEN %s\n", MEMFD_STR, b_suffix);
> -
> -	fd = mfd_assert_new("kern_memfd_share_open",
> -			    mfd_def_size,
> -			    MFD_CLOEXEC);
> -
> -	fd2 = mfd_assert_open(fd, O_RDWR, 0);
> -
> -	mfd_assert_read(fd);
> -	mfd_assert_write(fd);
> -
> -	mfd_assert_shrink(fd2);
> -	mfd_assert_grow(fd2);
> -
> -	close(fd2);
> -	close(fd);
> -}
> -
>  /*
>   * Test sealing with open(/proc/self/fd/%d)
>   * Via /proc we can get access to a separate file-context for the same memfd.
> @@ -990,15 +883,6 @@ static void test_share_open(char *banner, char *b_suffix)
>  {
>  	int fd, fd2;
>  
> -	/*
> -	 * hugetlbfs does not contain sealing support.  So test basic
> -	 * functionality of using /proc fd via hugetlbfs_proc_open
> -	 */
> -	if (hugetlbfs_test) {
> -		hugetlbfs_proc_open(b_suffix);
> -		return;
> -	}
> -
>  	printf("%s %s %s\n", MEMFD_STR, banner, b_suffix);
>  
>  	fd = mfd_assert_new("kern_memfd_share_open",
> @@ -1042,10 +926,6 @@ static void test_share_fork(char *banner, char *b_suffix)
>  	int fd;
>  	pid_t pid;
>  
> -	/* hugetlbfs does not contain sealing support */
> -	if (hugetlbfs_test)
> -		return;
> -
>  	printf("%s %s %s\n", MEMFD_STR, banner, b_suffix);
>  
>  	fd = mfd_assert_new("kern_memfd_share_fork",
> 

Re: [PATCH 4/6] hugetlbfs: implement memfd sealing

[David Herrmann]

Hi

On Sat, Nov 4, 2017 at 12:31 AM, Mike Kravetz <mike.kravetz@oracle.com> wrote:
> On 11/03/2017 10:56 AM, Mike Kravetz wrote:
>> On 11/03/2017 10:41 AM, David Herrmann wrote:
>>> Hi
>>>
>>> On Fri, Nov 3, 2017 at 6:12 PM, Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>>> On 11/03/2017 10:03 AM, David Herrmann wrote:
>>>>> Hi
>>>>>
>>>>> On Tue, Oct 31, 2017 at 7:40 PM, Marc-André Lureau
>>>>> <marcandre.lureau@redhat.com> wrote:
>>>>>> Implements memfd sealing, similar to shmem:
>>>>>> - WRITE: deny fallocate(PUNCH_HOLE). mmap() write is denied in
>>>>>>   memfd_add_seals(). write() doesn't exist for hugetlbfs.
>>>>>> - SHRINK: added similar check as shmem_setattr()
>>>>>> - GROW: added similar check as shmem_setattr() & shmem_fallocate()
>>>>>>
>>>>>> Except write() operation that doesn't exist with hugetlbfs, that
>>>>>> should make sealing as close as it can be to shmem support.
>>>>>
>>>>> SEAL, SHRINK, and GROW look fine to me.
>>>>>
>>>>> Regarding WRITE
>>>>
>>>> The commit message may not be clear.  However, hugetlbfs does not support
>>>> the write system call (or aio).  The only way to modify contents of a
>>>> hugetlbfs file is via mmap or hole punch/truncate.  So, we do not really
>>>> need to worry about those special (a)io cases for hugetlbfs.
>>>
>>> This is not about the write(2) syscall. Please consider this scenario
>>> about shmem:
>>>
>>> You create a memfd via memfd_create() and map it writable. You now
>>> call another kernel syscall that takes as input _any mapped page
>>> range_. You pass your mapped memfd-addresses to it. Those syscalls
>>> tend to use get_user_pages() to pin arbitrary user-mapped pages, as
>>> such this also affects shmem. In this case, those pages might stay
>>> mapped even if you munmap() your memfd!
>>>
>>> One example of this is using AIO-read() on any other file that
>>> supports it, passing your mapped memfd as buffer to _read into_. The
>>> operations supported on the memfd are irrelevant here.
>>> The selftests contain a FUSE-based test for this, since FUSE allows
>>> user-space to GUP pages for an arbitrary amount of time.
>>>
>>> The original fix for this is:
>>>
>>>     commit 05f65b5c70909ef686f865f0a85406d74d75f70f
>>>     Author: David Herrmann <dh.herrmann@gmail.com>
>>>     Date:   Fri Aug 8 14:25:36 2014 -0700
>>>
>>>         shm: wait for pins to be released when sealing
>>>
>>> Please have a look at this. Your patches use shmem_add_seals() almost
>>> unchanged, and as such you call into shmem_wait_for_pins() on
>>> hugetlbfs. I would really like to see an explicit ACK that this works
>>> on hugetlbfs.
>>
>> Thanks for the explanation.  I missed that in your first reply.  I'll
>> look into this for hugetlbfs.
>
> I reviewed the routines in the above commit and did not see anything that
> would prevent them from working properly with hugetlbfs.  I modified the
> fuse test to use hugetlbfs based mapping.  I also instrumented the above
> routines and verified that tags were set/checked/cleared as designed for
> hugetlb pages.  So, that is an ACK on working with hugetlbfs.
>
> This does bring up the point that the fuse seals test should also be
> modified to work with hugetlbfs as part of this series.

Perfect! Looks all good to me then!

Thanks
David