| Message ID | 20211201142918.921493-34-Liam.Howlett@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introducing the Maple Tree | expand |
On 12/1/21 15:30, Liam Howlett wrote: > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com> > > Use the VMA iterator instead. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > --- > arch/xtensa/kernel/syscall.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c > index 201356faa7e6..20ec9534e01f 100644 > --- a/arch/xtensa/kernel/syscall.c > +++ b/arch/xtensa/kernel/syscall.c > @@ -58,6 +58,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, > unsigned long len, unsigned long pgoff, unsigned long flags) > { > struct vm_area_struct *vmm; > + VMA_ITERATOR(vmi, mm, addr) Need to use current->mm or it won't compile, AFAICS. ; > > if (flags & MAP_FIXED) { > /* We do not accept a shared mapping if it would violate > @@ -79,7 +80,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, > else > addr = PAGE_ALIGN(addr); > > - for (vmm = find_vma(current->mm, addr); ; vmm = vmm->vm_next) { > + for_each_vma(vmi, vmm) { > /* At this point: (!vmm || addr < vmm->vm_end). */ Well if at this point !vmm then we are no longer here due to for_each_vma(). > if (TASK_SIZE - len < addr) > return -ENOMEM; Thus we can miss this? But maybe it could be moved above the for loop and checked just once, as addr only grows inside the for loop? However, the loop body continues: > if (!vmm || addr + len <= vm_start_gap(vmm)) > return addr; So after your patch we fail to return the unmapped area after the last vma.
* Vlastimil Babka <vbabka@suse.cz> [220118 07:23]: > On 12/1/21 15:30, Liam Howlett wrote: > > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com> > > > > Use the VMA iterator instead. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > > --- > > arch/xtensa/kernel/syscall.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c > > index 201356faa7e6..20ec9534e01f 100644 > > --- a/arch/xtensa/kernel/syscall.c > > +++ b/arch/xtensa/kernel/syscall.c > > @@ -58,6 +58,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, > > unsigned long len, unsigned long pgoff, unsigned long flags) > > { > > struct vm_area_struct *vmm; > > + VMA_ITERATOR(vmi, mm, addr) > > Need to use current->mm or it won't compile, AFAICS. I will fix this. > > ; > > > > if (flags & MAP_FIXED) { > > /* We do not accept a shared mapping if it would violate > > @@ -79,7 +80,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, > > else > > addr = PAGE_ALIGN(addr); > > > > - for (vmm = find_vma(current->mm, addr); ; vmm = vmm->vm_next) { > > + for_each_vma(vmi, vmm) { > > /* At this point: (!vmm || addr < vmm->vm_end). */ > > Well if at this point !vmm then we are no longer here due to for_each_vma(). > > > if (TASK_SIZE - len < addr) > > return -ENOMEM; > > Thus we can miss this? But maybe it could be moved above the for loop and > checked just once, as addr only grows inside the for loop? Yes, it could be missed. We could move it below, but not before as it is to detect the VMA overrunning TASK_SIZE. The check below will need to break. I think it's rare enough that it's okay to run slightly longer when returning -ENOMEM. > > However, the loop body continues: > > > if (!vmm || addr + len <= vm_start_gap(vmm)) > > return addr; > > So after your patch we fail to return the unmapped area after the last vma. I will restructure the loop to avoid this issue.
diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c index 201356faa7e6..20ec9534e01f 100644 --- a/arch/xtensa/kernel/syscall.c +++ b/arch/xtensa/kernel/syscall.c @@ -58,6 +58,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags) { struct vm_area_struct *vmm; + VMA_ITERATOR(vmi, mm, addr); if (flags & MAP_FIXED) { /* We do not accept a shared mapping if it would violate @@ -79,7 +80,7 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr, else addr = PAGE_ALIGN(addr); - for (vmm = find_vma(current->mm, addr); ; vmm = vmm->vm_next) { + for_each_vma(vmi, vmm) { /* At this point: (!vmm || addr < vmm->vm_end). */ if (TASK_SIZE - len < addr) return -ENOMEM;