| Message ID | 1567592763-25282-1-git-send-email-zhongjiang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Unsigned 'nr_pages' always larger than zero | expand |
On 9/4/19 12:26 PM, zhong jiang wrote: > With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' > compare with zero. And __get_user_pages_locked will return an long value. > Hence, Convert the long to compare with zero is feasible. It would be nicer if the parameter nr_pages was long again instead of unsigned long (note there are two variants of the function, so both should be changed). > Signed-off-by: zhong jiang <zhongjiang@huawei.com> Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with FOLL_LONGTERM") (which changed long to unsigned long) AFAICS... stable shouldn't be needed as the only "risk" is that we goto check_again even when we fail, which should be harmless. Vlastimil > --- > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 23a9f9c..956d5a1 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1508,7 +1508,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk, > pages, vmas, NULL, > gup_flags); > > - if ((nr_pages > 0) && migrate_allow) { > + if (((long)nr_pages > 0) && migrate_allow) { > drain_allow = true; > goto check_again; > } >
> On 9/4/19 12:26 PM, zhong jiang wrote: > > With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' > > compare with zero. And __get_user_pages_locked will return an long > value. > > Hence, Convert the long to compare with zero is feasible. > > It would be nicer if the parameter nr_pages was long again instead of > unsigned long (note there are two variants of the function, so both should be > changed). Why? What does it mean for nr_pages to be negative? The check below seems valid. Unsigned can be 0 so the check can fail. IOW Checking unsigned > 0 seems ok. What am I missing? Ira > > > Signed-off-by: zhong jiang <zhongjiang@huawei.com> > > Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with > FOLL_LONGTERM") > > (which changed long to unsigned long) > > AFAICS... stable shouldn't be needed as the only "risk" is that we goto > check_again even when we fail, which should be harmless. > > Vlastimil > > > --- > > mm/gup.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index 23a9f9c..956d5a1 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -1508,7 +1508,7 @@ static long check_and_migrate_cma_pages(struct > task_struct *tsk, > > pages, vmas, NULL, > > gup_flags); > > > > - if ((nr_pages > 0) && migrate_allow) { > > + if (((long)nr_pages > 0) && migrate_allow) { > > drain_allow = true; > > goto check_again; > > } > >
On Wed, Sep 04, 2019 at 06:25:19PM +0000, Weiny, Ira wrote: > > On 9/4/19 12:26 PM, zhong jiang wrote: > > > With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' > > > compare with zero. And __get_user_pages_locked will return an long > > value. > > > Hence, Convert the long to compare with zero is feasible. > > > > It would be nicer if the parameter nr_pages was long again instead of > > unsigned long (note there are two variants of the function, so both should be > > changed). > > Why? What does it mean for nr_pages to be negative? The check below seems valid. Unsigned can be 0 so the check can fail. IOW Checking unsigned > 0 seems ok. > > What am I missing? __get_user_pages can return a negative errno.
On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > On 9/4/19 12:26 PM, zhong jiang wrote: > > With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' > > compare with zero. And __get_user_pages_locked will return an long value. > > Hence, Convert the long to compare with zero is feasible. > > It would be nicer if the parameter nr_pages was long again instead of unsigned > long (note there are two variants of the function, so both should be changed). nr_pages should be unsigned - it's a count of pages! The bug is that __get_user_pages_locked() returns a signed long which can be a -ve errno. I think it's best if __get_user_pages_locked() is to get itself a new local with the same type as its return value. Something like: --- a/mm/gup.c~a +++ a/mm/gup.c @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( bool drain_allow = true; bool migrate_allow = true; LIST_HEAD(cma_page_list); + long ret; check_again: for (i = 0; i < nr_pages;) { @@ -1511,17 +1512,18 @@ check_again: * again migrating any new CMA pages which we failed to isolate * earlier. */ - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas, NULL, gup_flags); - if ((nr_pages > 0) && migrate_allow) { + nr_pages = ret; + if (ret > 0 && migrate_allow) { drain_allow = true; goto check_again; } } - return nr_pages; + return ret; } #else static long check_and_migrate_cma_pages(struct task_struct *tsk,
On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > On 9/4/19 12:26 PM, zhong jiang wrote: > > With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' > > compare with zero. And __get_user_pages_locked will return an long value. > > Hence, Convert the long to compare with zero is feasible. > > It would be nicer if the parameter nr_pages was long again instead of unsigned > long (note there are two variants of the function, so both should be changed). > > > Signed-off-by: zhong jiang <zhongjiang@huawei.com> > > Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with FOLL_LONGTERM") > > (which changed long to unsigned long) > > AFAICS... stable shouldn't be needed as the only "risk" is that we goto > check_again even when we fail, which should be harmless. > Really? If nr_pages gets a value of -EFAULT from the __get_user_pages_locked() call, check_and_migrate_cma_pages() will go berzerk? And does __get_user_pages_locked() correctly handle a -ve errno returned by __get_user_pages()? It's hard to see how...
> Subject: Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero > > On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> > wrote: > > > On 9/4/19 12:26 PM, zhong jiang wrote: > > > With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' > > > compare with zero. And __get_user_pages_locked will return an long > value. > > > Hence, Convert the long to compare with zero is feasible. > > > > It would be nicer if the parameter nr_pages was long again instead of > > unsigned long (note there are two variants of the function, so both should > be changed). > > nr_pages should be unsigned - it's a count of pages! > > The bug is that __get_user_pages_locked() returns a signed long which can > be a -ve errno. Ok... This is my bad... I think this is the correct fix though. Not changing the type of nr_pages. Sorry, Ira > > I think it's best if __get_user_pages_locked() is to get itself a new local with > the same type as its return value. Something like: > > --- a/mm/gup.c~a > +++ a/mm/gup.c > @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( > bool drain_allow = true; > bool migrate_allow = true; > LIST_HEAD(cma_page_list); > + long ret; > > check_again: > for (i = 0; i < nr_pages;) { > @@ -1511,17 +1512,18 @@ check_again: > * again migrating any new CMA pages which we failed to > isolate > * earlier. > */ > - nr_pages = __get_user_pages_locked(tsk, mm, start, > nr_pages, > + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, > pages, vmas, NULL, > gup_flags); > > - if ((nr_pages > 0) && migrate_allow) { > + nr_pages = ret; > + if (ret > 0 && migrate_allow) { > drain_allow = true; > goto check_again; > } > } > > - return nr_pages; > + return ret; > } > #else > static long check_and_migrate_cma_pages(struct task_struct *tsk, > >
On 9/4/19 8:48 PM, Andrew Morton wrote: > On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 9/4/19 12:26 PM, zhong jiang wrote: >>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' >>> compare with zero. And __get_user_pages_locked will return an long value. >>> Hence, Convert the long to compare with zero is feasible. >> >> It would be nicer if the parameter nr_pages was long again instead of unsigned >> long (note there are two variants of the function, so both should be changed). > > nr_pages should be unsigned - it's a count of pages! Hm right, I thought check_and_migrate_cma_pages() could be already called with negative nr_pages from __gup_longterm_locked(), but I missed there's a if (rc < 0) goto out before that. > The bug is that __get_user_pages_locked() returns a signed long which > can be a -ve errno. > > I think it's best if __get_user_pages_locked() is to get itself a new You mean check_and_migrate_cma_pages() > local with the same type as its return value. Something like: Agreed. > --- a/mm/gup.c~a > +++ a/mm/gup.c > @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( > bool drain_allow = true; > bool migrate_allow = true; > LIST_HEAD(cma_page_list); > + long ret; Should be initialized to nr_pages in case we don't go via "if (!list_empty(&cma_page_list))" at all. > > check_again: > for (i = 0; i < nr_pages;) { > @@ -1511,17 +1512,18 @@ check_again: > * again migrating any new CMA pages which we failed to isolate > * earlier. > */ > - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, > + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, > pages, vmas, NULL, > gup_flags); > > - if ((nr_pages > 0) && migrate_allow) { > + nr_pages = ret; > + if (ret > 0 && migrate_allow) { > drain_allow = true; > goto check_again; > } > } > > - return nr_pages; > + return ret; > } > #else > static long check_and_migrate_cma_pages(struct task_struct *tsk, > >
On 9/4/19 9:01 PM, Andrew Morton wrote: > On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 9/4/19 12:26 PM, zhong jiang wrote: >>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' >>> compare with zero. And __get_user_pages_locked will return an long value. >>> Hence, Convert the long to compare with zero is feasible. >> >> It would be nicer if the parameter nr_pages was long again instead of unsigned >> long (note there are two variants of the function, so both should be changed). >> >>> Signed-off-by: zhong jiang <zhongjiang@huawei.com> >> >> Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with FOLL_LONGTERM") >> >> (which changed long to unsigned long) >> >> AFAICS... stable shouldn't be needed as the only "risk" is that we goto >> check_again even when we fail, which should be harmless. >> > > Really? If nr_pages gets a value of -EFAULT from the > __get_user_pages_locked() call, check_and_migrate_cma_pages() will go > berzerk? Hmm it should only reach that goto when it migrated something, which means it won't have to be migrated again, so eventually it should terminate. But it's very subtle, so I might be wrong. > And does __get_user_pages_locked() correctly handle a -ve errno > returned by __get_user_pages()? It's hard to see how... I think it does, but agree.
On 2019/9/4 19:24, Vlastimil Babka wrote: > On 9/4/19 12:26 PM, zhong jiang wrote: >> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' >> compare with zero. And __get_user_pages_locked will return an long value. >> Hence, Convert the long to compare with zero is feasible. > It would be nicer if the parameter nr_pages was long again instead of unsigned > long (note there are two variants of the function, so both should be changed). Yep, the parameter 'nr_pages' was changed to long. and the variants ‘i、step’ should be changed accordingly. >> Signed-off-by: zhong jiang <zhongjiang@huawei.com> > Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with FOLL_LONGTERM") > > (which changed long to unsigned long) > > AFAICS... stable shouldn't be needed as the only "risk" is that we goto > check_again even when we fail, which should be harmless. Agreed, Thanks. Sincerely, zhong jiang > Vlastimil > >> --- >> mm/gup.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/gup.c b/mm/gup.c >> index 23a9f9c..956d5a1 100644 >> --- a/mm/gup.c >> +++ b/mm/gup.c >> @@ -1508,7 +1508,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk, >> pages, vmas, NULL, >> gup_flags); >> >> - if ((nr_pages > 0) && migrate_allow) { >> + if (((long)nr_pages > 0) && migrate_allow) { >> drain_allow = true; >> goto check_again; >> } >> > > . >
On 2019/9/5 2:48, Andrew Morton wrote: > On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 9/4/19 12:26 PM, zhong jiang wrote: >>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' >>> compare with zero. And __get_user_pages_locked will return an long value. >>> Hence, Convert the long to compare with zero is feasible. >> It would be nicer if the parameter nr_pages was long again instead of unsigned >> long (note there are two variants of the function, so both should be changed). > nr_pages should be unsigned - it's a count of pages! > > The bug is that __get_user_pages_locked() returns a signed long which > can be a -ve errno. > > I think it's best if __get_user_pages_locked() is to get itself a new > local with the same type as its return value. Something like: > > --- a/mm/gup.c~a > +++ a/mm/gup.c > @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( > bool drain_allow = true; > bool migrate_allow = true; > LIST_HEAD(cma_page_list); > + long ret; > > check_again: > for (i = 0; i < nr_pages;) { > @@ -1511,17 +1512,18 @@ check_again: > * again migrating any new CMA pages which we failed to isolate > * earlier. > */ > - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, > + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, > pages, vmas, NULL, > gup_flags); > > - if ((nr_pages > 0) && migrate_allow) { > + nr_pages = ret; > + if (ret > 0 && migrate_allow) { > drain_allow = true; > goto check_again; > } > } > > - return nr_pages; > + return ret; > } > #else > static long check_and_migrate_cma_pages(struct task_struct *tsk, Firstly, I consider the some modified method as you has writen down above. It seems to work well. According to Vlastimil's feedback, I repost the patch in v2, changing the parameter to long to fix the issue. which one do you prefer? Thanks, zhong jiang
On 9/4/19 11:48 AM, Andrew Morton wrote: > On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 9/4/19 12:26 PM, zhong jiang wrote: >>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' >>> compare with zero. And __get_user_pages_locked will return an long value. >>> Hence, Convert the long to compare with zero is feasible. >> >> It would be nicer if the parameter nr_pages was long again instead of unsigned >> long (note there are two variants of the function, so both should be changed). > > nr_pages should be unsigned - it's a count of pages! > Yes! > The bug is that __get_user_pages_locked() returns a signed long which > can be a -ve errno. > > I think it's best if __get_user_pages_locked() is to get itself a new > local with the same type as its return value. Something like: > > --- a/mm/gup.c~a > +++ a/mm/gup.c > @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( > bool drain_allow = true; > bool migrate_allow = true; > LIST_HEAD(cma_page_list); > + long ret; > > check_again: > for (i = 0; i < nr_pages;) { > @@ -1511,17 +1512,18 @@ check_again: > * again migrating any new CMA pages which we failed to isolate > * earlier. > */ > - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, > + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, > pages, vmas, NULL, > gup_flags); > > - if ((nr_pages > 0) && migrate_allow) { > + nr_pages = ret; > + if (ret > 0 && migrate_allow) { > drain_allow = true; > goto check_again; > } > } > > - return nr_pages; > + return ret; > } > #else > static long check_and_migrate_cma_pages(struct task_struct *tsk, > +1 for this approach, please. thanks,
On 9/5/19 8:18 AM, zhong jiang wrote: > On 2019/9/5 2:48, Andrew Morton wrote: > Firstly, I consider the some modified method as you has writen down above. It seems to work well. > According to Vlastimil's feedback, I repost the patch in v2, changing the parameter to long to fix > the issue. which one do you prefer? Please forget about my suggestion to change parameter to long, it was wrong. New variable is better. > Thanks, > zhong jiang >
On 2019/9/5 14:19, John Hubbard wrote: > On 9/4/19 11:48 AM, Andrew Morton wrote: >> On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka <vbabka@suse.cz> wrote: >> >>> On 9/4/19 12:26 PM, zhong jiang wrote: >>>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' >>>> compare with zero. And __get_user_pages_locked will return an long value. >>>> Hence, Convert the long to compare with zero is feasible. >>> >>> It would be nicer if the parameter nr_pages was long again instead of unsigned >>> long (note there are two variants of the function, so both should be changed). >> >> nr_pages should be unsigned - it's a count of pages! >> > > Yes! > >> The bug is that __get_user_pages_locked() returns a signed long which >> can be a -ve errno. >> >> I think it's best if __get_user_pages_locked() is to get itself a new >> local with the same type as its return value. Something like: >> >> --- a/mm/gup.c~a >> +++ a/mm/gup.c >> @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( >> bool drain_allow = true; >> bool migrate_allow = true; >> LIST_HEAD(cma_page_list); >> + long ret; >> check_again: >> for (i = 0; i < nr_pages;) { >> @@ -1511,17 +1512,18 @@ check_again: >> * again migrating any new CMA pages which we failed to isolate >> * earlier. >> */ >> - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, >> + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, >> pages, vmas, NULL, >> gup_flags); >> - if ((nr_pages > 0) && migrate_allow) { >> + nr_pages = ret; >> + if (ret > 0 && migrate_allow) { >> drain_allow = true; >> goto check_again; >> } >> } >> - return nr_pages; >> + return ret; >> } >> #else >> static long check_and_migrate_cma_pages(struct task_struct *tsk, >> > > +1 for this approach, please. > > > thanks, Hi, Andrew I didn't see the fix for the issue in the upstream. Your proposal should be appiled to upstream. Could you appiled the patch or repost by me ? Thanks, zhogn jiang
On Wed, 16 Oct 2019 17:07:44 +0800 zhong jiang <zhongjiang@huawei.com> wrote: > >> --- a/mm/gup.c~a > >> +++ a/mm/gup.c > >> @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( > >> bool drain_allow = true; > >> bool migrate_allow = true; > >> LIST_HEAD(cma_page_list); > >> + long ret; > >> check_again: > >> for (i = 0; i < nr_pages;) { > >> @@ -1511,17 +1512,18 @@ check_again: > >> * again migrating any new CMA pages which we failed to isolate > >> * earlier. > >> */ > >> - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, > >> + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, > >> pages, vmas, NULL, > >> gup_flags); > >> - if ((nr_pages > 0) && migrate_allow) { > >> + nr_pages = ret; > >> + if (ret > 0 && migrate_allow) { > >> drain_allow = true; > >> goto check_again; > >> } > >> } > >> - return nr_pages; > >> + return ret; > >> } > >> #else > >> static long check_and_migrate_cma_pages(struct task_struct *tsk, > >> > > > > +1 for this approach, please. > > > > > > thanks, > Hi, Andrew > > I didn't see the fix for the issue in the upstream. Your proposal should be > appiled to upstream. Could you appiled the patch or repost by me ? Forgotten about it ;) Please send a patch sometime?
On 2019/10/17 8:49, Andrew Morton wrote: > On Wed, 16 Oct 2019 17:07:44 +0800 zhong jiang <zhongjiang@huawei.com> wrote: > >>>> --- a/mm/gup.c~a >>>> +++ a/mm/gup.c >>>> @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages( >>>> bool drain_allow = true; >>>> bool migrate_allow = true; >>>> LIST_HEAD(cma_page_list); >>>> + long ret; >>>> check_again: >>>> for (i = 0; i < nr_pages;) { >>>> @@ -1511,17 +1512,18 @@ check_again: >>>> * again migrating any new CMA pages which we failed to isolate >>>> * earlier. >>>> */ >>>> - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, >>>> + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, >>>> pages, vmas, NULL, >>>> gup_flags); >>>> - if ((nr_pages > 0) && migrate_allow) { >>>> + nr_pages = ret; >>>> + if (ret > 0 && migrate_allow) { >>>> drain_allow = true; >>>> goto check_again; >>>> } >>>> } >>>> - return nr_pages; >>>> + return ret; >>>> } >>>> #else >>>> static long check_and_migrate_cma_pages(struct task_struct *tsk, >>>> >>> +1 for this approach, please. >>> >>> >>> thanks, >> Hi, Andrew >> >> I didn't see the fix for the issue in the upstream. Your proposal should be >> appiled to upstream. Could you appiled the patch or repost by me ? > Forgotten about it ;) Please send a patch sometime? > > . > I will repost the patch as your suggestion. Thanks, Sincerely, zhong jiang
diff --git a/mm/gup.c b/mm/gup.c index 23a9f9c..956d5a1 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1508,7 +1508,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk, pages, vmas, NULL, gup_flags); - if ((nr_pages > 0) && migrate_allow) { + if (((long)nr_pages > 0) && migrate_allow) { drain_allow = true; goto check_again; }
With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' compare with zero. And __get_user_pages_locked will return an long value. Hence, Convert the long to compare with zero is feasible. Signed-off-by: zhong jiang <zhongjiang@huawei.com> --- mm/gup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)