| Message ID | 20250410035717.473207-2-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: Minor fix, cleanup and improvements | expand |
On Thu, Apr 10, 2025 at 11:57:14AM +0800, Baoquan He wrote: > Not like fault_in_readable() or fault_in_writeable(), in > fault_in_safe_writeable() local variable 'start' is increased page > by page to loop till the whole address range is handled. However, > it mistakenly calcalates the size of handled range with 'uaddr - start'. > > Fix it here. > > Signed-off-by: Baoquan He <bhe@redhat.com> > Fixes: fe673d3f5bf1 ("mm: gup: make fault_in_safe_writeable() use fixup_user_fault()") Reviewed-by: Oscar Salvador <osalvador@suse.de> > --- > mm/gup.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 92351e2fa876..84461d384ae2 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2207,8 +2207,8 @@ size_t fault_in_safe_writeable(const char __user *uaddr, size_t size) > } while (start != end); > mmap_read_unlock(mm); > > - if (size > (unsigned long)uaddr - start) > - return size - ((unsigned long)uaddr - start); > + if (size > start - (unsigned long)uaddr) > + return size - (start - (unsigned long)uaddr); > return 0; > } > EXPORT_SYMBOL(fault_in_safe_writeable); > -- > 2.41.0 >
On Thu, 10 Apr 2025 11:57:14 +0800 Baoquan He <bhe@redhat.com> wrote: > Not like fault_in_readable() or fault_in_writeable(), in > fault_in_safe_writeable() local variable 'start' is increased page > by page to loop till the whole address range is handled. However, > it mistakenly calcalates the size of handled range with 'uaddr - start'. What are the userspace-visible runtime effects of this change?
On 04/10/25 at 08:43pm, Andrew Morton wrote: > On Thu, 10 Apr 2025 11:57:14 +0800 Baoquan He <bhe@redhat.com> wrote: > > > Not like fault_in_readable() or fault_in_writeable(), in > > fault_in_safe_writeable() local variable 'start' is increased page > > by page to loop till the whole address range is handled. However, > > it mistakenly calcalates the size of handled range with 'uaddr - start'. > > What are the userspace-visible runtime effects of this change? I see it mainly affect gfs2_file_direct_read(). Not sure if GFS2 people can sense any exceptional behaviour caused by this code bug. >
On 10.04.25 05:57, Baoquan He wrote: > Not like fault_in_readable() or fault_in_writeable(), in > fault_in_safe_writeable() local variable 'start' is increased page > by page to loop till the whole address range is handled. However, > it mistakenly calcalates the size of handled range with 'uaddr - start'. > > Fix it here. > > Signed-off-by: Baoquan He <bhe@redhat.com> > Fixes: fe673d3f5bf1 ("mm: gup: make fault_in_safe_writeable() use fixup_user_fault()") > --- > mm/gup.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 92351e2fa876..84461d384ae2 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2207,8 +2207,8 @@ size_t fault_in_safe_writeable(const char __user *uaddr, size_t size) > } while (start != end); > mmap_read_unlock(mm); > > - if (size > (unsigned long)uaddr - start) > - return size - ((unsigned long)uaddr - start); > + if (size > start - (unsigned long)uaddr) > + return size - (start - (unsigned long)uaddr); > return 0; > } > EXPORT_SYMBOL(fault_in_safe_writeable); Acked-by: David Hildenbrand <david@redhat.com>
On Fri, Apr 11, 2025 at 7:32 AM Baoquan He <bhe@redhat.com> wrote: > On 04/10/25 at 08:43pm, Andrew Morton wrote: > > On Thu, 10 Apr 2025 11:57:14 +0800 Baoquan He <bhe@redhat.com> wrote: > > > > > Not like fault_in_readable() or fault_in_writeable(), in > > > fault_in_safe_writeable() local variable 'start' is increased page > > > by page to loop till the whole address range is handled. However, > > > it mistakenly calcalates the size of handled range with 'uaddr - start'. > > > > What are the userspace-visible runtime effects of this change? > > I see it mainly affect gfs2_file_direct_read(). Not sure if GFS2 people > can sense any exceptional behaviour caused by this code bug. Thanks for the heads up. In gfs2, fault_in_iov_iter_writeable() is used in gfs2_file_direct_read() and gfs2_file_read_iter(), so this potentially affects buffered as well as direct reads. This bug could cause those gfs2 functions to spin in a loop. Can this fix please be sent to Linus for inclusion into 6.15? Thanks, Andreas
On Fri, 11 Apr 2025 17:07:32 +0200 Andreas Gruenbacher <agruenba@redhat.com> wrote:
> Can this fix please be sent to Linus for inclusion into 6.15?
Sure, I made the adjustments.
diff --git a/mm/gup.c b/mm/gup.c index 92351e2fa876..84461d384ae2 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2207,8 +2207,8 @@ size_t fault_in_safe_writeable(const char __user *uaddr, size_t size) } while (start != end); mmap_read_unlock(mm); - if (size > (unsigned long)uaddr - start) - return size - ((unsigned long)uaddr - start); + if (size > start - (unsigned long)uaddr) + return size - (start - (unsigned long)uaddr); return 0; } EXPORT_SYMBOL(fault_in_safe_writeable);
Not like fault_in_readable() or fault_in_writeable(), in fault_in_safe_writeable() local variable 'start' is increased page by page to loop till the whole address range is handled. However, it mistakenly calcalates the size of handled range with 'uaddr - start'. Fix it here. Signed-off-by: Baoquan He <bhe@redhat.com> Fixes: fe673d3f5bf1 ("mm: gup: make fault_in_safe_writeable() use fixup_user_fault()") --- mm/gup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)