[PATCH 2/2] mm: various cleanups in get_user_pages()

[PATCH 2/2] mm: various cleanups in get_user_pages()

[Eugene Teo]

This patch contains various cleanups, including making sure vma is valid, and
the return value of follow_hugetlb_page() is validated.

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 mm/memory.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 54f951b..49403a8 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1003,7 +1003,9 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned int foll_flags;
 
 		vma = find_extend_vma(mm, start);
-		if (!vma && in_gate_area(tsk, start)) {
+		if (!vma)
+			goto finish_or_fault;
+		if (in_gate_area(tsk, start)) {
 			unsigned long pg = start & PAGE_MASK;
 			struct vm_area_struct *gate_vma = get_gate_vma(tsk);
 			pgd_t *pgd;
@@ -1011,7 +1013,7 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			pmd_t *pmd;
 			pte_t *pte;
 			if (write) /* user gate pages are read-only */
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			if (pg > TASK_SIZE)
 				pgd = pgd_offset_k(pg);
 			else
@@ -1021,11 +1023,11 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			BUG_ON(pud_none(*pud));
 			pmd = pmd_offset(pud, pg);
 			if (pmd_none(*pmd))
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			pte = pte_offset_map(pmd, pg);
 			if (pte_none(*pte)) {
 				pte_unmap(pte);
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			}
 			if (pages) {
 				struct page *page = vm_normal_page(gate_vma, start, *pte);
@@ -1041,13 +1043,15 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			continue;
 		}
 
-		if (!vma || (vma->vm_flags & (VM_IO | VM_PFNMAP))
+		if ((vma->vm_flags & (VM_IO | VM_PFNMAP))
 				|| !(vm_flags & vma->vm_flags))
-			return i ? : -EFAULT;
+			goto finish_or_fault;
 
 		if (is_vm_hugetlb_page(vma)) {
 			i = follow_hugetlb_page(mm, vma, pages, vmas,
 						&start, len, i, write);
+			if (i == -EFAULT)
+				goto finish_or_fault;
 			continue;
 		}
 
@@ -1080,9 +1084,9 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 						foll_flags & FOLL_WRITE);
 				if (ret & VM_FAULT_ERROR) {
 					if (ret & VM_FAULT_OOM)
-						return i ? i : -ENOMEM;
+						goto finish_or_oom;
 					else if (ret & VM_FAULT_SIGBUS)
-						return i ? i : -EFAULT;
+						goto finish_or_fault;
 					BUG();
 				}
 				if (ret & VM_FAULT_MAJOR)
@@ -1115,6 +1119,12 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		}
 	}
 	return i;
+
+finish_or_oom:
+	return i ? : -ENOMEM;
+
+finish_or_fault:
+	return i ? : -EFAULT;
 }
 EXPORT_SYMBOL(get_user_pages);
 

[PATCH 2/2 resend] mm: various cleanups in get_user_pages()

[Eugene Teo]

Argh. Sorry, I spotted a mistake. Here's a resend:

[PATCH 2/2] mm: various cleanups in get_user_pages()

This patch contains various cleanups, including making sure vma is valid, and
the return value of follow_hugetlb_page() is validated.

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 mm/memory.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 54f951b..77105c4 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1003,7 +1003,9 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned int foll_flags;
 
 		vma = find_extend_vma(mm, start);
-		if (!vma && in_gate_area(tsk, start)) {
+		if (!vma)
+			goto finish_or_fault;
+		if (in_gate_area(tsk, start)) {
 			unsigned long pg = start & PAGE_MASK;
 			struct vm_area_struct *gate_vma = get_gate_vma(tsk);
 			pgd_t *pgd;
@@ -1011,7 +1013,7 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			pmd_t *pmd;
 			pte_t *pte;
 			if (write) /* user gate pages are read-only */
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			if (pg > TASK_SIZE)
 				pgd = pgd_offset_k(pg);
 			else
@@ -1021,11 +1023,11 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			BUG_ON(pud_none(*pud));
 			pmd = pmd_offset(pud, pg);
 			if (pmd_none(*pmd))
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			pte = pte_offset_map(pmd, pg);
 			if (pte_none(*pte)) {
 				pte_unmap(pte);
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			}
 			if (pages) {
 				struct page *page = vm_normal_page(gate_vma, start, *pte);
@@ -1041,13 +1043,15 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			continue;
 		}
 
-		if (!vma || (vma->vm_flags & (VM_IO | VM_PFNMAP))
+		if ((vma->vm_flags & (VM_IO | VM_PFNMAP))
 				|| !(vm_flags & vma->vm_flags))
-			return i ? : -EFAULT;
+			goto finish_or_fault;
 
 		if (is_vm_hugetlb_page(vma)) {
 			i = follow_hugetlb_page(mm, vma, pages, vmas,
 						&start, len, i, write);
+			if (i == -EFAULT)
+				return -EFAULT;
 			continue;
 		}
 
@@ -1080,9 +1084,9 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 						foll_flags & FOLL_WRITE);
 				if (ret & VM_FAULT_ERROR) {
 					if (ret & VM_FAULT_OOM)
-						return i ? i : -ENOMEM;
+						goto finish_or_oom;
 					else if (ret & VM_FAULT_SIGBUS)
-						return i ? i : -EFAULT;
+						goto finish_or_fault;
 					BUG();
 				}
 				if (ret & VM_FAULT_MAJOR)
@@ -1115,6 +1119,12 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		}
 	}
 	return i;
+
+finish_or_oom:
+	return i ? : -ENOMEM;
+
+finish_or_fault:
+	return i ? : -EFAULT;
 }
 EXPORT_SYMBOL(get_user_pages);
 

Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()

[Eugene Teo]

Sorry for the repeated emails. Kindly ignore the previous resend. Please
review this instead. Thanks. I have tested this.

[PATCH 2/2] mm: various cleanups in get_user_pages()
 
This patch contains various cleanups, including making sure vma is valid, and
the return value of follow_hugetlb_page() is validated.

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 mm/memory.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 54f951b..c7e0610 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1003,7 +1003,9 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned int foll_flags;
 
 		vma = find_extend_vma(mm, start);
-		if (!vma && in_gate_area(tsk, start)) {
+		if (!vma)
+			goto finish_or_fault;
+		if (in_gate_area(tsk, start)) {
 			unsigned long pg = start & PAGE_MASK;
 			struct vm_area_struct *gate_vma = get_gate_vma(tsk);
 			pgd_t *pgd;
@@ -1011,7 +1013,7 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			pmd_t *pmd;
 			pte_t *pte;
 			if (write) /* user gate pages are read-only */
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			if (pg > TASK_SIZE)
 				pgd = pgd_offset_k(pg);
 			else
@@ -1021,11 +1023,11 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			BUG_ON(pud_none(*pud));
 			pmd = pmd_offset(pud, pg);
 			if (pmd_none(*pmd))
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			pte = pte_offset_map(pmd, pg);
 			if (pte_none(*pte)) {
 				pte_unmap(pte);
-				return i ? : -EFAULT;
+				goto finish_or_fault;
 			}
 			if (pages) {
 				struct page *page = vm_normal_page(gate_vma, start, *pte);
@@ -1041,9 +1043,9 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			continue;
 		}
 
-		if (!vma || (vma->vm_flags & (VM_IO | VM_PFNMAP))
+		if ((vma->vm_flags & (VM_IO | VM_PFNMAP))
 				|| !(vm_flags & vma->vm_flags))
-			return i ? : -EFAULT;
+			goto finish_or_fault;
 
 		if (is_vm_hugetlb_page(vma)) {
 			i = follow_hugetlb_page(mm, vma, pages, vmas,
@@ -1080,9 +1082,9 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 						foll_flags & FOLL_WRITE);
 				if (ret & VM_FAULT_ERROR) {
 					if (ret & VM_FAULT_OOM)
-						return i ? i : -ENOMEM;
+						goto finish_or_oom;
 					else if (ret & VM_FAULT_SIGBUS)
-						return i ? i : -EFAULT;
+						goto finish_or_fault;
 					BUG();
 				}
 				if (ret & VM_FAULT_MAJOR)
@@ -1115,6 +1117,12 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		}
 	}
 	return i;
+
+finish_or_oom:
+	return i ? : -ENOMEM;
+
+finish_or_fault:
+	return i ? : -EFAULT;
 }
 EXPORT_SYMBOL(get_user_pages);
 

Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()

[Nick Piggin]

On Wednesday 13 February 2008 00:10, Eugene Teo wrote:
> Sorry for the repeated emails. Kindly ignore the previous resend. Please
> review this instead. Thanks. I have tested this.

If it is causing this much problems, can you split the cleanups into
their own patches.


> [PATCH 2/2] mm: various cleanups in get_user_pages()
>
> This patch contains various cleanups, including making sure vma is valid,
> and the return value of follow_hugetlb_page() is validated.
>
> Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
> ---
>  mm/memory.c |   24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 54f951b..c7e0610 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1003,7 +1003,9 @@ int get_user_pages(struct task_struct *tsk, struct
> mm_struct *mm, unsigned int foll_flags;
>
>  		vma = find_extend_vma(mm, start);
> -		if (!vma && in_gate_area(tsk, start)) {
> +		if (!vma)
> +			goto finish_or_fault;
> +		if (in_gate_area(tsk, start)) {
>  			unsigned long pg = start & PAGE_MASK;
>  			struct vm_area_struct *gate_vma = get_gate_vma(tsk);
>  			pgd_t *pgd;

Doesn't this break the logic?

If you don't have a vma, but you are in the gate area, then you
should use the gate vma. With your patch, gate area will fault.

> @@ -1011,7 +1013,7 @@ int get_user_pages(struct task_struct *tsk, struct
> mm_struct *mm, pmd_t *pmd;
>  			pte_t *pte;
>  			if (write) /* user gate pages are read-only */
> -				return i ? : -EFAULT;
> +				goto finish_or_fault;

I don't know if this is exactly a cleanup or not... I guess gcc
probably isn't smart enough to fold them all together, so it should
use a little less code in the unlikely branches. Does it?

Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()

[Eugene Teo]

Hi Nick,

Thanks for the review.

<quote sender="Nick Piggin">
> On Wednesday 13 February 2008 00:10, Eugene Teo wrote:
[...]
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 54f951b..c7e0610 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1003,7 +1003,9 @@ int get_user_pages(struct task_struct *tsk, struct
> > mm_struct *mm, unsigned int foll_flags;
> >
> >  		vma = find_extend_vma(mm, start);
> > -		if (!vma && in_gate_area(tsk, start)) {
> > +		if (!vma)
> > +			goto finish_or_fault;
> > +		if (in_gate_area(tsk, start)) {
> >  			unsigned long pg = start & PAGE_MASK;
> >  			struct vm_area_struct *gate_vma = get_gate_vma(tsk);
> >  			pgd_t *pgd;
> 
> Doesn't this break the logic?
> 
> If you don't have a vma, but you are in the gate area, then you
> should use the gate vma. With your patch, gate area will fault.

Yes, you are right. I also relooked at the patch, and actually vma is
validated after if (... in_gate_area(tsk, start)) { ... }, so my patch
is not correct.

> > @@ -1011,7 +1013,7 @@ int get_user_pages(struct task_struct *tsk, struct
> > mm_struct *mm, pmd_t *pmd;
> >  			pte_t *pte;
> >  			if (write) /* user gate pages are read-only */
> > -				return i ? : -EFAULT;
> > +				goto finish_or_fault;
> 
> I don't know if this is exactly a cleanup or not... I guess gcc
> probably isn't smart enough to fold them all together, so it should
> use a little less code in the unlikely branches. Does it?

Agree.

Eugene