Fix get_unmapped_area and fsync for hugetlb shm segments
PATCH 1/1 Fix get_unmapped_area and fsync for hugetlb shm segments
This patch provides the following hugetlb-related fixes to the recent stacked shm files changes:
- Update is_file_hugepages() so it will reconize hugetlb shm segments.
- get_unmapped_area must be called with the nested file struct to handle the sfd->file->f_ops->get_unmapped_area == NULL case.
- The fsync f_op must be wrapped since it is specified in the hugetlbfs f_ops.
This is based on proposed fixes from Eric Biederman that were debugged and tested by me. Without it, attempting to use hugetlb shared memory segments on powerpc (and likely ia64) will kill your box.
Please Apply.
---
include/linux/hugetlb.h | 8 +++++++- include/linux/shm.h | 5 +++++ ipc/shm.c | 32 ++++++++++++++++++++++++++------ 3 files changed, 38 insertions(+), 7 deletions(-)
include/linux/hugetlb.h +7 -1
@@ -4,6 +4,7 @@#ifdef CONFIG_HUGETLB_PAGE #include <linux/mempolicy.h> +#include <linux/shm.h> #include <asm/tlbflush.h> struct ctl_table ;
@@ -168,7 +169,12 @@ void hugetlb_put_quota(struct address_space *mapping);static inline int is_file_hugepages (struct file *file) { -return file->f_op == &hugetlbfs_file_operations; +if (file->f_op == &hugetlbfs_file_operations) +return 1 ; +if (is_file_shm_hugepages (file)) +return 1 ; + +return 0 ; }static inline void set_file_hugepages (struct file *file)
include/linux/shm.h +5 -0
@@ -96,12 +96,17 @@ struct shmid_kernel /* private to the kernel */#ifdef CONFIG_SYSVIPC long do_shmat (int shmid,char __user *shmaddr,int shmflg,unsigned long *addr); +extern int is_file_shm_hugepages (struct file *file);#else static inline long do_shmat (int shmid,char __user *shmaddr,int shmflg,unsigned long *addr) {return -ENOSYS ; } +static inline int is_file_shm_hugepages (struct file *file) +{ +return 0 ; +}#endif #endif /* __KERNEL__ */
ipc/shm.c +26 -6
@@ -285,21 +285,41 @@ static int shm_release(struct inode *ino, struct file *file)return 0 ; } -#ifndef CONFIG_MMU +static int shm_fsync (struct file *file,struct dentry *dentry,int datasync) +{ +int (*fsync) (struct file *,struct dentry *,int datasync); +struct shm_file_data *sfd =shm_file_data (file); +int ret = -EINVAL ; + + fsync = sfd->file->f_op->fsync; +if (fsync) + ret =fsync (sfd->file, sfd->file->f_path.dentry, datasync); +return ret; +} +static unsigned long shm_get_unmapped_area (struct file *file,unsigned long addr,unsigned long len,unsigned long pgoff,unsigned long flags) {struct shm_file_data *sfd =shm_file_data (file); -return sfd->file->f_op->get_unmapped_area (sfd->file, addr, len, pgoff, - flags); +return get_unmapped_area (sfd->file, addr, len, pgoff, flags); +} + +int is_file_shm_hugepages (struct file *file) +{ +int ret =0 ; + +if (file->f_op == &shm_file_operations) { +struct shm_file_data *sfd; + sfd =shm_file_data (file); + ret =is_file_hugepages (sfd->file); + } +return ret; } -#else -#define shm_get_unmapped_area NULL -#endif static const struct file_operations shm_file_operations = { .mmap = shm_mmap, + .fsync = shm_fsync, .release = shm_release, .get_unmapped_area = shm_get_unmapped_area, };
Quoted 10 lines · Adam Litke
static inline int
is_file_hugepages(struct file *file){include/linux/hugetlb.h +6
-1-return file->f_op == &hugetlbfs_file_operations; +if (file->f_op == &hugetlbfs_file_operations) +return 1 ; +if (is_file_shm_hugepages (file)) +return 1 ; + +return 0 ; }
...
ipc/shm.c static int shm_release(struct inode *ino, struct file *file) · line 317
Quoted 10 lines
ipc/shm.c +10
-0+int is_file_shm_hugepages (struct file *file) +{ +int ret =0 ; + +if (file->f_op == &shm_file_operations) { +struct shm_file_data *sfd; + sfd =shm_file_data (file); + ret =is_file_hugepages (sfd->file); + } +return ret;
A comment to prepare others for the impending doubletake might be nice. Or maybe just open-coding the equality check for &huetlbfs_file_operations in is_file_shm_hugepages() if others find it as jarring as I. Please extend my ack to any follow-up fiddling with that.
The patch addresses relatively straightforward issues and naturally at that.
-- wli
Quoted 12 lines · Bill Irwin
Quoted 10 lines · Adam Litke
static inline int
is_file_hugepages(struct file *file){include/linux/hugetlb.h +6
-1-return file->f_op == &hugetlbfs_file_operations; +if (file->f_op == &hugetlbfs_file_operations) +return 1 ; +if (is_file_shm_hugepages (file)) +return 1 ; + +return 0 ; }...
Quoted 15 lines
ipc/shm.c static int shm_release(struct inode *ino, struct file *file) · line 317
Quoted 10 lines
ipc/shm.c +10
-0+int is_file_shm_hugepages (struct file *file) +{ +int ret =0 ; + +if (file->f_op == &shm_file_operations) { +struct shm_file_data *sfd; + sfd =shm_file_data (file); + ret =is_file_hugepages (sfd->file); + } +return ret;A comment to prepare others for the impending doubletake might be nice. Or maybe just open-coding the equality check for
&huetlbfs_file_operationsinis_file_shm_hugepages()if others find it as jarring as I. Please extend my ack to any follow-up fiddling with that.
You did notice we are testing a different struct file?
Quoted 2 lines
The patch addresses relatively straightforward issues and naturally at that.
The whole concept is recursive so I'm not certain being a recursive check is that bad but I understand the point.
I think the right answer is most likely to add an extra file method or two so we can remove the need for is_file_hugepages.
There are still 4 calls to is_file_hugepages in ipc/shm.c and 2 calls in mm/mmap.c not counting the one in is_file_shm_hugepages.
The special cases make it difficult to properly wrap hugetlbfs files with another file, which is why we have the weird special case above.
Eric
Quoted 38 lines · Eric W. Biederman
Quoted 12 lines · Bill Irwin
Quoted 10 lines · Adam Litke
static inline int
is_file_hugepages(struct file *file){include/linux/hugetlb.h +6
-1-return file->f_op == &hugetlbfs_file_operations; +if (file->f_op == &hugetlbfs_file_operations) +return 1 ; +if (is_file_shm_hugepages (file)) +return 1 ; + +return 0 ; }...
Quoted 15 lines
ipc/shm.c static int shm_release(struct inode *ino, struct file *file) · line 317
Quoted 10 lines
ipc/shm.c +10
-0+int is_file_shm_hugepages (struct file *file) +{ +int ret =0 ; + +if (file->f_op == &shm_file_operations) { +struct shm_file_data *sfd; + sfd =shm_file_data (file); + ret =is_file_hugepages (sfd->file); + } +return ret;A comment to prepare others for the impending doubletake might be nice. Or maybe just open-coding the equality check for
&huetlbfs_file_operationsinis_file_shm_hugepages()if others find it as jarring as I. Please extend my ack to any follow-up fiddling with that.You did notice we are testing a different struct file?
Quoted 2 lines
The patch addresses relatively straightforward issues and naturally at that.
The whole concept is recursive so I'm not certain being a recursive check is that bad but I understand the point.
Quoted 8 lines
I think the right answer is most likely to add an extra file method or two so we can remove the need for
is_file_hugepages.There are still 4 calls to
is_file_hugepagesinipc/shm.cand 2 calls inmm/mmap.cnot counting the one inis_file_shm_hugepages.The special cases make it difficult to properly wrap hugetlbfs files with another file, which is why we have the weird special case above.
:) Enter my remove-is_file_hugepages() patches (which I posted a few weeks ago). I'll rework them and repost soon. That should help to make all of this cleaner.
Signature · 2 lines
Adam Litke - (agl at us.ibm.com) IBM Linux Technology Center
Quoted 3 lines · Adam Litke
:) Enter my remove-is_file_hugepages() patches (which I posted a few weeks ago). I'll rework them and repost soon. That should help to make all of this cleaner.
Those were great. I've wanted something like them for a long, long time.
-- wli
Quoted 4 lines · Bill Irwin
Quoted 4 lines
A comment to prepare others for the impending doubletake might be nice. Or maybe just open-coding the equality check for
&hugetlbfs_file_operationsinis_file_shm_hugepages()if others find it as jarring as I. Please extend my ack to any follow-up fiddling with that.
Quoted 1 line · Eric W. Biederman
You did notice we are testing a different struct file?
Yes.
Quoted 2 lines · Bill Irwin
Quoted 2 lines
The patch addresses relatively straightforward issues and naturally at that.
Quoted 2 lines · Eric W. Biederman
The whole concept is recursive so I'm not certain being a recursive check is that bad but I understand the point.
Hence my characterization as natural. You did notice this was an ack?
Quoted 6 lines · Eric W. Biederman
I think the right answer is most likely to add an extra file method or two so we can remove the need for
is_file_hugepages.
There are still 4 calls tois_file_hugepagesinipc/shm.cand 2 calls inmm/mmap.cnot counting the one inis_file_shm_hugepages. The special cases make it difficult to properly wrap hugetlbfs files with another file, which is why we have the weird special case above.
It's not clear to me that the core can be insulated from hugetlb's distinct pagecache and memory mapping granularities in a Linux-native manner, but if you come up with something new or manage to get the known methods past Linus, akpm, et al, more power to you.
I'm not entirely sure what you're up to, but I'm mostly here to sanction others' design notions since my own are far too extreme, and, of course, review and ack patches, take bugreports and write fixes (not that I've managed to get to any of them first in a long while, if ever), and so on. I say killing the is_whatever_hugepages() checks with whatever abstraction is good, since I don't like them myself, provided it's sane. Go for it.
-- wli
Quoted 8 lines · Bill Irwin
Quoted 6 lines · Eric W. Biederman
I think the right answer is most likely to add an extra file method or two so we can remove the need for
is_file_hugepages.
There are still 4 calls tois_file_hugepagesinipc/shm.cand 2 calls inmm/mmap.cnot counting the one inis_file_shm_hugepages. The special cases make it difficult to properly wrap hugetlbfs files with another file, which is why we have the weird special case above.
Quoted 4 lines
It's not clear to me that the core can be insulated from hugetlb's distinct pagecache and memory mapping granularities in a Linux-native manner, but if you come up with something new or manage to get the known methods past Linus, akpm, et al, more power to you.
I will agree with that there are limits on what can be achieved. However looking at where we have tests for is_file_hugepages most of those tests don't appear to be inherently anything to do with huge pages, so it wouldn't surprise me if we couldn't generalize things a little more.
Quoted 6 lines
I'm not entirely sure what you're up to, but I'm mostly here to sanction others' design notions since my own are far too extreme, and, of course, review and ack patches, take bugreports and write fixes (not that I've managed to get to any of them first in a long while, if ever), and so on. I say killing the
is_whatever_hugepages()checks with whatever abstraction is good, since I don't like them myself, provided it's sane. Go for it.
Mostly I had reference counting and consistency problems with ipc/shm.c that had horrible leak potential when I exited a ipc namespace. Implementing everything as stacked files made the code simpler and more maintainable. (shm_nattach stopped being a special case yea!)
I'm happy to stop here but if someone cares to proceed with removing is_file_hugepages I want to encourage that. I don't see any other cleanups short of that are really worth doing.
Everything in ipc/shm.c could be considered a weird special case, so I'm not going to worry about it too much. Although removing those special cases is good.
There is some odd accounting logic in mm/mmap.c based on is_file_hugepages and there is the get_unmapped_area case. For get_unmapped_area I see no reason to presume that the only kind of file that must live at a specific address are huge pages (even if that is the only kind of file where we have that case today). So generalizing that check should be relatively straight forward.
Eric