Re: Ping: [PATCH] libxc/PM: correct (not just) error handling in xc_get_cpufreq_para()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 11:59, Anthony PERARD wrote:
> On Mon, Apr 07, 2025 at 05:38:57PM +0200, Jan Beulich wrote:
>> On 07.04.2025 17:23, Anthony PERARD wrote:
>>> On Mon, Apr 07, 2025 at 03:23:48PM +0200, Jan Beulich wrote:
>>>> On 07.04.2025 14:45, Anthony PERARD wrote:
>>>>> Calling xc_get_cpufreq_para with:
>>>>>
>>>>>     user_para = {
>>>>>         .cpu_num = 0,
>>>>>         .freq_num = 0,
>>>>>         .gov_num = 9,
>>>>>     };
>>>>>
>>>>> seems broken. It's looks like the `scaling_available_governors` bounce
>>>>> buffer is going to be used without been allocated properly handled, with
>>>>> this patch.
>>>>
>>>> The local variable "in_gov_num" controls its allocation and use, together 
>>>> with
>>>> has_num. "Use" as in passing to set_xen_guest_handle(). The only further 
>>>> use
>>>
>>> When has_num == 0, `in_gov_num` is only used to set `sys_para->gov_num`.
>>> It also make a conditional call to xc_hypercall_bounce_post() but
>>> there's nothing to do.
>>>
>>> Why user_para.gov_num seems to control the size of a buffer, but then
>>> that buffer is just discarded under some condition with this patch?
>>
>> That's nothing this patch changes. Previously has_num would also have been
>> false in the example you give.
> 
> Right, sorry. I was persuaded that `has_num` meant "any" of the buffers
> are allocated, instead of the written "all" of them are allocated in C.
> The logic in this function is really hard to follow because it doesn't
> make sense, especially the conditional on `has_num`.
> 
> Your patch does make requesting governors actually optional now (and now
> that I realise the calculation of `has_num` doesn't really reflect the
> name). The introduced `in_gov_num` local variable isn't very useful as
> the only real need is in the cleaning path (and we discussed earlier
> that cleaning can be done unconditionally).

Hmm, yes. See below.

> So the patch is fine:
> 
> Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks.

> Oh, one more thing, it's funny that a lot of faff is done toward making
> the cleaning optional, with all the "unlock_*" label, but then cleaning
> code path can be executed when e.g. cpu_num=0,freq_num=4 (unless the
> hypercall return an error in such case, but the code shouldn't rely on
> that...).

Yeah, perhaps I could have dropped the conditional there, rather than updating
it. Are you happy for me to do so, dropping in_gov_num again (adjusting the
description some, of course)?

Jan