| Message ID | 20221024172139.80435-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/userptr: restore probe_range behaviour | expand |
On 24/10/2022 18:21, Matthew Auld wrote: > The conversion looks harmless, however the addr value is updated inside > the loop with the previous vm_end, which then incorrectly leads to > for_each_vma_range() iterating over stuff outside the range we care > about. Fix this by storing the end value separately. > > Testcase: igt@gem_userptr_blits@probe > Fixes: f683b9d61319 ("i915: use the VMA iterator") > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Matthew Wilcox (Oracle) <willy@infradead.org> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Yu Zhao <yuzhao@google.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index b7e24476a0fd..dadb3e3fa9c8 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -427,9 +427,10 @@ probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > { > VMA_ITERATOR(vmi, mm, addr); > struct vm_area_struct *vma; > + unsigned long end = addr + len; > > mmap_read_lock(mm); > - for_each_vma_range(vmi, vma, addr + len) { > + for_each_vma_range(vmi, vma, end) { > /* Check for holes, note that we also update the addr below */ > if (vma->vm_start > addr) > break; I am unsure of the for_each_vma_range() behaviour regarding holes. If it just skips overs them and continues to next VMA in the range then patch looks good to me. Could someone confirm? Regards, Tvrtko
On Tue, Oct 25, 2022 at 04:40:23PM +0100, Tvrtko Ursulin wrote: > > On 24/10/2022 18:21, Matthew Auld wrote: > > The conversion looks harmless, however the addr value is updated inside > > the loop with the previous vm_end, which then incorrectly leads to > > for_each_vma_range() iterating over stuff outside the range we care > > about. Fix this by storing the end value separately. > > > > Testcase: igt@gem_userptr_blits@probe > > Fixes: f683b9d61319 ("i915: use the VMA iterator") > > Reported-by: kernel test robot <oliver.sang@intel.com> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: Matthew Wilcox (Oracle) <willy@infradead.org> > > Cc: Vlastimil Babka <vbabka@suse.cz> > > Cc: Yu Zhao <yuzhao@google.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > index b7e24476a0fd..dadb3e3fa9c8 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -427,9 +427,10 @@ probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > > { > > VMA_ITERATOR(vmi, mm, addr); > > struct vm_area_struct *vma; > > + unsigned long end = addr + len; > > mmap_read_lock(mm); > > - for_each_vma_range(vmi, vma, addr + len) { > > + for_each_vma_range(vmi, vma, end) { > > /* Check for holes, note that we also update the addr below */ > > if (vma->vm_start > addr) > > break; > > I am unsure of the for_each_vma_range() behaviour regarding holes. If it > just skips overs them and continues to next VMA in the range then patch > looks good to me. Could someone confirm? It's "For each VMA in this range". It doesn't iterate over non-VMAs within that range ;-) Nor does a gap between VMAs stop the iteration.
On 25/10/2022 16:54, Matthew Wilcox wrote: > On Tue, Oct 25, 2022 at 04:40:23PM +0100, Tvrtko Ursulin wrote: >> >> On 24/10/2022 18:21, Matthew Auld wrote: >>> The conversion looks harmless, however the addr value is updated inside >>> the loop with the previous vm_end, which then incorrectly leads to >>> for_each_vma_range() iterating over stuff outside the range we care >>> about. Fix this by storing the end value separately. >>> >>> Testcase: igt@gem_userptr_blits@probe >>> Fixes: f683b9d61319 ("i915: use the VMA iterator") >>> Reported-by: kernel test robot <oliver.sang@intel.com> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>> Cc: Matthew Wilcox (Oracle) <willy@infradead.org> >>> Cc: Vlastimil Babka <vbabka@suse.cz> >>> Cc: Yu Zhao <yuzhao@google.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> index b7e24476a0fd..dadb3e3fa9c8 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> @@ -427,9 +427,10 @@ probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) >>> { >>> VMA_ITERATOR(vmi, mm, addr); >>> struct vm_area_struct *vma; >>> + unsigned long end = addr + len; >>> mmap_read_lock(mm); >>> - for_each_vma_range(vmi, vma, addr + len) { >>> + for_each_vma_range(vmi, vma, end) { >>> /* Check for holes, note that we also update the addr below */ >>> if (vma->vm_start > addr) >>> break; >> >> I am unsure of the for_each_vma_range() behaviour regarding holes. If it >> just skips overs them and continues to next VMA in the range then patch >> looks good to me. Could someone confirm? > > It's "For each VMA in this range". It doesn't iterate over non-VMAs > within that range ;-) Nor does a gap between VMAs stop the iteration. Thank you Matthew - I ventured briefly into the maple leaf world but quickly decided it would be much easier to ask. :) Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index b7e24476a0fd..dadb3e3fa9c8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -427,9 +427,10 @@ probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) { VMA_ITERATOR(vmi, mm, addr); struct vm_area_struct *vma; + unsigned long end = addr + len; mmap_read_lock(mm); - for_each_vma_range(vmi, vma, addr + len) { + for_each_vma_range(vmi, vma, end) { /* Check for holes, note that we also update the addr below */ if (vma->vm_start > addr) break;
The conversion looks harmless, however the addr value is updated inside the loop with the previous vm_end, which then incorrectly leads to for_each_vma_range() iterating over stuff outside the range we care about. Fix this by storing the end value separately. Testcase: igt@gem_userptr_blits@probe Fixes: f683b9d61319 ("i915: use the VMA iterator") Reported-by: kernel test robot <oliver.sang@intel.com> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Matthew Wilcox (Oracle) <willy@infradead.org> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Yu Zhao <yuzhao@google.com> --- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)