Re: [Intel-gfx] [PATCH] drm: Fix a infinite loop condition when order becomes 0

[Arunpravin Paneer Selvam <arunpravin.paneerselvam@amd.com>]

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&amp;data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7C3610aafe216d421c715c08da069ac1d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829559006306914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=GM3iXiDQCx%2BM4pD1nmivRFRvkehwTNd2Jtd713cF51g%3D&amp;reserved=0
>>>> drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Ftree%2F&amp;data=04%7C01%7CArunpravin.PaneerSelvam%40amd.com%7C3610aafe216d421c715c08da069ac1d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829559006306914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=i7pvmDJu310XRX7h3cQ344j5RYHq7fBZ520l%2F%2Br1%2BQU%3D&amp;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
>