Re: [PATCH 01/19] mm: introduce MAP_SHARED_VALIDATE, a mechanism to safely define new mmap flags

["Williams, Dan J" <dan.j.williams@intel.com>]

On Fri, 2017-10-13 at 00:12 -0700, Christoph Hellwig wrote:
> So did we settle on the new mmap_validate vs adding a new argument
> to ->mmap for real now?  I have to say I'd much prefer passing an
> additional argument instead, but if higher powers rule that out
> this version is ok.

Even if we decide to add a parameter to ->mmap() I think that should be
done after we merge this version. Otherwise there's no way to stage
these changes in advance of the merge window since we need to run the
"add parameter" coccinelle script near or after -rc1 when there's no
risk of new ->mmap() users being added.

> 
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 13dab191a23e..5aee97d64cae 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1701,6 +1701,8 @@ struct file_operations {
> >  	long (*unlocked_ioctl) (struct file *, unsigned int,
> > unsigned long);
> >  	long (*compat_ioctl) (struct file *, unsigned int,
> > unsigned long);
> >  	int (*mmap) (struct file *, struct vm_area_struct *);
> > +	int (*mmap_validate) (struct file *, struct vm_area_struct
> > *,
> > +			unsigned long);
> 
> Can we make this return a bool for ok vs not ok?  That way we only
> need to have the error code discussion in one place instead of every
> file system.

How about the following incremental update? It allows ->mmap_validate()
to be used as a full replacement for ->mmap() and it limits the error
code freedom to a centralized mmap_status_errno() routine:

---

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5aee97d64cae..e07fcac17ba7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1685,6 +1685,13 @@ struct block_device_operations;
 #define NOMMU_VMFLAGS \
 	(NOMMU_MAP_READ | NOMMU_MAP_WRITE | NOMMU_MAP_EXEC)
 
+enum mmap_status {
+	MMAP_SUCCESS,
+	MMAP_UNSUPPORTED_FLAGS,
+	MMAP_INVALID_FILE,
+	MMAP_RESOURCE_FAILURE,
+};
+typedef enum mmap_status mmap_status_t;
 
 struct iov_iter;
 
@@ -1701,7 +1708,7 @@ struct file_operations {
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
-	int (*mmap_validate) (struct file *, struct vm_area_struct *,
+	mmap_status_t (*mmap_validate) (struct file *, struct vm_area_struct *,
 			unsigned long);
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *, fl_owner_t id);
diff --git a/mm/mmap.c b/mm/mmap.c
index 2649c00581a0..c1b6a8c25ce7 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1432,7 +1432,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
 				vm_flags &= ~VM_MAYEXEC;
 			}
 
-			if (!file->f_op->mmap)
+			if (!file->f_op->mmap && !file->f_op->mmap_validate)
 				return -ENODEV;
 			if (vm_flags & (VM_GROWSDOWN|VM_GROWSUP))
 				return -EINVAL;
@@ -1612,6 +1612,27 @@ static inline int accountable_mapping(struct file *file, vm_flags_t vm_flags)
 	return (vm_flags & (VM_NORESERVE | VM_SHARED | VM_WRITE)) == VM_WRITE;
 }
 
+static int mmap_status_errno(mmap_status_t stat)
+{
+	static const int rc[] = {
+		[MMAP_SUCCESS] = 0,
+		[MMAP_UNSUPPORTED_FLAGS] = -EOPNOTSUPP,
+		[MMAP_INVALID_FILE] = -ENOTTY,
+		[MMAP_RESOURCE_FAILURE] = -ENOMEM,
+	};
+
+	switch (stat) {
+	case MMAP_SUCCESS:
+	case MMAP_UNSUPPORTED_FLAGS:
+	case MMAP_INVALID_FILE:
+	case MMAP_RESOURCE_FAILURE:
+		return rc[stat];
+	default:
+		/* unknown mmap_status */
+		return rc[MMAP_UNSUPPORTED_FLAGS];
+	}
+}
+
 unsigned long mmap_region(struct file *file, unsigned long addr,
 		unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
 		struct list_head *uf, unsigned long map_flags)
@@ -1619,6 +1640,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma, *prev;
 	int error;
+	mmap_status_t status;
 	struct rb_node **rb_link, *rb_parent;
 	unsigned long charged = 0;
 
@@ -1697,11 +1719,19 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 		 * vma_link() below can deny write-access if VM_DENYWRITE is set
 		 * and map writably if VM_SHARED is set. This usually means the
 		 * new file must not have been exposed to user-space, yet.
+		 *
+		 * We require ->mmap_validate in the MAP_SHARED_VALIDATE
+		 * case, prefer ->mmap_validate over ->mmap, and
+		 * otherwise fallback to legacy ->mmap.
 		 */
 		vma->vm_file = get_file(file);
-		if ((map_flags & MAP_TYPE) == MAP_SHARED_VALIDATE)
-			error = file->f_op->mmap_validate(file, vma, map_flags);
-		else
+		if ((map_flags & MAP_TYPE) == MAP_SHARED_VALIDATE) {
+			status = file->f_op->mmap_validate(file, vma, map_flags);
+			error = mmap_status_errno(status);
+		} else if (file->f_op->mmap_validate) {
+			status = file->f_op->mmap_validate(file, vma, map_flags);
+			error = mmap_status_errno(status);
+		} else
 			error = call_mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;