On 27.06.2022 12:03, Penny Zheng wrote:
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Wednesday, June 22, 2022 5:24 PM
>>
>> Furthermore careful with the local variable name used here. Consider what
>> would happen with an invocation of
>>
>> put_static_pages(d, page, i);
>>
>> To common approach is to suffix an underscore to the variable name.
>> Such names are not supposed to be used outside of macros definitions, and
>> hence there's then no potential for such a conflict.
>>
>
> Understood!! I will change "unsigned int i" to "unsigned int _i";
Note how I said "suffix", not "prefix".
>> Finally I think you mean (1u << (order)) to be on the safe side against UB if
>> order could ever reach 31. Then again - is "order" as a parameter needed
>> here in the first place? Wasn't it that staticmem operations are limited to
>> order-0 regions?
>
> Yes, right now, the actual usage is limited to order-0, how about I add
> assertion here
> and remove order parameter:
>
> /* Add page on the resv_page_list *after* it has been freed. */
> if ( unlikely(pg->count_info & PGC_static) )
> {
> ASSERT(!order);
> put_static_pages(d, pg);
> }
I don't mind an ASSERT() as long as upper layers indeed guarantee this.
What I'm worried about is that you might assert on user controlled input.
Jan
