| Message ID | 1507996677.21357.1.camel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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: Nah - my earlier comment was simply misinformed because I didn't read the whole patch and the _validate name mislead me. So I think the current calling conventions are ok, I'd just like a better name (mmap_flags maybe?) and avoid the need the file system also has to implement ->mmap.
On Mon 16-10-17 00:45:04, Christoph Hellwig wrote: > > 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: > > Nah - my earlier comment was simply misinformed because I didn't > read the whole patch and the _validate name mislead me. > > So I think the current calling conventions are ok, I'd just like a > better name (mmap_flags maybe?) and avoid the need the file system > also has to implement ->mmap. OK, I can do that. But I had just realized that if MAP_DIRECT isn't going to end up using mmap(2) interface but something else (and I'm not sure where discussions on this matter ended), we don't need flags argument for ->mmap at all. MAP_SYNC uses a VMA flag anyway and thus it is fine with the current ->mmap interface. We still need some opt-in mechanism for MAP_SHARED_VALIDATE though (probably supported mmap flags as Dan had in one version of his patch). Thoughts on which way to go for now? Honza
On Tue, Oct 17, 2017 at 4:50 AM, Jan Kara <jack@suse.cz> wrote: > On Mon 16-10-17 00:45:04, Christoph Hellwig wrote: >> > 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: >> >> Nah - my earlier comment was simply misinformed because I didn't >> read the whole patch and the _validate name mislead me. >> >> So I think the current calling conventions are ok, I'd just like a >> better name (mmap_flags maybe?) and avoid the need the file system >> also has to implement ->mmap. > > OK, I can do that. But I had just realized that if MAP_DIRECT isn't going > to end up using mmap(2) interface but something else (and I'm not sure > where discussions on this matter ended), we don't need flags argument for > ->mmap at all. MAP_SYNC uses a VMA flag anyway and thus it is fine with the > current ->mmap interface. We still need some opt-in mechanism for > MAP_SHARED_VALIDATE though (probably supported mmap flags as Dan had in one > version of his patch). Thoughts on which way to go for now? The "supported mmap flags" approach also solves the problem you raised about MAP_SYNC being silently accepted by an ->mmap() handler that does not know about the new flag. I.e. leading userpace to potentially assume an invalid data consistency model. I'll revive that approach now that the MAP_DIRECT problem is going to be solved via a different interface.
On Tue, Oct 17, 2017 at 01:50:47PM +0200, Jan Kara wrote: > OK, I can do that. But I had just realized that if MAP_DIRECT isn't going > to end up using mmap(2) interface but something else (and I'm not sure > where discussions on this matter ended), we don't need flags argument for > ->mmap at all. MAP_SYNC uses a VMA flag anyway and thus it is fine with the > current ->mmap interface. We still need some opt-in mechanism for > MAP_SHARED_VALIDATE though (probably supported mmap flags as Dan had in one > version of his patch). Thoughts on which way to go for now? Yes, I'd much prefer the mmap_flags in file_operations. The other option would be a new FMODE_* flag which is what Al did for various other optional features, but I generally thing that is a confusing interface.
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;