| Message ID | c996af1c-13c3-fa37-1f0b-d9e2b90d40a1@virtuozzo.com |
|---|---|
| State | New |
| Series | "Series without cover letter" |
| Headers | show |
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]);
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
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 >
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 >> >
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(-)