New: Kernel bug at mm/hugetlb.c:741

https://bugzilla.kernel.org/show_bug.cgi?id=199037

Bug ID: 199037
Summary: Kernel bug at mm/hugetlb.c:741
Product: Memory Management
Version: 2.5
Kernel Version: 4.16.0-rc3
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Page Allocator
Assignee: [email protected]
Reporter: [email protected]
Regression: No

Created attachment 274595
--> https://bugzilla.kernel.org/attachment.cgi?id=274595&action=edit
crash.c

Hello,
I apologize as this is my first time reporting a bug. When I compile and run the attached file it crashes the latest kernel running in QEMU. Call trace
here: https://pastebin.com/1mMQvH0E

Let me know if you have any questions.

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 22 lines · On Tue, 06 Mar 2018 21:11:50 +0000 [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=199037

Bug ID: 199037
Summary: Kernel bug at mm/hugetlb.c:741
Product: Memory Management
Version: 2.5
Kernel Version: 4.16.0-rc3
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Page Allocator
Assignee: [email protected]
Reporter: [email protected]
Regression: No

Created attachment 274595
--> https://bugzilla.kernel.org/attachment.cgi?id=274595&action=edit
crash.c

Quoted 7 lines

Hello,
I apologize as this is my first time reporting a bug. When I compile and run the attached file it crashes the latest kernel running in QEMU. Call trace
here: https://pastebin.com/1mMQvH0E

Let me know if you have any questions.

Thanks for the report.

That's VM_BUG_ON(resv_map->adds_in_progress) in resv_map_release().

Do you know if earlier kernel versions are affected?

It looks quite bisectable. Does the crash happen every time the test program is run?

Quoted 7 lines · Andrew Morton

That's VM_BUG_ON(resv_map->adds_in_progress) in resv_map_release().

Do you know if earlier kernel versions are affected?

It looks quite bisectable. Does the crash happen every time the test program is run?

I'll take a look. There was a previous bug in this area:
ff8c0c53: mm/hugetlb.c: don't call region_abort if region_chg fails

Quoted 11 lines · Mike Kravetz

Quoted 7 lines · Andrew Morton

That's VM_BUG_ON(resv_map->adds_in_progress) in resv_map_release().

Do you know if earlier kernel versions are affected?

It looks quite bisectable. Does the crash happen every time the test program is run?

I'll take a look. There was a previous bug in this area:
ff8c0c53: mm/hugetlb.c: don't call region_abort if region_chg fails

This is similar to the issue addressed in 045c7a3f ("fix offset overflow in hugetlbfs mmap"). The problem here is that the pgoff argument passed to remap_file_pages() is 0x20000000000000. In the process of converting this to a page offset and putting it in vm_pgoff, and then converting back to bytes to compute mapping length we end up with 0. We ultimately end up passing (from,to) page offsets into hugetlbfs where from is greater than to. :( This confuses the heck out the the huge page reservation code as the 'negative' range looks like an error and we never complete the reservation process and leave the 'adds_in_progress'.

This issue has existed for a long time. The VM_BUG_ON just happens to catch the situation which was previously not reported or had some other side effect. Commit 045c7a3f tried to catch these overflow issues when converting types, but obviously missed this one. I can easily add a test for this specific value/condition, but want to think about it a little more and see if there is a better way to catch all of these.

--
Mike Kravetz

Quoted 11 lines · Mike Kravetz

Quoted 7 lines · Andrew Morton

That's VM_BUG_ON(resv_map->adds_in_progress) in resv_map_release().

Do you know if earlier kernel versions are affected?

It looks quite bisectable. Does the crash happen every time the test program is run?

I'll take a look. There was a previous bug in this area:
ff8c0c53: mm/hugetlb.c: don't call region_abort if region_chg fails

This is similar to the issue addressed in 045c7a3f ("fix offset overflow in hugetlbfs mmap"). The problem here is that the pgoff argument passed to remap_file_pages() is 0x20000000000000. In the process of converting this to a page offset and putting it in vm_pgoff, and then converting back to bytes to compute mapping length we end up with 0. We ultimately end up passing (from,to) page offsets into hugetlbfs where from is greater than to. :( This confuses the heck out the the huge page reservation code as the 'negative' range looks like an error and we never complete the reservation process and leave the 'adds_in_progress'.

This issue has existed for a long time. The VM_BUG_ON just happens to catch the situation which was previously not reported or had some other side effect. Commit 045c7a3f tried to catch these overflow issues when converting types, but obviously missed this one. I can easily add a test for this specific value/condition, but want to think about it a little more and see if there is a better way to catch all of these.

Quoted 23 lines · Mike Kravetz

Quoted 11 lines · Mike Kravetz

Quoted 7 lines · Andrew Morton

That's VM_BUG_ON(resv_map->adds_in_progress) in resv_map_release().

Do you know if earlier kernel versions are affected?

It looks quite bisectable. Does the crash happen every time the test program is run?

I'll take a look. There was a previous bug in this area:
ff8c0c53: mm/hugetlb.c: don't call region_abort if region_chg fails

This is similar to the issue addressed in 045c7a3f ("fix offset overflow in hugetlbfs mmap"). The problem here is that the pgoff argument passed to remap_file_pages() is 0x20000000000000. In the process of converting this to a page offset and putting it in vm_pgoff, and then converting back to bytes to compute mapping length we end up with 0. We ultimately end up passing (from,to) page offsets into hugetlbfs where from is greater than to. :( This confuses the heck out the the huge page reservation code as the 'negative' range looks like an error and we never complete the reservation process and leave the 'adds_in_progress'.

Quoted 6 lines

This issue has existed for a long time. The VM_BUG_ON just happens to catch the situation which was previously not reported or had some other side effect. Commit 045c7a3f tried to catch these overflow issues when converting types, but obviously missed this one. I can easily add a test for this specific value/condition, but want to think about it a little more and see if there is a better way to catch all of these.

Well, I instrumented hugetlbfs_file_mmap when called via the remap_file_pages system call path. Upon entry, vma->vm_pgoff is 0x20000000000000 which is the same as the value of the argument pgoff passed to the system call. vm_pgoff really should be a page offset (i.e. 0x20000000000000 >> PAGE_SHIFT). So, there is also an issue earlier in the remap_file_pages system call sequence.

A vma with vm_pgoff large enough to overflow a loff_t type when converted to a byte offset can be passed via the remap_file_pages system call. The hugetlbfs mmap routine uses the byte offset to calculate reservations and file size.

A sequence such as:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
kernel BUG at mm/hugetlb.c:749!

Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a mapping with a negative range (end < start) that leaves invalid state which causes the BUG.

Reported-by Nic Losby <[email protected]>

Signed-off-by Mike Kravetz <[email protected]>

---
 fs/hugetlbfs/inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/hugetlbfs/inode.c +4 -4

index 8fe1b0aa2896..cb288dec5564 100644
@@ -127,12 +127,13 @@ static int hugetlbfs_file_mmap(struct file *file,
 struct vm_area_struct *vma)
        vma->vm_ops = &hugetlb_vm_ops;
 
        /*
-        * Offset passed to mmap (before page shift) could have been
-        * negative when represented as a (l)off_t.
+        * page based offset in vm_pgoff could be sufficiently large to
+        * overflow a (l)off_t when converted to byte offset.
         */
-       if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
+       if (vma->vm_pgoff && ((loff_t)vma->vm_pgoff << PAGE_SHIFT) <= 0)
                return -EINVAL;
 
+       /* must be huge page aligned */
        if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
                return -EINVAL;
 
--

2.13.6

A vma with vm_pgoff large enough to overflow a loff_t type when converted to a byte offset can be passed via the remap_file_pages system call. The hugetlbfs mmap routine uses the byte offset to calculate reservations and file size.

A sequence such as:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
kernel BUG at mm/hugetlb.c:749!

Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a mapping with a negative range (end < start) that leaves invalid state which causes the BUG.

Reported-by Nic Losby <[email protected]>

Signed-off-by Mike Kravetz <[email protected]>

---
 fs/hugetlbfs/inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/hugetlbfs/inode.c +0 -0

index 8fe1b0aa2896..cb288dec5564 100644
@@ -127,12 +127,13 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	vma->vm_ops = &hugetlb_vm_ops;

fs/hugetlbfs/inode.c +1 -0

+	/* must be huge page aligned */
 	if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
 		return -EINVAL;
 
 

Hi Mike,

Quoted 36 lines · Mike Kravetz

A vma with vm_pgoff large enough to overflow a loff_t type when converted to a byte offset can be passed via the remap_file_pages system call. The hugetlbfs mmap routine uses the byte offset to calculate reservations and file size.

A sequence such as:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
kernel BUG at mm/hugetlb.c:749!

Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a mapping with a negative range (end < start) that leaves invalid state which causes the BUG.

Reported-by Nic Losby <[email protected]>

Signed-off-by Mike Kravetz <[email protected]>

---
 fs/hugetlbfs/inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/hugetlbfs/inode.c +0 -0

index 8fe1b0aa2896..cb288dec5564 100644
@@ -127,12 +127,13 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	vma->vm_ops = &hugetlb_vm_ops;

fs/hugetlbfs/inode.c static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma) · line 134

Quoted 10 lines

fs/hugetlbfs/inode.c +3 -3

 
 	/*
-	 * Offset passed to mmap (before page shift) could have been
-	 * negative when represented as a (l)off_t.
+	 * page based offset in vm_pgoff could be sufficiently large to
+	 * overflow a (l)off_t when converted to byte offset.
 	 */
-	if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
+	if (vma->vm_pgoff && ((loff_t)vma->vm_pgoff << PAGE_SHIFT) <= 0)
 		return -EINVAL;

This seems still no the right fix, taking the following case as an example:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x0020001000000000, 0);

You should just check the highest PAGE_SHIFT+1 bits of pgoff in you want check at this point, right?

However, region_chg makes me a litter puzzle that when its return value < 0, sometime adds_in_progress is added like this case, while sometime it is not. so why not just change at the beginning of region_chg ?

	if (f > t)
		return -EINVAL;

Quoted 5 lines · Yisheng Xie

However, region_chg makes me a litter puzzle that when its return value < 0, sometime adds_in_progress is added like this case, while sometime it is not. so why not just change at the beginning of region_chg ?

	if (f > t)
		return -EINVAL;

If region_chg returns a value < 0, this indicates an error and adds_in_progress should not be incremented. In the case of this bug, region_chg was passed values where f > t. Of course, this should never happen. But, because it assumed f <= t, it returned a negative count needed huge page reservations. The calling code interpreted the negative value as an error and a subsequent region_add or region_abort.

I am not opposed to adding the suggested "if (f > t)". However, the region tracking routines are simple helpers only used by the hugetlbfs code and the assumption is that they are being called correctly. As such, I would prefer to leave off the check. But, this is the second time they have been called incorrectly due to insufficient argument checking. If we do add this to region_chg, I would also add the check to all region_* routines for consistency.

I will send out a V2 of this patch tomorrow with the corrected overflow checking and possibly checks added to the region_* routines.

A vma with vm_pgoff large enough to overflow a loff_t type when converted to a byte offset can be passed via the remap_file_pages system call. The hugetlbfs mmap routine uses the byte offset to calculate reservations and file size.

A sequence such as:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
kernel BUG at mm/hugetlb.c:749!

Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a mapping with a negative range (end < start) that leaves invalid state which causes the BUG.

The previous overflow fix to this code was incomplete and did not take the remap_file_pages system call into account.

fs/hugetlbfs/inode.c +1 -0

@@ -111,6 +111,7 @@ static void huge_pagevec_release(struct pagevec *pvec)
 static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct inode *inode = file_inode(file);
+	unsigned long ovfl_mask;
 	loff_t len, vma_len;
 	int ret;
 	struct hstate *h = hstate_file(file);

@@ -127,12 +128,16 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	vma->vm_ops = &hugetlb_vm_ops;
 

Quoted 40 lines · On Thu, 8 Mar 2018 13:05:02 -0800 Mike Kravetz <[email protected]> wrote:

A vma with vm_pgoff large enough to overflow a loff_t type when converted to a byte offset can be passed via the remap_file_pages system call. The hugetlbfs mmap routine uses the byte offset to calculate reservations and file size.

A sequence such as:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
kernel BUG at mm/hugetlb.c:749!

Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a mapping with a negative range (end < start) that leaves invalid state which causes the BUG.

The previous overflow fix to this code was incomplete and did not take the remap_file_pages system call into account.

fs/hugetlbfs/inode.c +1 -0

@@ -111,6 +111,7 @@ static void huge_pagevec_release(struct pagevec *pvec)
 static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct inode *inode = file_inode(file);
+	unsigned long ovfl_mask;
 	loff_t len, vma_len;
 	int ret;
 	struct hstate *h = hstate_file(file);

@@ -127,12 +128,16 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	vma->vm_ops = &hugetlb_vm_ops;
 

fs/hugetlbfs/inode.c static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma) · line 136

Quoted 10 lines

fs/hugetlbfs/inode.c +5 -3

 	/*
-	 * Offset passed to mmap (before page shift) could have been
-	 * negative when represented as a (l)off_t.
+	 * page based offset in vm_pgoff could be sufficiently large to
+	 * overflow a (l)off_t when converted to byte offset.
 	 */
-	if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
+	ovfl_mask = (1UL << (PAGE_SHIFT + 1)) - 1;
+	ovfl_mask <<= ((sizeof(unsigned long) * BITS_PER_BYTE) -
+		       (PAGE_SHIFT + 1));

That's a compile-time constant. The compiler will indeed generate an immediate load, but I think it would be better to make the code look more like we know that it's a constant, if you get what I mean. Something like

/*
* If a pgoff_t is to be converted to a byte index, this is the max value it
* can have to avoid overflow in that conversion.
*/
#define PGOFF_T_MAX <long string of crap>

And I bet that this constant could be used elsewhere - surely it's a very common thing to be checking for.

Also, the expression seems rather complicated. Why are we adding 1 to PAGE_SHIFT? Isn't there a logical way of using PAGE_MASK?

The resulting constant is 0xfff8000000000000 on 64-bit. We could just use along the lines of

	1UL << (BITS_PER_LONG - PAGE_SHIFT - 1)

But why the -1? We should be able to handle a pgoff_t of 0xfff0000000000000 in this code?

Also, we later to

	len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
	/* check for overflow */
	if (len < vma_len)
		return -EINVAL;

which is ungainly: even if we passed the PGOFF_T_MAX test, there can still be an overflow which we still must check for. Is that avoidable? Probably not...

Here are a couple options for computing the mask. I changed the name you suggested to make it more obvious that the mask is being used to check for loff_t overflow.

If we want to explicitly comptue the mask as in code above.

#define PGOFF_LOFFT_MAX \
	(((1UL << (PAGE_SHIFT + 1)) - 1) <<  (BITS_PER_LONG - (PAGE_SHIFT + 1)))

Or, we use PAGE_MASK
#define PGOFF_LOFFT_MAX (PAGE_MASK << (BITS_PER_LONG - (2 * PAGE_SHIFT) - 1))

In either case, we need a big comment explaining the mask and how we have that extra bit +/- 1 because the offset will be converted to a signed value.

Quoted 7 lines

Also, we later to

	len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
	/* check for overflow */
	if (len < vma_len)
		return -EINVAL;

Quoted 3 lines

which is ungainly: even if we passed the PGOFF_T_MAX test, there can still be an overflow which we still must check for. Is that avoidable? Probably not...

Yes, it is required. That check takes into account the length argument which is added to page offset. So, yes you can pass the first check and fail this one.

A vma with vm_pgoff large enough to overflow a loff_t type when converted to a byte offset can be passed via the remap_file_pages system call. The hugetlbfs mmap routine uses the byte offset to calculate reservations and file size.

A sequence such as:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
kernel BUG at mm/hugetlb.c:749!

Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a mapping with a negative range (end < start) that leaves invalid state which causes the BUG.

The previous overflow fix to this code was incomplete and did not take the remap_file_pages system call into account.

...

A vma with vm_pgoff large enough to overflow a loff_t type when converted to a byte offset can be passed via the remap_file_pages system call. The hugetlbfs mmap routine uses the byte offset to calculate reservations and file size.

A sequence such as:
mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0); remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
kernel BUG at mm/hugetlb.c:749!

Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a mapping with a negative range (end < start) that leaves invalid state which causes the BUG.

The previous overflow fix to this code was incomplete and did not take the remap_file_pages system call into account.

Fixes 045c7a3f53d9 ("hugetlbfs: fix offset overflow in hugetlbfs mmap")

Cc <[email protected]>

Reported-by Nic Losby <[email protected]>

Signed-off-by Mike Kravetz <[email protected]>

---
Changes in v3
* Use a simpler mask computation as suggested by Andrew Morton
Changes in v2
* Use bitmask for overflow check as suggested by Yisheng Xie
* Add explicit (from > to) check when setting up reservations
* Cc stable

 fs/hugetlbfs/inode.c | 16 +++++++++++++---
 mm/hugetlb.c         |  6 ++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

fs/hugetlbfs/inode.c +13 -3

index 8fe1b0aa2896..e46117dc006a 100644
@@ -108,6 +108,15 @@ static void huge_pagevec_release(struct pagevec *pvec)
 	pagevec_reinit(pvec);
 }
 
+/*
+ * Mask used when checking the page offset value passed in via system
+ * calls.  This value will be converted to a loff_t which is signed.
+ * Therefore, we want to check the upper PAGE_SHIFT + 1 bits of the
+ * value.  The extra bit (- 1 in the shift value) is to take the sign
+ * bit into account.
+ */
+#define PGOFF_LOFFT_MAX (PAGE_MASK << (BITS_PER_LONG - (2 * PAGE_SHIFT) - 1))
+
 static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct inode *inode = file_inode(file);

@@ -127,12 +136,13 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	vma->vm_ops = &hugetlb_vm_ops;
 
 	/*
-	 * Offset passed to mmap (before page shift) could have been
-	 * negative when represented as a (l)off_t.
+	 * page based offset in vm_pgoff could be sufficiently large to
+	 * overflow a (l)off_t when converted to byte offset.
 	 */
-	if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
+	if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
 		return -EINVAL;
 
+	/* must be huge page aligned */
 	if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
 		return -EINVAL;
 

mm/hugetlb.c +6 -0

index 7c204e3d132b..8eeade0a0b7a 100644
@@ -4374,6 +4374,12 @@ int hugetlb_reserve_pages(struct inode *inode,
 	struct resv_map *resv_map;
 	long gbl_reserve;
 
+	/* This should never happen */
+	if (from > to) {
+		VM_WARN(1, "%s called with a negative range\n", __func__);
+		return -EINVAL;
+	}
+
 	/*
 	 * Only apply hugepage reservation if asked. At fault time, an
 	 * attempt will be made for VM_NORESERVE to allocate a page

Signature · 1 line

2.13.6

On Thu 08-03-18 16:27:26, Mike Kravetz wrote:

OK, looks good to me. Hairy but seems to be the easiest way around this.

Acked-by Michal Hocko <[email protected]>

fs/hugetlbfs/inode.c static void huge_pagevec_release(struct pagevec *pvec) · line 118

Quoted 8 lines

fs/hugetlbfs/inode.c +8 -0

+/*
+ * Mask used when checking the page offset value passed in via system
+ * calls.  This value will be converted to a loff_t which is signed.
+ * Therefore, we want to check the upper PAGE_SHIFT + 1 bits of the
+ * value.  The extra bit (- 1 in the shift value) is to take the sign
+ * bit into account.
+ */
+#define PGOFF_LOFFT_MAX (PAGE_MASK << (BITS_PER_LONG - (2 * PAGE_SHIFT) - 1))

Quoted 5 lines · Michal Hocko

On Thu 08-03-18 16:27:26, Mike Kravetz wrote:

OK, looks good to me. Hairy but seems to be the easiest way around this.

Acked-by Michal Hocko <[email protected]>

<snip>

Quoted 8 lines

fs/hugetlbfs/inode.c static void huge_pagevec_release(struct pagevec *pvec) · line 118

Quoted 8 lines

fs/hugetlbfs/inode.c +8 -0

+/*
+ * Mask used when checking the page offset value passed in via system
+ * calls.  This value will be converted to a loff_t which is signed.
+ * Therefore, we want to check the upper PAGE_SHIFT + 1 bits of the
+ * value.  The extra bit (- 1 in the shift value) is to take the sign
+ * bit into account.
+ */
+#define PGOFF_LOFFT_MAX (PAGE_MASK << (BITS_PER_LONG - (2 * PAGE_SHIFT) - 1))

Thanks Michal,

However, kbuild found a problem with this definition on certain configs. Consider a config where,
BITS_PER_LONG = 32 (32bit config)
PAGE_SHIFT = 16 (64K pages)
This results in the negative shift value.
Not something I would not immediately think of, but a valid config.

The definition has been changed to,

#define PGOFF_LOFFT_MAX \
	(((1UL << (PAGE_SHIFT + 1)) - 1) <<  (BITS_PER_LONG - (PAGE_SHIFT + 1)))

as discussed here,
http://lkml.kernel.org/r/[email protected]