[PATCH] drivers/xen/grant-table.c: Be sure of unsigned value never comparing with 0

[PATCH] drivers/xen/grant-table.c: Be sure of unsigned value never comparing with 0

[Chen Gang]

In grow_gnttab_list(), 'i' is 'unsigned int', and 'nr_glist_frames' may
be 0 because 'nr_grant_frames' may be 0. So 'i' may never be less than
'nr_glist_frames' in failure processing, which cause infinite looping.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/grant-table.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index c254ae0..be07645 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -592,8 +592,8 @@ static int grow_gnttab_list(unsigned int more_frames)
 	return 0;
 
 grow_nomem:
-	for ( ; i >= nr_glist_frames; i--)
-		free_page((unsigned long) gnttab_list[i]);
+	while (i > nr_glist_frames)
+		free_page((unsigned long) gnttab_list[--i]);
 	return -ENOMEM;
 }
 
-- 
1.9.3

Re: [PATCH] drivers/xen/grant-table.c: Be sure of unsigned value never comparing with 0

[David Vrabel]

On 26/08/14 16:38, Chen Gang wrote:
> In grow_gnttab_list(), 'i' is 'unsigned int', and 'nr_glist_frames' may
> be 0 because 'nr_grant_frames' may be 0. So 'i' may never be less than
> 'nr_glist_frames' in failure processing, which cause infinite looping.

nr_grant_frames is at least 1.  See gnttab_init().

> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -592,8 +592,8 @@ static int grow_gnttab_list(unsigned int more_frames)
>  	return 0;
>  
>  grow_nomem:
> -	for ( ; i >= nr_glist_frames; i--)
> -		free_page((unsigned long) gnttab_list[i]);
> +	while (i > nr_glist_frames)
> +		free_page((unsigned long) gnttab_list[--i]);

while (i-- > nr_glist_frames)
    ...

Would have been better.

David

Re: [PATCH] drivers/xen/grant-table.c: Be sure of unsigned value never comparing with 0

[Chen Gang]

On 08/27/2014 01:03 AM, David Vrabel wrote:
> On 26/08/14 16:38, Chen Gang wrote:
>> In grow_gnttab_list(), 'i' is 'unsigned int', and 'nr_glist_frames' may
>> be 0 because 'nr_grant_frames' may be 0. So 'i' may never be less than
>> 'nr_glist_frames' in failure processing, which cause infinite looping.
> 
> nr_grant_frames is at least 1.  See gnttab_init().
> 

OK, thanks, that sounds reasonable to me, it is not a real wold bug, it
is my fault.  :-)

>> --- a/drivers/xen/grant-table.c
>> +++ b/drivers/xen/grant-table.c
>> @@ -592,8 +592,8 @@ static int grow_gnttab_list(unsigned int more_frames)
>>  	return 0;
>>  
>>  grow_nomem:
>> -	for ( ; i >= nr_glist_frames; i--)
>> -		free_page((unsigned long) gnttab_list[i]);
>> +	while (i > nr_glist_frames)
>> +		free_page((unsigned long) gnttab_list[--i]);
> 
> while (i-- > nr_glist_frames)
>     ...
> 
> Would have been better.
> 

OK, thanks, that sounds reasonable to me.

If necessary to send patch v2 (change comments and contents), please
let me know, and I shall send.

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [PATCH] drivers/xen/grant-table.c: Be sure of unsigned value never comparing with 0

[David Vrabel]

On 26/08/14 20:42, Chen Gang wrote:
> On 08/27/2014 01:03 AM, David Vrabel wrote:
>> On 26/08/14 16:38, Chen Gang wrote:
>>> In grow_gnttab_list(), 'i' is 'unsigned int', and 'nr_glist_frames' may
>>> be 0 because 'nr_grant_frames' may be 0. So 'i' may never be less than
>>> 'nr_glist_frames' in failure processing, which cause infinite looping.
>>
>> nr_grant_frames is at least 1.  See gnttab_init().
>>
> 
> OK, thanks, that sounds reasonable to me, it is not a real wold bug, it
> is my fault.  :-)
> 
>>> --- a/drivers/xen/grant-table.c
>>> +++ b/drivers/xen/grant-table.c
>>> @@ -592,8 +592,8 @@ static int grow_gnttab_list(unsigned int more_frames)
>>>  	return 0;
>>>  
>>>  grow_nomem:
>>> -	for ( ; i >= nr_glist_frames; i--)
>>> -		free_page((unsigned long) gnttab_list[i]);
>>> +	while (i > nr_glist_frames)
>>> +		free_page((unsigned long) gnttab_list[--i]);
>>
>> while (i-- > nr_glist_frames)
>>     ...
>>
>> Would have been better.
>>
> 
> OK, thanks, that sounds reasonable to me.
> 
> If necessary to send patch v2 (change comments and contents), please
> let me know, and I shall send.

Applied to devel/for-linus-3.18 with this description:

    xen/grant-table: refactor error cleanup in grow_gnttab_list()

    The cleanup loop in grow_gnttab_list() is safe from the underflow of
    the unsigned 'i' since nr_glist_frames is >= 1, but refactor it
    anyway.

Thanks.

David

Re: [PATCH] drivers/xen/grant-table.c: Be sure of unsigned value never comparing with 0

[Chen Gang]

On 8/27/14 18:06, David Vrabel wrote:
> 
> Applied to devel/for-linus-3.18 with this description:
> 
>     xen/grant-table: refactor error cleanup in grow_gnttab_list()
> 
>     The cleanup loop in grow_gnttab_list() is safe from the underflow of
>     the unsigned 'i' since nr_glist_frames is >= 1, but refactor it
>     anyway.
> 

OK, thank you very much for your encouragement! And next, I shall try to
provide another patches for Xen (hope I can finish within this month).
And I should be more careful about the patches which I shall send.

Also I want to consult: "Is there a common test for Xen"? If there is,
please let me know, and I should try to perform the common test before
send any patches.

Welcome any additional ideas, suggestions, and completions.

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed