Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()
Hi Nick, Thanks for the review. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()
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 <[EMAIL PROTECTED]> > --- > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()
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 <[EMAIL PROTECTED]> --- 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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2 resend] mm: various cleanups in get_user_pages()
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 <[EMAIL PROTECTED]> --- 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, , 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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()
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 [EMAIL PROTECTED] --- 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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2 resend] mm: various cleanups in get_user_pages()
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 [EMAIL PROTECTED] --- 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); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2 resend] mm: various cleanups in get_user_pages()
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 [EMAIL PROTECTED] --- 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); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/