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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2025 14:45, Anthony PERARD wrote:
> On Mon, Apr 07, 2025 at 01:38:24PM +0200, Jan Beulich wrote:
>> On 27.03.2025 14:32, Jan Beulich wrote:
>>> From their introduction all xc_hypercall_bounce_pre() uses, when they
>>> failed, would properly cause exit from the function including cleanup,
>>> yet without informing the caller of the failure. Purge the unlock_1
>>> label for being both pointless and mis-named.
>>>
>>> An earlier attempt to switch to the usual split between return value and
>>> errno wasn't quite complete.
>>>
>>> HWP work made the cleanup of the "available governors" array
>>> conditional, neglecting the fact that the condition used may not be the
>>> condition that was used to allocate the buffer (as the structure field
>>> is updated upon getting back EAGAIN). Throughout the function, use the
>>> local variable being introduced to address that.
>>>
>>> Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall 
>>> buffers")
>>> Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error 
>>> and stash error in errno")
>>> Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> May I ask for an ack or comments towards what needs changing?
> 
> 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
of that bounce buffer is on the exit path, i.e. it being passed to
xc_hypercall_bounce_post(). The backing function xc__hypercall_bounce_post()
is dealing fine with the buffer being NULL. And that's what it would be left
at from DECLARE_NAMED_HYPERCALL_BOUNCE() if buffer allocation is skipped.

Jan