[PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[Adam Litke]

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.

Signed-off-by: Adam Litke <agl@us.ibm.com>
---

 include/linux/hugetlb.h |    8 +++++++-
 include/linux/shm.h     |    5 +++++
 ipc/shm.c               |   32 ++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index a60995a..3f3e7a6 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -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)
diff --git a/include/linux/shm.h b/include/linux/shm.h
index a2c896a..ad2e3af 100644
--- a/include/linux/shm.h
+++ b/include/linux/shm.h
@@ -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__ */
diff --git a/ipc/shm.c b/ipc/shm.c
index eb57e22..abf864d 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -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,
 };

Re: [PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[Bill Irwin]

On Thu, Mar 01, 2007 at 03:46:08PM -0800, Adam Litke wrote:
>  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;
>  }
...
> +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.

Acked-by: William Irwin <bill.irwin@oracle.com>


-- wli

Re: [PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[Eric W. Biederman]

Bill Irwin <bill.irwin@oracle.com> writes:

> On Thu, Mar 01, 2007 at 03:46:08PM -0800, Adam Litke wrote:
>>  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;
>>  }
> ...
>> +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.

You did notice we are testing a different struct file?

> 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

Re: [PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[Adam Litke]

On Wed, 2007-03-07 at 16:03 -0700, Eric W. Biederman wrote:
> Bill Irwin <bill.irwin@oracle.com> writes:
> 
> > On Thu, Mar 01, 2007 at 03:46:08PM -0800, Adam Litke wrote:
> >>  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;
> >>  }
> > ...
> >> +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.
> 
> You did notice we are testing a different struct file?
> 
> > 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.

:) 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.

-- 
Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

Re: [PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[Bill Irwin]

On Wed, Mar 07, 2007 at 05:26:48PM -0600, Adam Litke wrote:
> :) 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

Re: [PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[Bill Irwin]

Bill Irwin <bill.irwin@oracle.com> writes:
>> A comment to prepare others for the impending doubletake might be nice.
>> Or maybe just open-coding the equality check for &hugetlbfs_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.

On Wed, Mar 07, 2007 at 04:03:17PM -0700, Eric W. Biederman wrote:
> You did notice we are testing a different struct file?

Yes.


Bill Irwin <bill.irwin@oracle.com> writes:
>> The patch addresses relatively straightforward issues and naturally at
>> that.

On Wed, Mar 07, 2007 at 04:03:17PM -0700, Eric W. Biederman wrote:
> 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?


On Wed, Mar 07, 2007 at 04:03:17PM -0700, Eric W. Biederman wrote:
> 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.

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

Re: [PATCH] Fix get_unmapped_area and fsync for hugetlb shm segments

[Eric W. Biederman]

Bill Irwin <bill.irwin@oracle.com> writes:

> On Wed, Mar 07, 2007 at 04:03:17PM -0700, Eric W. Biederman wrote:
>> 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.
>
> 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.

> 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