userfaultfd: hugetlbfs: unmap the correct pointer

PATCH Superseded Dan Carpenter 5 participants 12 messages Jan 12, 2017 – Jan 14, 2017

Subsystems (3)

Memory management Kernel janitors MM core

lore

Share & tools

lore

Dan Carpenter

author

2017-01-12 19:20 UTC #0 lore raw

PATCH 1/1 userfaultfd: hugetlbfs: unmap the correct pointer

kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.

Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")

Signed-off-by Dan Carpenter <[email protected]>

mm/memory.c +1 -1

index 6012a05..dfd3604 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(dst_page + 1);
 		else
 			kunmap_atomic(page_kaddr);

Michal Hocko

2017-01-12 19:33 UTC #1 lore raw

Quoted 4 lines · Dan Carpenter

kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")

This looks like a linux-next sha1. This is not stable and will change...

mm/memory.c long copy_huge_page_from_user(struct page *dst_page, · line 4175

Quoted 12 lines

Signed-off-by Dan Carpenter <[email protected]>

mm/memory.c +1 -1

index 6012a05..dfd3604 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(dst_page + 1);

I guess you meant dst_page + i

Quoted 3 lines

mm/memory.c +0 -0

 		else
 			kunmap_atomic(page_kaddr);

Signature · 2 lines

Michal Hocko
SUSE Labs

Dan Carpenter

author

2017-01-13 08:16 UTC #2 lore raw

Quoted 8 lines · Michal Hocko

Quoted 4 lines · Dan Carpenter
kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")
This looks like a linux-next sha1. This is not stable and will change...

Yeah. But probably Andrew is just going to fold it into the original anyway. Probably most of linux-next trees don't rebase so the hash is good and the people who rebase fold it in so it doesn't show up in the released code. It basically never hurts to have the Fixes tag.

Quoted 14 lines

mm/memory.c long copy_huge_page_from_user(struct page *dst_page, · line 4175
Quoted 12 lines
Signed-off-by Dan Carpenter <[email protected]>
mm/memory.c +1 -1
index 6012a05..dfd3604 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(dst_page + 1);
I guess you meant dst_page + i

Huh. I would have sworn I copy and pasted this. Anyway, thanks for catching this. I will resend.

regards,
dan carpenter

Michal Hocko

2017-01-13 08:26 UTC #3 lore raw

Quoted 7 lines · Dan Carpenter

Quoted 6 lines · Michal Hocko
Quoted 4 lines · Dan Carpenter
kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")

Quoted 7 lines

Quoted 2 lines
This looks like a linux-next sha1. This is not stable and will change...
Yeah. But probably Andrew is just going to fold it into the original anyway. Probably most of linux-next trees don't rebase so the hash is good and the people who rebase fold it in so it doesn't show up in the released code. It basically never hurts to have the Fixes tag.

Yeah, I have a vague recollection that some of those sha1 leaked to Linus. Do not have any examples handy though. It is true that Andrew folds those fixes into the original patch so it might be helpful to have
Fixes: mmotm-patch-file-name.patch
instead.

Signature · 2 lines

Michal Hocko
SUSE Labs

Dan Carpenter

author

2017-01-13 08:40 UTC #4 lore raw

Quoted 16 lines · Michal Hocko

Quoted 7 lines · Dan Carpenter
Quoted 6 lines · Michal Hocko
Quoted 4 lines · Dan Carpenter
kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")
Quoted 7 lines
Quoted 2 lines
This looks like a linux-next sha1. This is not stable and will change...
Yeah. But probably Andrew is just going to fold it into the original anyway. Probably most of linux-next trees don't rebase so the hash is good and the people who rebase fold it in so it doesn't show up in the released code. It basically never hurts to have the Fixes tag.

Quoted 5 lines

Yeah, I have a vague recollection that some of those sha1 leaked to Linus. Do not have any examples handy though. It is true that Andrew folds those fixes into the original patch so it might be helpful to have
Fixes: mmotm-patch-file-name.patch

I have no idea how to do that. I'm always just working on linux-next and not the individual trees... I'm interested to hear from Andrew what's easiest because I don't know at all how quilt works.

My work flow is that I have scripts to generate patches from within vim. Most of the time I'm just working on one file but occasionally I will combine two patches together in mutt.

For Dave's networking patches I have a separate git tree where I try to apply them to net and then net-next to see which tree it should go into. Otherwise, I generally assume the maintainer knows which tree they belong in.

regards,
dan carpenter

Dan Carpenter

author

2017-01-13 08:26 UTC #5 lore raw

PATCH v2 1/1 userfaultfd: hugetlbfs: unmap the correct pointer

In reply to Michal Hocko

kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.

Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")

Signed-off-by Dan Carpenter <[email protected]>

---
v2: I was also unmapping the wrong pointer because I had a typo.

mm/memory.c +1 -1

index 6012a05..aca8ef6 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(page_kaddr + i);
 		else
 			kunmap_atomic(page_kaddr);

Michal Hocko

Acked-by on v2 1/1

2017-01-13 08:40 UTC #6 lore raw

Quoted 5 lines · Dan Carpenter

kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")
Signed-off-by Dan Carpenter <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 16 lines

---
v2: I was also unmapping the wrong pointer because I had a typo.

mm/memory.c +1 -1

index 6012a05..aca8ef6 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(page_kaddr + i);
 		else
 			kunmap_atomic(page_kaddr);

Signature · 2 lines

Michal Hocko
SUSE Labs

Mike Kravetz

Reviewed-by

2017-01-13 16:29 UTC #7 lore raw

Quoted 8 lines · Michal Hocko

Quoted 5 lines · Dan Carpenter
kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Michal Hocko <[email protected]>

Thanks for catching this!

Reviewed-by Mike Kravetz <[email protected]>

Signature · 1 line

Mike Kravetz

Quoted 18 lines

Quoted 17 lines

---
v2: I was also unmapping the wrong pointer because I had a typo.

mm/memory.c +1 -1

index 6012a05..aca8ef6 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(page_kaddr + i);
 		else
 			kunmap_atomic(page_kaddr);

Hugh Dickins

2017-01-14 00:02 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 13 lines · Dan Carpenter

kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")
Signed-off-by Dan Carpenter <[email protected]>
---
v2: I was also unmapping the wrong pointer because I had a typo.
mm/memory.c +0 -0
index 6012a05..aca8ef6 100644

mm/memory.c long copy_huge_page_from_user(struct page *dst_page, · line 4177

Quoted 7 lines

mm/memory.c +1 -1

@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(page_kaddr + i);
 		else
 			kunmap_atomic(page_kaddr);

I think you need to look at that again.

Hugh

Andrew Morton

maintainer

2017-01-14 00:13 UTC #9 lore raw

Quoted 10 lines · On Fri, 13 Jan 2017 16:02:37 -0800 (PST) Hugh Dickins <[email protected]> wrote:

Quoted 8 lines · Dan Carpenter
kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")
Signed-off-by Dan Carpenter <[email protected]>
---
v2: I was also unmapping the wrong pointer because I had a typo.

Quoted 15 lines

mm/memory.c long copy_huge_page_from_user(struct page *dst_page, · line 4177

Quoted 12 lines

mm/memory.c +1 -1

index 6012a05..aca8ef6 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(page_kaddr + i);
 		else
 			kunmap_atomic(page_kaddr);

I think you need to look at that again.

um, yup.

mm/memory.c renamed from mm/memory.c~userfaultfd-hugetlbfs-fix-__mcopy_atomic_hugetlb-retry-error-processing-fix-fix +1 -1

@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct pag
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr + i);
+			kunmap(dst_page + i);
 		else
 			kunmap_atomic(page_kaddr);

Dan Carpenter

author

2017-01-14 06:56 UTC #10 lore raw

Thanks, Andrew.

regards,
dan carpenter

Dan Carpenter

author

2017-01-14 06:55 UTC #11 lore raw

In reply to Hugh Dickins

Quoted 10 lines · Hugh Dickins

Quoted 8 lines · Dan Carpenter
kunmap_atomic() and kunmap() take different pointers. People often get these mixed up.
Fixes 16374db2e9a0 ("userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing")
Signed-off-by Dan Carpenter <[email protected]>
---
v2: I was also unmapping the wrong pointer because I had a typo.

Quoted 14 lines

mm/memory.c long copy_huge_page_from_user(struct page *dst_page, · line 4177

Quoted 12 lines

mm/memory.c +1 -1

index 6012a05..aca8ef6 100644
@@ -4172,7 +4172,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 				(const void __user *)(src + i * PAGE_SIZE),
 				PAGE_SIZE);
 		if (allow_pagefault)
-			kunmap(page_kaddr);
+			kunmap(page_kaddr + i);
 		else
 			kunmap_atomic(page_kaddr);

I think you need to look at that again.

Oh wow... What absolute heck! I can't believe how badly I'm messing up on this.

regards,
dan carpenter