[PATCH] mm: Unsigned 'nr_pages' always larger than zero

[PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

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(-)

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;
 		}
-- 
1.7.12.4

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Vlastimil Babka]

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;
>  		}
> 

RE: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Weiny, Ira]

> 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;
> >  		}
> >

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Matthew Wilcox]

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.

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Andrew Morton]

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,

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Andrew Morton]

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...

RE: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Weiny, Ira]

> 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,
> 
> 

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Vlastimil Babka]

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,
> 
> 

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Vlastimil Babka]

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.

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

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;
>>  		}
>>
>
> .
>

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

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

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA
  

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Vlastimil Babka]

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
> 

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[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

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Andrew Morton]

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?

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

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

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/10/19 0:07, Alexander Duyck wrote:
> On Fri, Oct 18, 2019 at 7:00 AM zhong jiang <zhongjiang@huawei.com> wrote:
>> On 2019/10/18 4:42, John Hubbard wrote:
>>> On 10/16/19 8:19 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.
>>>>
>>>> The patch use a new local variable to store the return value of
>>>> __get_user_pages_locked(). Then use it to compare with zero.
>>> Hi Zhong,
>>>
>>> The above are actually more like notes to yourself, and while those are
>>> good to have, but it's not yet a perfect commit description: it talks
>>> about how you got here, which is only part of the story. A higher level
>>> summary is better, and let the code itself cover the details.
>>>
>>> Like this:
>>>
>>> First line (subject):
>>>
>>>    mm/gup: allow CMA migration to propagate errors back to caller
>>>
>>> Commit description:
>>>
>>> check_and_migrate_cma_pages() was recording the result of
>>> __get_user_pages_locked() in an unsigned "nr_pages" variable. Because
>>> __get_user_pages_locked() returns a signed value that can include
>>> negative errno values, this had the effect of hiding errors.
>>>
>>> Change check_and_migrate_cma_pages() implementation so that it
>>> uses a signed variable instead, and propagates the results back
>>> to the caller just as other gup internal functions do.
>>>
>>> This was discovered with the help of unsigned_lesser_than_zero.cocci.
>>>
>>>> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
>>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>>> ---
>>>>  mm/gup.c | 8 +++++---
>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>> index 8f236a3..1fe0ceb 100644
>>>> --- a/mm/gup.c
>>>> +++ b/mm/gup.c
>>>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>>>      bool drain_allow = true;
>>>>      bool migrate_allow = true;
>>>>      LIST_HEAD(cma_page_list);
>>>> +    long ret;
>>> Ira pointed out that this needs initialization, see below.
>>>
>>>>  check_again:
>>>>      for (i = 0; i < nr_pages;) {
>>>> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>>>               * 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;
>>> Although technically correct, it makes me feel odd to see the assignment
>>> done from signed to unsigned, *before* checking for >0. And Ira is hinting
>>> at the same thing, when he asks if we can return early here. See below...
>>>
>>>> +            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,
>>>>
>>> So, overall, I'd recommend this, instead:
>>>
>>> diff --git a/mm/gup.c b/mm/gup.c
>>> index 23a9f9c9d377..72bc027037fa 100644
>>> --- a/mm/gup.c
>>> +++ b/mm/gup.c
>>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>>         bool drain_allow = true;
>>>         bool migrate_allow = true;
>>>         LIST_HEAD(cma_page_list);
>>> +       long ret = nr_pages;
>>>
>> I think the ret should be assigned as zero. we will return ret if cma_page_list is empty.
>> I miss something?
> That wasn't the behavior before. Before if the cma_page_list was empty
> nr_pages would not be modified from what what passed. I believe that
> is the intended behavior for this since the cma_page_list seems like a
> workaround to migrate out any CMA pages if present in the longterm
> mapping. If there are no CMA pages the return result should be
> nr_pages.
Thanks for your clarification.

Sincerely,
zhong jiang
> Thanks.
>
> - Alex
>
>
> .
>

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/10/21 20:57, Vlastimil Babka wrote:
> On 10/17/19 10:42 PM, John Hubbard wrote:
>> On 10/16/19 8:19 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.
>>>
>>> The patch use a new local variable to store the return value of
>>> __get_user_pages_locked(). Then use it to compare with zero.
>> Hi Zhong,
>>
>> The above are actually more like notes to yourself, and while those are
>> good to have, but it's not yet a perfect commit description: it talks 
>> about how you got here, which is only part of the story. A higher level
>> summary is better, and let the code itself cover the details. 
>>
>> Like this:
>>
>> First line (subject): 
>>
>>    mm/gup: allow CMA migration to propagate errors back to caller
>>
>> Commit description:
>>
>> check_and_migrate_cma_pages() was recording the result of 
>> __get_user_pages_locked() in an unsigned "nr_pages" variable. Because
>> __get_user_pages_locked() returns a signed value that can include
>> negative errno values, this had the effect of hiding errors.
>>
>> Change check_and_migrate_cma_pages() implementation so that it
>> uses a signed variable instead, and propagates the results back
>> to the caller just as other gup internal functions do.
>>
>> This was discovered with the help of unsigned_lesser_than_zero.cocci.
> Agreed.
>
>>> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>> ---
>>>  mm/gup.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/gup.c b/mm/gup.c
>>> index 8f236a3..1fe0ceb 100644
>>> --- a/mm/gup.c
>>> +++ b/mm/gup.c
>>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>>  	bool drain_allow = true;
>>>  	bool migrate_allow = true;
>>>  	LIST_HEAD(cma_page_list);
>>> +	long ret;
>> Ira pointed out that this needs initialization, see below.
>>
>>>  
>>>  check_again:
>>>  	for (i = 0; i < nr_pages;) {
>>> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>>  		 * 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;
>> Although technically correct, it makes me feel odd to see the assignment
>> done from signed to unsigned, *before* checking for >0. And Ira is hinting
>> at the same thing, when he asks if we can return early here. See below...
>>
>>> +		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,
>>>
>> So, overall, I'd recommend this, instead:
> Also agreed. Zhong, can you resend it like that?
Ok,  I will resend it right now.  Thanks

Sincerely,
zhong jiang
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 23a9f9c9d377..72bc027037fa 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>         bool drain_allow = true;
>>         bool migrate_allow = true;
>>         LIST_HEAD(cma_page_list);
>> +       long ret = nr_pages;
>>  
>>  check_again:
>>         for (i = 0; i < nr_pages;) {
>> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>                  * 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) {
>> +               if ((ret > 0) && migrate_allow) {
>> +                       nr_pages = ret;
>>                         drain_allow = true;
>>                         goto check_again;
>>                 }
>>         }
>>  
>> -       return nr_pages;
>> +       return ret;
>>  }
>>  #else
>>  static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>
>>
>> thanks,
>>
>> John Hubbard
>> NVIDIA
>>
>
> .
>

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Vlastimil Babka]

On 10/17/19 10:42 PM, John Hubbard wrote:
> On 10/16/19 8:19 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.
>>
>> The patch use a new local variable to store the return value of
>> __get_user_pages_locked(). Then use it to compare with zero.
> 
> Hi Zhong,
> 
> The above are actually more like notes to yourself, and while those are
> good to have, but it's not yet a perfect commit description: it talks 
> about how you got here, which is only part of the story. A higher level
> summary is better, and let the code itself cover the details. 
> 
> Like this:
> 
> First line (subject): 
> 
>    mm/gup: allow CMA migration to propagate errors back to caller
> 
> Commit description:
> 
> check_and_migrate_cma_pages() was recording the result of 
> __get_user_pages_locked() in an unsigned "nr_pages" variable. Because
> __get_user_pages_locked() returns a signed value that can include
> negative errno values, this had the effect of hiding errors.
> 
> Change check_and_migrate_cma_pages() implementation so that it
> uses a signed variable instead, and propagates the results back
> to the caller just as other gup internal functions do.
> 
> This was discovered with the help of unsigned_lesser_than_zero.cocci.

Agreed.

> 
>>
>> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  mm/gup.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 8f236a3..1fe0ceb 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  	bool drain_allow = true;
>>  	bool migrate_allow = true;
>>  	LIST_HEAD(cma_page_list);
>> +	long ret;
> 
> Ira pointed out that this needs initialization, see below.
> 
>>  
>>  check_again:
>>  	for (i = 0; i < nr_pages;) {
>> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  		 * 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;
> 
> Although technically correct, it makes me feel odd to see the assignment
> done from signed to unsigned, *before* checking for >0. And Ira is hinting
> at the same thing, when he asks if we can return early here. See below...
> 
>> +		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,
>>
> 
> So, overall, I'd recommend this, instead:

Also agreed. Zhong, can you resend it like that?

> diff --git a/mm/gup.c b/mm/gup.c
> index 23a9f9c9d377..72bc027037fa 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>         bool drain_allow = true;
>         bool migrate_allow = true;
>         LIST_HEAD(cma_page_list);
> +       long ret = nr_pages;
>  
>  check_again:
>         for (i = 0; i < nr_pages;) {
> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>                  * 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) {
> +               if ((ret > 0) && migrate_allow) {
> +                       nr_pages = ret;
>                         drain_allow = true;
>                         goto check_again;
>                 }
>         }
>  
> -       return nr_pages;
> +       return ret;
>  }
>  #else
>  static long check_and_migrate_cma_pages(struct task_struct *tsk,
> 
> 
> thanks,
> 
> John Hubbard
> NVIDIA
> 

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Alexander Duyck]

On Fri, Oct 18, 2019 at 7:00 AM zhong jiang <zhongjiang@huawei.com> wrote:
>
> On 2019/10/18 4:42, John Hubbard wrote:
> > On 10/16/19 8:19 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.
> >>
> >> The patch use a new local variable to store the return value of
> >> __get_user_pages_locked(). Then use it to compare with zero.
> > Hi Zhong,
> >
> > The above are actually more like notes to yourself, and while those are
> > good to have, but it's not yet a perfect commit description: it talks
> > about how you got here, which is only part of the story. A higher level
> > summary is better, and let the code itself cover the details.
> >
> > Like this:
> >
> > First line (subject):
> >
> >    mm/gup: allow CMA migration to propagate errors back to caller
> >
> > Commit description:
> >
> > check_and_migrate_cma_pages() was recording the result of
> > __get_user_pages_locked() in an unsigned "nr_pages" variable. Because
> > __get_user_pages_locked() returns a signed value that can include
> > negative errno values, this had the effect of hiding errors.
> >
> > Change check_and_migrate_cma_pages() implementation so that it
> > uses a signed variable instead, and propagates the results back
> > to the caller just as other gup internal functions do.
> >
> > This was discovered with the help of unsigned_lesser_than_zero.cocci.
> >
> >> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> >> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> >> ---
> >>  mm/gup.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/mm/gup.c b/mm/gup.c
> >> index 8f236a3..1fe0ceb 100644
> >> --- a/mm/gup.c
> >> +++ b/mm/gup.c
> >> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
> >>      bool drain_allow = true;
> >>      bool migrate_allow = true;
> >>      LIST_HEAD(cma_page_list);
> >> +    long ret;
> > Ira pointed out that this needs initialization, see below.
> >
> >>
> >>  check_again:
> >>      for (i = 0; i < nr_pages;) {
> >> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
> >>               * 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;
> > Although technically correct, it makes me feel odd to see the assignment
> > done from signed to unsigned, *before* checking for >0. And Ira is hinting
> > at the same thing, when he asks if we can return early here. See below...
> >
> >> +            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,
> >>
> > So, overall, I'd recommend this, instead:
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 23a9f9c9d377..72bc027037fa 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
> >         bool drain_allow = true;
> >         bool migrate_allow = true;
> >         LIST_HEAD(cma_page_list);
> > +       long ret = nr_pages;
> >
> I think the ret should be assigned as zero. we will return ret if cma_page_list is empty.
> I miss something?

That wasn't the behavior before. Before if the cma_page_list was empty
nr_pages would not be modified from what what passed. I believe that
is the intended behavior for this since the cma_page_list seems like a
workaround to migrate out any CMA pages if present in the longterm
mapping. If there are no CMA pages the return result should be
nr_pages.

Thanks.

- Alex

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/10/18 4:42, John Hubbard wrote:
> On 10/16/19 8:19 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.
>>
>> The patch use a new local variable to store the return value of
>> __get_user_pages_locked(). Then use it to compare with zero.
> Hi Zhong,
>
> The above are actually more like notes to yourself, and while those are
> good to have, but it's not yet a perfect commit description: it talks 
> about how you got here, which is only part of the story. A higher level
> summary is better, and let the code itself cover the details. 
>
> Like this:
>
> First line (subject): 
>
>    mm/gup: allow CMA migration to propagate errors back to caller
>
> Commit description:
>
> check_and_migrate_cma_pages() was recording the result of 
> __get_user_pages_locked() in an unsigned "nr_pages" variable. Because
> __get_user_pages_locked() returns a signed value that can include
> negative errno values, this had the effect of hiding errors.
>
> Change check_and_migrate_cma_pages() implementation so that it
> uses a signed variable instead, and propagates the results back
> to the caller just as other gup internal functions do.
>
> This was discovered with the help of unsigned_lesser_than_zero.cocci.
>
>> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  mm/gup.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 8f236a3..1fe0ceb 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  	bool drain_allow = true;
>>  	bool migrate_allow = true;
>>  	LIST_HEAD(cma_page_list);
>> +	long ret;
> Ira pointed out that this needs initialization, see below.
>
>>  
>>  check_again:
>>  	for (i = 0; i < nr_pages;) {
>> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  		 * 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;
> Although technically correct, it makes me feel odd to see the assignment
> done from signed to unsigned, *before* checking for >0. And Ira is hinting
> at the same thing, when he asks if we can return early here. See below...
>
>> +		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,
>>
> So, overall, I'd recommend this, instead:
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 23a9f9c9d377..72bc027037fa 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>         bool drain_allow = true;
>         bool migrate_allow = true;
>         LIST_HEAD(cma_page_list);
> +       long ret = nr_pages;
>  
I think the ret should be assigned as zero. we will return ret if cma_page_list is empty.
I miss something?

Thanks,
zhong jiang
>  check_again:
>         for (i = 0; i < nr_pages;) {
> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>                  * 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) {
> +               if ((ret > 0) && migrate_allow) {
> +                       nr_pages = ret;
>                         drain_allow = true;
>                         goto check_again;
>                 }
>         }
>  
> -       return nr_pages;
> +       return ret;
>  }
>  #else
>  static long check_and_migrate_cma_pages(struct task_struct *tsk,
>
>
> thanks,
>
> John Hubbard
> NVIDIA
>
> .
>

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/10/18 4:42, John Hubbard wrote:
> On 10/16/19 8:19 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.
>>
>> The patch use a new local variable to store the return value of
>> __get_user_pages_locked(). Then use it to compare with zero.
> Hi Zhong,
>
> The above are actually more like notes to yourself, and while those are
> good to have, but it's not yet a perfect commit description: it talks 
> about how you got here, which is only part of the story. A higher level
> summary is better, and let the code itself cover the details. 
>
> Like this:
>
> First line (subject): 
>
>    mm/gup: allow CMA migration to propagate errors back to caller
>
> Commit description:
>
> check_and_migrate_cma_pages() was recording the result of 
> __get_user_pages_locked() in an unsigned "nr_pages" variable. Because
> __get_user_pages_locked() returns a signed value that can include
> negative errno values, this had the effect of hiding errors.
>
> Change check_and_migrate_cma_pages() implementation so that it
> uses a signed variable instead, and propagates the results back
> to the caller just as other gup internal functions do.
>
> This was discovered with the help of unsigned_lesser_than_zero.cocci.
>
>> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  mm/gup.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 8f236a3..1fe0ceb 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  	bool drain_allow = true;
>>  	bool migrate_allow = true;
>>  	LIST_HEAD(cma_page_list);
>> +	long ret;
> Ira pointed out that this needs initialization, see below.
I miss that.  Thanks for pointing out.
>>  
>>  check_again:
>>  	for (i = 0; i < nr_pages;) {
>> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  		 * 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;
> Although technically correct, it makes me feel odd to see the assignment
> done from signed to unsigned, *before* checking for >0. And Ira is hinting
> at the same thing, when he asks if we can return early here. See below...
At first.  I realize the same thing, But assign the value from signed to unsigned
before checking for >0 doesn't matter when we can return early here. Because we
only return the ret.  Of course, The assignment is meaningless when we can return early.

It indeed is better to do as you suggested. :-(
>> +		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,
>>
> So, overall, I'd recommend this, instead:
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 23a9f9c9d377..72bc027037fa 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>         bool drain_allow = true;
>         bool migrate_allow = true;
>         LIST_HEAD(cma_page_list);
> +       long ret = nr_pages;
>  
>  check_again:
>         for (i = 0; i < nr_pages;) {
> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>                  * 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) {
> +               if ((ret > 0) && migrate_allow) {
> +                       nr_pages = ret;
>                         drain_allow = true;
>                         goto check_again;
>                 }
>         }
>  
> -       return nr_pages;
> +       return ret;
>  }
>  #else
>  static long check_and_migrate_cma_pages(struct task_struct *tsk,
>
>
> thanks,
>
> John Hubbard
> NVIDIA
>
> .
>
Thanks for your suggestion. I will repost it as you has proposed.

Sincerely,
zhong jiang

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/10/18 2:01, Ira Weiny wrote:
> On Thu, Oct 17, 2019 at 11:19:46AM +0800, 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.
>>
>> The patch use a new local variable to store the return value of
>> __get_user_pages_locked(). Then use it to compare with zero.
>>
>> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>>  mm/gup.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 8f236a3..1fe0ceb 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  	bool drain_allow = true;
>>  	bool migrate_allow = true;
>>  	LIST_HEAD(cma_page_list);
>> +	long ret;
>>  
>>  check_again:
>>  	for (i = 0; i < nr_pages;) {
>> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>  		 * 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;
> I think if ret is negative we want to return the error here.
I think the code works.  Because we alway return the ret.  we will return the error value if the ret is negative .
>> +		if ((ret > 0) && migrate_allow) {
>>  			drain_allow = true;
>>  			goto check_again;
>>  		}
>>  	}
>>  
>> -	return nr_pages;
>> +	return ret;
> If the cma_page_list is empty doesn't this return uninitialized data?
You're right.  I miss that.  Thanks for pointing out.

Sincerely,
zhong jiang
> Ira
>
>>  }
>>  #else
>>  static long check_and_migrate_cma_pages(struct task_struct *tsk,
>> -- 
>> 1.7.12.4
>>
>>
> .
>

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[John Hubbard]

On 10/16/19 8:19 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.
> 
> The patch use a new local variable to store the return value of
> __get_user_pages_locked(). Then use it to compare with zero.

Hi Zhong,

The above are actually more like notes to yourself, and while those are
good to have, but it's not yet a perfect commit description: it talks 
about how you got here, which is only part of the story. A higher level
summary is better, and let the code itself cover the details. 

Like this:

First line (subject): 

   mm/gup: allow CMA migration to propagate errors back to caller

Commit description:

check_and_migrate_cma_pages() was recording the result of 
__get_user_pages_locked() in an unsigned "nr_pages" variable. Because
__get_user_pages_locked() returns a signed value that can include
negative errno values, this had the effect of hiding errors.

Change check_and_migrate_cma_pages() implementation so that it
uses a signed variable instead, and propagates the results back
to the caller just as other gup internal functions do.

This was discovered with the help of unsigned_lesser_than_zero.cocci.

> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  mm/gup.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 8f236a3..1fe0ceb 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>  	bool drain_allow = true;
>  	bool migrate_allow = true;
>  	LIST_HEAD(cma_page_list);
> +	long ret;

Ira pointed out that this needs initialization, see below.

>  
>  check_again:
>  	for (i = 0; i < nr_pages;) {
> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>  		 * 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;

Although technically correct, it makes me feel odd to see the assignment
done from signed to unsigned, *before* checking for >0. And Ira is hinting
at the same thing, when he asks if we can return early here. See below...

> +		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,
> 

So, overall, I'd recommend this, instead:

diff --git a/mm/gup.c b/mm/gup.c
index 23a9f9c9d377..72bc027037fa 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
        bool drain_allow = true;
        bool migrate_allow = true;
        LIST_HEAD(cma_page_list);
+       long ret = nr_pages;
 
 check_again:
        for (i = 0; i < nr_pages;) {
@@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
                 * 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) {
+               if ((ret > 0) && migrate_allow) {
+                       nr_pages = ret;
                        drain_allow = true;
                        goto check_again;
                }
        }
 
-       return nr_pages;
+       return ret;
 }
 #else
 static long check_and_migrate_cma_pages(struct task_struct *tsk,


thanks,

John Hubbard
NVIDIA

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[Ira Weiny]

On Thu, Oct 17, 2019 at 11:19:46AM +0800, 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.
> 
> The patch use a new local variable to store the return value of
> __get_user_pages_locked(). Then use it to compare with zero.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
>  mm/gup.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 8f236a3..1fe0ceb 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>  	bool drain_allow = true;
>  	bool migrate_allow = true;
>  	LIST_HEAD(cma_page_list);
> +	long ret;
>  
>  check_again:
>  	for (i = 0; i < nr_pages;) {
> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
>  		 * 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;

I think if ret is negative we want to return the error here.

> +		if ((ret > 0) && migrate_allow) {
>  			drain_allow = true;
>  			goto check_again;
>  		}
>  	}
>  
> -	return nr_pages;
> +	return ret;

If the cma_page_list is empty doesn't this return uninitialized data?

Ira

>  }
>  #else
>  static long check_and_migrate_cma_pages(struct task_struct *tsk,
> -- 
> 1.7.12.4
> 
>

[PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

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.

The patch use a new local variable to store the return value of
__get_user_pages_locked(). Then use it to compare with zero.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
 mm/gup.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 8f236a3..1fe0ceb 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
 	bool drain_allow = true;
 	bool migrate_allow = true;
 	LIST_HEAD(cma_page_list);
+	long ret;
 
 check_again:
 	for (i = 0; i < nr_pages;) {
@@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk,
 		 * 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.7.12.4