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

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

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&amp;data=04%7C01%7Carunpravin.paneerselvam%40amd.com%7C439b31d360ef495ab13408da071a6e1f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637830107357395422%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Z8KNmbUXmhk0xA8z7yHJN2j%2BRJ5VwpuMXww21mrC8x8%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%7C439b31d360ef495ab13408da071a6e1f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637830107357395422%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Mwqy6NVTiR%2FoHFpLvXnQdE95kHoJJUEiig0Juz37ATQ%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.
> 
> 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.
>