| Message ID | 20250331081327.256412-2-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: Minor fix, cleanup and improvements | expand |
On 31.03.25 10:13, 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. Fixes: ? Do we know of user-visible effects? > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > v1->v2: > - Fix a patch log typo caused by copy-and-paste error. Thanks > to Yanjun. > > mm/gup.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 855ab860f88b..73777b1de679 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); Can we instead just use the uaddr and start variables like in fault_in_readable? That is, turn "start" into a const and adjust uaddr instead. (maybe we should also handle the types of these variables similar to as in fault_in_readable)
On Tue, Apr 01, 2025 at 10:10:03AM +0200, David Hildenbrand wrote: > On 31.03.25 10:13, Baoquan He wrote: > > --- 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); > > Can we instead just use the uaddr and start variables like in > fault_in_readable? > > That is, turn "start" into a const and adjust uaddr instead. Yes, I think that would be much cleaner. Otherwise, this looks good to me.
On 04/01/25 at 10:10am, David Hildenbrand wrote: > On 31.03.25 10:13, 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. > > Fixes: ? Do we know of user-visible effects? I believe it should impact, while I didn't hear of any complaint. Yeah, it's worth to have one Fixes. > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > --- > > v1->v2: > > - Fix a patch log typo caused by copy-and-paste error. Thanks > > to Yanjun. > > > > mm/gup.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index 855ab860f88b..73777b1de679 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); > > Can we instead just use the uaddr and start variables like in > fault_in_readable? > > That is, turn "start" into a const and adjust uaddr instead. Sounds good to me, that makes these similar blocks own consistent code style. Will change in v3. Thanks for reviewing. > > (maybe we should also handle the types of these variables similar to as in > fault_in_readable) > > -- > Cheers, > > David / dhildenb >
On 04/01/25 at 04:00pm, Oscar Salvador wrote: > On Tue, Apr 01, 2025 at 10:10:03AM +0200, David Hildenbrand wrote: > > On 31.03.25 10:13, Baoquan He wrote: > > > --- 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); > > > > Can we instead just use the uaddr and start variables like in > > fault_in_readable? > > > > That is, turn "start" into a const and adjust uaddr instead. > > Yes, I think that would be much cleaner. > > Otherwise, this looks good to me. Will change in v3 as both of you suggested, thanks for reviewing this v2 series.
diff --git a/mm/gup.c b/mm/gup.c index 855ab860f88b..73777b1de679 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> --- v1->v2: - Fix a patch log typo caused by copy-and-paste error. Thanks to Yanjun. mm/gup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)