| Message ID | 20220314194049.534471-1-Arunpravin.PaneerSelvam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix a infinite loop condition when order becomes 0 | expand |
Dear Arunpravin, Am 14.03.22 um 20:40 schrieb Arunpravin: > handle a situation in the condition order-- == min_order, > when order = 0, leading to order = -1, it now won't exit > the loop. To avoid this problem, added a order check in > the same condition, (i.e) when order is 0, we return > -ENOSPC > > Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> Please use your full name. > --- > drivers/gpu/drm/drm_buddy.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c > index 72f52f293249..5ab66aaf2bbd 100644 > --- a/drivers/gpu/drm/drm_buddy.c > +++ b/drivers/gpu/drm/drm_buddy.c In what tree is that file? > @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, > if (!IS_ERR(block)) > break; > > - if (order-- == min_order) { > + if (!order || order-- == min_order) { > err = -ENOSPC; > goto err_free; > } Kind regards, Paul
On 15/03/22 1:49 pm, Paul Menzel wrote: > Dear Arunpravin, > > > Am 14.03.22 um 20:40 schrieb Arunpravin: >> handle a situation in the condition order-- == min_order, >> when order = 0, leading to order = -1, it now won't exit >> the loop. To avoid this problem, added a order check in >> the same condition, (i.e) when order is 0, we return >> -ENOSPC >> >> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> > > Please use your full name. okay > >> --- >> drivers/gpu/drm/drm_buddy.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >> index 72f52f293249..5ab66aaf2bbd 100644 >> --- a/drivers/gpu/drm/drm_buddy.c >> +++ b/drivers/gpu/drm/drm_buddy.c > > In what tree is that file? > drm-tip - https://cgit.freedesktop.org/drm-tip/tree/ drm-misc-next - https://cgit.freedesktop.org/drm/drm-misc/tree/ >> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >> if (!IS_ERR(block)) >> break; >> >> - if (order-- == min_order) { >> + if (!order || order-- == min_order) { >> err = -ENOSPC; >> goto err_free; >> } > > Kind regards, > > Paul >
Dear Arunpravin, Am 15.03.22 um 10:01 schrieb Arunpravin: > On 15/03/22 1:49 pm, Paul Menzel wrote: >> Am 14.03.22 um 20:40 schrieb Arunpravin: >>> handle a situation in the condition order-- == min_order, >>> when order = 0, leading to order = -1, it now won't exit >>> the loop. To avoid this problem, added a order check in >>> the same condition, (i.e) when order is 0, we return >>> -ENOSPC >>> >>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> >> >> Please use your full name. > okay You might also configure that in your email program. >>> --- >>> drivers/gpu/drm/drm_buddy.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>> index 72f52f293249..5ab66aaf2bbd 100644 >>> --- a/drivers/gpu/drm/drm_buddy.c >>> +++ b/drivers/gpu/drm/drm_buddy.c >> >> In what tree is that file? >> > drm-tip - https://cgit.freedesktop.org/drm-tip/tree/ > drm-misc-next - https://cgit.freedesktop.org/drm/drm-misc/tree/ > >>> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>> if (!IS_ERR(block)) >>> break; >>> >>> - if (order-- == min_order) { >>> + if (!order || order-- == min_order) { >>> err = -ENOSPC; >>> goto err_free; >>> } Thank you for the hint. So the whole function is: do { order = min(order, (unsigned int)fls(pages) - 1); BUG_ON(order > mm->max_order); BUG_ON(order < min_order); do { if (flags & DRM_BUDDY_RANGE_ALLOCATION) /* Allocate traversing within the range */ block = alloc_range_bias(mm, start, end, order); else /* Allocate from freelist */ block = alloc_from_freelist(mm, order, flags); if (!IS_ERR(block)) break; if (order-- == min_order) { err = -ENOSPC; goto err_free; } } while (1); mark_allocated(block); mm->avail -= drm_buddy_block_size(mm, block); kmemleak_update_trace(block); list_add_tail(&block->link, &allocated); pages -= BIT(order); if (!pages) break; } while (1); Was the BUG_ON triggered for your case? BUG_ON(order < min_order); Please give more details. Kind regards, Paul
On 14/03/2022 19:40, Arunpravin wrote: > handle a situation in the condition order-- == min_order, > when order = 0, leading to order = -1, it now won't exit > the loop. To avoid this problem, added a order check in > the same condition, (i.e) when order is 0, we return > -ENOSPC > > Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> Hmm, it sounded like we were instead going to go with the round_up(size, min_page_size), or check and bail if the size is misaligned, in which case we don't need this, AFAICT. > --- > drivers/gpu/drm/drm_buddy.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c > index 72f52f293249..5ab66aaf2bbd 100644 > --- a/drivers/gpu/drm/drm_buddy.c > +++ b/drivers/gpu/drm/drm_buddy.c > @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, > if (!IS_ERR(block)) > break; > > - if (order-- == min_order) { > + if (!order || order-- == min_order) { > err = -ENOSPC; > goto err_free; > }
On 15/03/22 2:35 pm, Paul Menzel wrote: > Dear Arunpravin, > > > Am 15.03.22 um 10:01 schrieb Arunpravin: > >> On 15/03/22 1:49 pm, Paul Menzel wrote: > >>> Am 14.03.22 um 20:40 schrieb Arunpravin: >>>> handle a situation in the condition order-- == min_order, >>>> when order = 0, leading to order = -1, it now won't exit >>>> the loop. To avoid this problem, added a order check in >>>> the same condition, (i.e) when order is 0, we return >>>> -ENOSPC >>>> >>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> >>> >>> Please use your full name. >> okay > > You might also configure that in your email program. yes > >>>> --- >>>> drivers/gpu/drm/drm_buddy.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>> index 72f52f293249..5ab66aaf2bbd 100644 >>>> --- a/drivers/gpu/drm/drm_buddy.c >>>> +++ b/drivers/gpu/drm/drm_buddy.c >>> >>> In what tree is that file? >>> >> drm-tip - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm-tip%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7Cc456573102c04191cf9708da0662f798%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829319396954551%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5Bspe5QGjQ0KHfVI8%2F%2BXqxR45q6tOL4FE2fVD3uwL%2FM%3D&reserved=0 >> drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7Cc456573102c04191cf9708da0662f798%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829319396954551%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=g2S14TfsHF5ORo9jTZ3uA0l1BH8mnAxk2OWYJeF5i8k%3D&reserved=0 >> >>>> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>> if (!IS_ERR(block)) >>>> break; >>>> >>>> - if (order-- == min_order) { >>>> + if (!order || order-- == min_order) { >>>> err = -ENOSPC; >>>> goto err_free; >>>> } > > Thank you for the hint. So the whole function is: > > do { > order = min(order, (unsigned int)fls(pages) - 1); > BUG_ON(order > mm->max_order); > BUG_ON(order < min_order); > > do { > if (flags & DRM_BUDDY_RANGE_ALLOCATION) > /* Allocate traversing within the range */ > block = alloc_range_bias(mm, start, end, order); > else > /* Allocate from freelist */ > block = alloc_from_freelist(mm, order, flags); > > if (!IS_ERR(block)) > break; > > if (order-- == min_order) { > err = -ENOSPC; > goto err_free; > } > } while (1); > > mark_allocated(block); > mm->avail -= drm_buddy_block_size(mm, block); > kmemleak_update_trace(block); > list_add_tail(&block->link, &allocated); > > pages -= BIT(order); > > if (!pages) > break; > } while (1); > > Was the BUG_ON triggered for your case? > > BUG_ON(order < min_order); no, this BUG_ON is not triggered for this bug > > Please give more details. there is a chance when there is no space to allocate, order value decrements and reaches to 0 at one point, here we should exit the loop, otherwise, further order value decrements to -1 and do..while loop doesn't exit. Hence added a check to exit the loop if order value becomes 0. Thanks, Arun > > > Kind regards, > > Paul >
Dear Arunpravin, Am 15.03.22 um 16:42 schrieb Arunpravin: > On 15/03/22 2:35 pm, Paul Menzel wrote: >> Am 15.03.22 um 10:01 schrieb Arunpravin: >> >>> On 15/03/22 1:49 pm, Paul Menzel wrote: >> >>>> Am 14.03.22 um 20:40 schrieb Arunpravin: >>>>> handle a situation in the condition order-- == min_order, >>>>> when order = 0, leading to order = -1, it now won't exit >>>>> the loop. To avoid this problem, added a order check in >>>>> the same condition, (i.e) when order is 0, we return >>>>> -ENOSPC >>>>> >>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> >>>> >>>> Please use your full name. >>> okay >> >> You might also configure that in your email program. > yes Not done yet though. ;-) >>>>> --- >>>>> drivers/gpu/drm/drm_buddy.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>>> index 72f52f293249..5ab66aaf2bbd 100644 >>>>> --- a/drivers/gpu/drm/drm_buddy.c >>>>> +++ b/drivers/gpu/drm/drm_buddy.c >>>> >>>> In what tree is that file? >>>> >>> drm-tip - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm-tip%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7Cc456573102c04191cf9708da0662f798%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829319396954551%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5Bspe5QGjQ0KHfVI8%2F%2BXqxR45q6tOL4FE2fVD3uwL%2FM%3D&reserved=0 >>> drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7Cc456573102c04191cf9708da0662f798%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829319396954551%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=g2S14TfsHF5ORo9jTZ3uA0l1BH8mnAxk2OWYJeF5i8k%3D&reserved=0 Thank Outlook. Now everybody feels safe. >>>>> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>>> if (!IS_ERR(block)) >>>>> break; >>>>> >>>>> - if (order-- == min_order) { >>>>> + if (!order || order-- == min_order) { >>>>> err = -ENOSPC; >>>>> goto err_free; >>>>> } >> >> Thank you for the hint. So the whole function is: >> >> do { >> order = min(order, (unsigned int)fls(pages) - 1); >> BUG_ON(order > mm->max_order); >> BUG_ON(order < min_order); >> >> do { >> if (flags & DRM_BUDDY_RANGE_ALLOCATION) >> /* Allocate traversing within the range */ >> block = alloc_range_bias(mm, start, end, order); >> else >> /* Allocate from freelist */ >> block = alloc_from_freelist(mm, order, flags); >> >> if (!IS_ERR(block)) >> break; >> >> if (order-- == min_order) { >> err = -ENOSPC; >> goto err_free; >> } >> } while (1); >> >> mark_allocated(block); >> mm->avail -= drm_buddy_block_size(mm, block); >> kmemleak_update_trace(block); >> list_add_tail(&block->link, &allocated); >> >> pages -= BIT(order); >> >> if (!pages) >> break; >> } while (1); >> >> Was the BUG_ON triggered for your case? >> >> BUG_ON(order < min_order); > no, this BUG_ON is not triggered for this bug >> >> Please give more details. > > there is a chance when there is no space to allocate, order value > decrements and reaches to 0 at one point, here we should exit the loop, > otherwise, further order value decrements to -1 and do..while loop > doesn't exit. Hence added a check to exit the loop if order value becomes 0. Sorry, I do not see it. How can that be with order ≥ min_order and the check `order-- == min_order`? Is min_order 0? Please explain that in the next commit message. Kind regards, Paul
On 15/03/22 9:14 pm, Paul Menzel wrote: > Dear Arunpravin, > > > Am 15.03.22 um 16:42 schrieb Arunpravin: > >> On 15/03/22 2:35 pm, Paul Menzel wrote: > >>> Am 15.03.22 um 10:01 schrieb Arunpravin: >>> >>>> On 15/03/22 1:49 pm, Paul Menzel wrote: >>> >>>>> Am 14.03.22 um 20:40 schrieb Arunpravin: >>>>>> handle a situation in the condition order-- == min_order, >>>>>> when order = 0, leading to order = -1, it now won't exit >>>>>> the loop. To avoid this problem, added a order check in >>>>>> the same condition, (i.e) when order is 0, we return >>>>>> -ENOSPC >>>>>> >>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> >>>>> >>>>> Please use your full name. >>>> okay >>> >>> You might also configure that in your email program. >> yes > > Not done yet though. ;-) > done in v2 :) >>>>>> --- >>>>>> drivers/gpu/drm/drm_buddy.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>>>> index 72f52f293249..5ab66aaf2bbd 100644 >>>>>> --- a/drivers/gpu/drm/drm_buddy.c >>>>>> +++ b/drivers/gpu/drm/drm_buddy.c >>>>> >>>>> In what tree is that file? >>>>> >>>> drm-tip - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm-tip%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7C3610aafe216d421c715c08da069ac1d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829559006306914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GM3iXiDQCx%2BM4pD1nmivRFRvkehwTNd2Jtd713cF51g%3D&reserved=0 >>>> drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7C3610aafe216d421c715c08da069ac1d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829559006306914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=i7pvmDJu310XRX7h3cQ344j5RYHq7fBZ520l%2F%2Br1%2BQU%3D&reserved=0 > > Thank Outlook. Now everybody feels safe. > >>>>>> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>>>> if (!IS_ERR(block)) >>>>>> break; >>>>>> >>>>>> - if (order-- == min_order) { >>>>>> + if (!order || order-- == min_order) { >>>>>> err = -ENOSPC; >>>>>> goto err_free; >>>>>> } >>> >>> Thank you for the hint. So the whole function is: >>> >>> do { >>> order = min(order, (unsigned int)fls(pages) - 1); >>> BUG_ON(order > mm->max_order); >>> BUG_ON(order < min_order); >>> >>> do { >>> if (flags & DRM_BUDDY_RANGE_ALLOCATION) >>> /* Allocate traversing within the range */ >>> block = alloc_range_bias(mm, start, end, order); >>> else >>> /* Allocate from freelist */ >>> block = alloc_from_freelist(mm, order, flags); >>> >>> if (!IS_ERR(block)) >>> break; >>> >>> if (order-- == min_order) { >>> err = -ENOSPC; >>> goto err_free; >>> } >>> } while (1); >>> >>> mark_allocated(block); >>> mm->avail -= drm_buddy_block_size(mm, block); >>> kmemleak_update_trace(block); >>> list_add_tail(&block->link, &allocated); >>> >>> pages -= BIT(order); >>> >>> if (!pages) >>> break; >>> } while (1); >>> >>> Was the BUG_ON triggered for your case? >>> >>> BUG_ON(order < min_order); >> no, this BUG_ON is not triggered for this bug >>> >>> Please give more details. >> >> there is a chance when there is no space to allocate, order value >> decrements and reaches to 0 at one point, here we should exit the loop, >> otherwise, further order value decrements to -1 and do..while loop >> doesn't exit. Hence added a check to exit the loop if order value becomes 0. > > Sorry, I do not see it. How can that be with order ≥ min_order and the > check `order-- == min_order`? Is min_order 0? Please explain that in the > next commit message. > please check v2, yes when min_order is 0, the above said situation may occur.And, since the order is unsigned int, I think it will not trigger the BUG_ON(order < min_order) when order becomes -1. Hence I think we needed a check !order to exit the loop. Regards, Arun > > Kind regards, > > Paul >
Dear Arunprivin, Am 16.03.22 um 07:49 schrieb Arunpravin Paneer Selvam: > On 15/03/22 9:14 pm, Paul Menzel wrote: >> Am 15.03.22 um 16:42 schrieb Arunpravin: >> >>> On 15/03/22 2:35 pm, Paul Menzel wrote: >> >>>> Am 15.03.22 um 10:01 schrieb Arunpravin: >>>> >>>>> On 15/03/22 1:49 pm, Paul Menzel wrote: >>>> >>>>>> Am 14.03.22 um 20:40 schrieb Arunpravin: >>>>>>> handle a situation in the condition order-- == min_order, >>>>>>> when order = 0, leading to order = -1, it now won't exit >>>>>>> the loop. To avoid this problem, added a order check in >>>>>>> the same condition, (i.e) when order is 0, we return >>>>>>> -ENOSPC >>>>>>> >>>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> >>>>>> >>>>>> Please use your full name. >>>>> okay >>>> >>>> You might also configure that in your email program. >>> yes >> >> Not done yet though. ;-) >> > done in v2 :) >>>>>>> --- >>>>>>> drivers/gpu/drm/drm_buddy.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>>>>> index 72f52f293249..5ab66aaf2bbd 100644 >>>>>>> --- a/drivers/gpu/drm/drm_buddy.c >>>>>>> +++ b/drivers/gpu/drm/drm_buddy.c >>>>>> >>>>>> In what tree is that file? >>>>>> >>>>> drm-tip - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm-tip%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7C3610aafe216d421c715c08da069ac1d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829559006306914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GM3iXiDQCx%2BM4pD1nmivRFRvkehwTNd2Jtd713cF51g%3D&reserved=0 >>>>> drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Ftree%2F&data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7C3610aafe216d421c715c08da069ac1d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829559006306914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=i7pvmDJu310XRX7h3cQ344j5RYHq7fBZ520l%2F%2Br1%2BQU%3D&reserved=0 >> >> Thank Outlook. Now everybody feels safe. >> >>>>>>> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>>>>> if (!IS_ERR(block)) >>>>>>> break; >>>>>>> >>>>>>> - if (order-- == min_order) { >>>>>>> + if (!order || order-- == min_order) { >>>>>>> err = -ENOSPC; >>>>>>> goto err_free; >>>>>>> } >>>> >>>> Thank you for the hint. So the whole function is: >>>> >>>> do { >>>> order = min(order, (unsigned int)fls(pages) - 1); >>>> BUG_ON(order > mm->max_order); >>>> BUG_ON(order < min_order); >>>> >>>> do { >>>> if (flags & DRM_BUDDY_RANGE_ALLOCATION) >>>> /* Allocate traversing within the range */ >>>> block = alloc_range_bias(mm, start, end, order); >>>> else >>>> /* Allocate from freelist */ >>>> block = alloc_from_freelist(mm, order, flags); >>>> >>>> if (!IS_ERR(block)) >>>> break; >>>> >>>> if (order-- == min_order) { >>>> err = -ENOSPC; >>>> goto err_free; >>>> } >>>> } while (1); >>>> >>>> mark_allocated(block); >>>> mm->avail -= drm_buddy_block_size(mm, block); >>>> kmemleak_update_trace(block); >>>> list_add_tail(&block->link, &allocated); >>>> >>>> pages -= BIT(order); >>>> >>>> if (!pages) >>>> break; >>>> } while (1); >>>> >>>> Was the BUG_ON triggered for your case? >>>> >>>> BUG_ON(order < min_order); >>> no, this BUG_ON is not triggered for this bug >>>> >>>> Please give more details. >>> >>> there is a chance when there is no space to allocate, order value >>> decrements and reaches to 0 at one point, here we should exit the loop, >>> otherwise, further order value decrements to -1 and do..while loop >>> doesn't exit. Hence added a check to exit the loop if order value becomes 0. >> >> Sorry, I do not see it. How can that be with order ≥ min_order and the >> check `order-- == min_order`? Is min_order 0? Please explain that in the >> next commit message. >> > please check v2, yes when min_order is 0, the above said situation may > occur.And, since the order is unsigned int, I think it will not trigger > the BUG_ON(order < min_order) when order becomes -1. Hence I think we > needed a check !order to exit the loop. Thank you for clarifying this. I still do not understand it though. With order = fls(pages) - 1; min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); is zorder` always non-negative? Let’s assume it is. Also, can min_order get “negative” (wraps around)? I would add BUG_ON statements for these cases? BUG_ON(fls(pages) - 1 < 1); BUG_ON(ilog2(min_page_size) - ilog2(mm->chunk_size) < 1); Assuming “negative” is not possible, your case can only happen if `order` and `min_order` are 0, right? If `order` is greater than 0, and `min_order` is 0, the first BUG_ON in the while loop would be hit. If `order` is 0 and `min_order` is greater than 0, everything should work as the condition in `if (order-- == min_order)` is going to be true eventually. Could you please analyze this more. The current patch looks more like papering over something, or I am missing something. Kind regards, Paul PS: The commit message summary of your v2 should also be updated.
On 16/03/22 12:28 pm, Paul Menzel wrote: > Dear Arunprivin, > > > Am 16.03.22 um 07:49 schrieb Arunpravin Paneer Selvam: > >> On 15/03/22 9:14 pm, Paul Menzel wrote: > >>> Am 15.03.22 um 16:42 schrieb Arunpravin: >>> >>>> On 15/03/22 2:35 pm, Paul Menzel wrote: >>> >>>>> Am 15.03.22 um 10:01 schrieb Arunpravin: >>>>> >>>>>> On 15/03/22 1:49 pm, Paul Menzel wrote: >>>>> >>>>>>> Am 14.03.22 um 20:40 schrieb Arunpravin: >>>>>>>> handle a situation in the condition order-- == min_order, >>>>>>>> when order = 0, leading to order = -1, it now won't exit >>>>>>>> the loop. To avoid this problem, added a order check in >>>>>>>> the same condition, (i.e) when order is 0, we return >>>>>>>> -ENOSPC >>>>>>>> >>>>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> >>>>>>> >>>>>>> Please use your full name. >>>>>> okay >>>>> >>>>> You might also configure that in your email program. >>>> yes >>> >>> Not done yet though. ;-) >>> >> done in v2 :) >>>>>>>> --- >>>>>>>> drivers/gpu/drm/drm_buddy.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>>>>>> index 72f52f293249..5ab66aaf2bbd 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_buddy.c >>>>>>>> +++ b/drivers/gpu/drm/drm_buddy.c >>>>>>> >>>>>>> In what tree is that file? >>>>>>> >>>>>> drm-tip - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm-tip%2Ftree%2F&data=04%7C01%7Carunpravin.paneerselvam%40amd.com%7C439b31d360ef495ab13408da071a6e1f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637830107357395422%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Z8KNmbUXmhk0xA8z7yHJN2j%2BRJ5VwpuMXww21mrC8x8%3D&reserved=0 >>>>>> drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Ftree%2F&data=04%7C01%7Carunpravin.paneerselvam%40amd.com%7C439b31d360ef495ab13408da071a6e1f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637830107357395422%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Mwqy6NVTiR%2FoHFpLvXnQdE95kHoJJUEiig0Juz37ATQ%3D&reserved=0 >>> >>> Thank Outlook. Now everybody feels safe. >>> >>>>>>>> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>>>>>> if (!IS_ERR(block)) >>>>>>>> break; >>>>>>>> >>>>>>>> - if (order-- == min_order) { >>>>>>>> + if (!order || order-- == min_order) { >>>>>>>> err = -ENOSPC; >>>>>>>> goto err_free; >>>>>>>> } >>>>> >>>>> Thank you for the hint. So the whole function is: >>>>> >>>>> do { >>>>> order = min(order, (unsigned int)fls(pages) - 1); >>>>> BUG_ON(order > mm->max_order); >>>>> BUG_ON(order < min_order); >>>>> >>>>> do { >>>>> if (flags & DRM_BUDDY_RANGE_ALLOCATION) >>>>> /* Allocate traversing within the range */ >>>>> block = alloc_range_bias(mm, start, end, order); >>>>> else >>>>> /* Allocate from freelist */ >>>>> block = alloc_from_freelist(mm, order, flags); >>>>> >>>>> if (!IS_ERR(block)) >>>>> break; >>>>> >>>>> if (order-- == min_order) { >>>>> err = -ENOSPC; >>>>> goto err_free; >>>>> } >>>>> } while (1); >>>>> >>>>> mark_allocated(block); >>>>> mm->avail -= drm_buddy_block_size(mm, block); >>>>> kmemleak_update_trace(block); >>>>> list_add_tail(&block->link, &allocated); >>>>> >>>>> pages -= BIT(order); >>>>> >>>>> if (!pages) >>>>> break; >>>>> } while (1); >>>>> >>>>> Was the BUG_ON triggered for your case? >>>>> >>>>> BUG_ON(order < min_order); >>>> no, this BUG_ON is not triggered for this bug >>>>> >>>>> Please give more details. >>>> >>>> there is a chance when there is no space to allocate, order value >>>> decrements and reaches to 0 at one point, here we should exit the loop, >>>> otherwise, further order value decrements to -1 and do..while loop >>>> doesn't exit. Hence added a check to exit the loop if order value becomes 0. >>> >>> Sorry, I do not see it. How can that be with order ≥ min_order and the >>> check `order-- == min_order`? Is min_order 0? Please explain that in the >>> next commit message. >>> >> please check v2, yes when min_order is 0, the above said situation may >> occur.And, since the order is unsigned int, I think it will not trigger >> the BUG_ON(order < min_order) when order becomes -1. Hence I think we >> needed a check !order to exit the loop. > > Thank you for clarifying this. I still do not understand it though. With > > order = fls(pages) - 1; > min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); > > is zorder` always non-negative? Let’s assume it is. Also, can min_order > get “negative” (wraps around)? > > I would add BUG_ON statements for these cases? > > BUG_ON(fls(pages) - 1 < 1); > BUG_ON(ilog2(min_page_size) - ilog2(mm->chunk_size) < 1); > > Assuming “negative” is not possible, your case can only happen if > `order` and `min_order` are 0, right? If `order` is greater than 0, and > `min_order` is 0, the first BUG_ON in the while loop would be hit. If > `order` is 0 and `min_order` is greater than 0, everything should work > as the condition in `if (order-- == min_order)` is going to be true > eventually. > > Could you please analyze this more. The current patch looks more like > papering over something, or I am missing something. > Thanks for the analysis, Matthew suggested to add a simple check, I have sent the patch for the review. Regards, Arun > > Kind regards, > > Paul > > > PS: The commit message summary of your v2 should also be updated. >
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 72f52f293249..5ab66aaf2bbd 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, if (!IS_ERR(block)) break; - if (order-- == min_order) { + if (!order || order-- == min_order) { err = -ENOSPC; goto err_free; }
handle a situation in the condition order-- == min_order, when order = 0, leading to order = -1, it now won't exit the loop. To avoid this problem, added a order check in the same condition, (i.e) when order is 0, we return -ENOSPC Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com> --- drivers/gpu/drm/drm_buddy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)