[RH7,1/2] cbt: endless loop on rollback in ploop_pb_map_alloc()

Submitted by Vasily Averin on May 31, 2020, 5:19 a.m.

Details

Message ID c996af1c-13c3-fa37-1f0b-d9e2b90d40a1@virtuozzo.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Vasily Averin May 31, 2020, 5:19 a.m.
found by smatch:
drivers/block/ploop/push_backup.c:96 ploop_pb_map_alloc() warn:
 always true condition '(--i >= 0) => (0-u64max >= 0)'

it leads to endless loop on rollback.

https://jira.sw.ru/browse/PSBM-104530
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/block/ploop/push_backup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/block/ploop/push_backup.c b/drivers/block/ploop/push_backup.c
index 168fa24..ac07919 100644
--- a/drivers/block/ploop/push_backup.c
+++ b/drivers/block/ploop/push_backup.c
@@ -81,9 +81,9 @@  int ploop_pb_get_uuid(struct ploop_pushbackup_desc *pbd, __u8 *uuid)
 
 static struct page **ploop_pb_map_alloc(unsigned long block_max)
 {
-	unsigned long npages = NR_PAGES(block_max);
+	long npages = NR_PAGES(block_max);
 	struct page **map = vmalloc(npages * sizeof(void *));
-	unsigned long i;
+	long i;
 
 	if (!map)
 		return NULL;
@@ -106,7 +106,7 @@  static struct page **ploop_pb_map_alloc(unsigned long block_max)
 static void ploop_pb_map_free(struct page **map, unsigned long block_max)
 {
 	if (map) {
-		unsigned long i;
+		long i;
 		for (i = 0; i < NR_PAGES(block_max); i++)
 			if (map[i])
 				__free_page(map[i]);

Comments

Kirill Tkhai June 1, 2020, 8 a.m.
On 31.05.2020 08:19, Vasily Averin wrote:
> found by smatch:
> drivers/block/ploop/push_backup.c:96 ploop_pb_map_alloc() warn:
>  always true condition '(--i >= 0) => (0-u64max >= 0)'
> 
> it leads to endless loop on rollback.
> 
> https://jira.sw.ru/browse/PSBM-104530
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  drivers/block/ploop/push_backup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/ploop/push_backup.c b/drivers/block/ploop/push_backup.c
> index 168fa24..ac07919 100644
> --- a/drivers/block/ploop/push_backup.c
> +++ b/drivers/block/ploop/push_backup.c
> @@ -81,9 +81,9 @@ int ploop_pb_get_uuid(struct ploop_pushbackup_desc *pbd, __u8 *uuid)
>  
>  static struct page **ploop_pb_map_alloc(unsigned long block_max)
>  {
> -	unsigned long npages = NR_PAGES(block_max);
> +	long npages = NR_PAGES(block_max);
>  	struct page **map = vmalloc(npages * sizeof(void *));
> -	unsigned long i;
> +	long i;
>  
>  	if (!map)
>  		return NULL;
> @@ -106,7 +106,7 @@ static struct page **ploop_pb_map_alloc(unsigned long block_max)
>  static void ploop_pb_map_free(struct page **map, unsigned long block_max)
>  {
>  	if (map) {
> -		unsigned long i;
> +		long i;
>  		for (i = 0; i < NR_PAGES(block_max); i++)
>  			if (map[i])
>  				__free_page(map[i]);
> 

unsigned long here is not BUG
Kirill Tkhai June 1, 2020, 8:12 a.m.
On 01.06.2020 11:00, Kirill Tkhai wrote:
> On 31.05.2020 08:19, Vasily Averin wrote:
>> found by smatch:
>> drivers/block/ploop/push_backup.c:96 ploop_pb_map_alloc() warn:
>>  always true condition '(--i >= 0) => (0-u64max >= 0)'
>>
>> it leads to endless loop on rollback.
>>
>> https://jira.sw.ru/browse/PSBM-104530
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> 
>> ---
>>  drivers/block/ploop/push_backup.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/block/ploop/push_backup.c b/drivers/block/ploop/push_backup.c
>> index 168fa24..ac07919 100644
>> --- a/drivers/block/ploop/push_backup.c
>> +++ b/drivers/block/ploop/push_backup.c
>> @@ -81,9 +81,9 @@ int ploop_pb_get_uuid(struct ploop_pushbackup_desc *pbd, __u8 *uuid)
>>  
>>  static struct page **ploop_pb_map_alloc(unsigned long block_max)
>>  {
>> -	unsigned long npages = NR_PAGES(block_max);
>> +	long npages = NR_PAGES(block_max);
>>  	struct page **map = vmalloc(npages * sizeof(void *));
>> -	unsigned long i;
>> +	long i;

Though, this long confuses me.

block_max is unsigned long, while now we can iterate up to LONG_MAX.

>>  
>>  	if (!map)
>>  		return NULL;
>> @@ -106,7 +106,7 @@ static struct page **ploop_pb_map_alloc(unsigned long block_max)
>>  static void ploop_pb_map_free(struct page **map, unsigned long block_max)
>>  {
>>  	if (map) {
>> -		unsigned long i;
>> +		long i;
>>  		for (i = 0; i < NR_PAGES(block_max); i++)
>>  			if (map[i])
>>  				__free_page(map[i]);
>>
> 
> unsigned long here is not BUG
>
Kirill Tkhai June 1, 2020, 8:14 a.m.
On 01.06.2020 11:12, Kirill Tkhai wrote:
> On 01.06.2020 11:00, Kirill Tkhai wrote:
>> On 31.05.2020 08:19, Vasily Averin wrote:
>>> found by smatch:
>>> drivers/block/ploop/push_backup.c:96 ploop_pb_map_alloc() warn:
>>>  always true condition '(--i >= 0) => (0-u64max >= 0)'
>>>
>>> it leads to endless loop on rollback.
>>>
>>> https://jira.sw.ru/browse/PSBM-104530
>>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>>
>> Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>
>>> ---
>>>  drivers/block/ploop/push_backup.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/block/ploop/push_backup.c b/drivers/block/ploop/push_backup.c
>>> index 168fa24..ac07919 100644
>>> --- a/drivers/block/ploop/push_backup.c
>>> +++ b/drivers/block/ploop/push_backup.c
>>> @@ -81,9 +81,9 @@ int ploop_pb_get_uuid(struct ploop_pushbackup_desc *pbd, __u8 *uuid)
>>>  
>>>  static struct page **ploop_pb_map_alloc(unsigned long block_max)
>>>  {
>>> -	unsigned long npages = NR_PAGES(block_max);
>>> +	long npages = NR_PAGES(block_max);
>>>  	struct page **map = vmalloc(npages * sizeof(void *));
>>> -	unsigned long i;
>>> +	long i;
> 
> Though, this long confuses me.
> 
> block_max is unsigned long, while now we can iterate up to LONG_MAX.

Though, there is NR_PAGES() shift, so everything OK.
 
>>>  
>>>  	if (!map)
>>>  		return NULL;
>>> @@ -106,7 +106,7 @@ static struct page **ploop_pb_map_alloc(unsigned long block_max)
>>>  static void ploop_pb_map_free(struct page **map, unsigned long block_max)
>>>  {
>>>  	if (map) {
>>> -		unsigned long i;
>>> +		long i;
>>>  		for (i = 0; i < NR_PAGES(block_max); i++)
>>>  			if (map[i])
>>>  				__free_page(map[i]);
>>>
>>
>> unsigned long here is not BUG
>>
>