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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> 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
> 
> 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.

> Is the proposed parameter (where only gov_num is set) a valid input? If
> not, why is it not rejected before making the hypercall? (with this
> patch)

Prior to the change adding the user_para->gov_num conditionals the interface
was all or nothing: You got actual data back only if you asked for all three
pieces. Said change then made obtaining the governors optional, yet only
quite. Once obtaining frequencies also becomes optional, I think we will be
in a better position to sanitize this function. Right now I'm only trying to
get some of the basic flaws sorted. The three modes we want/need to support
right now are
- caller wants just counts, but not actual data (would pass in all three
  *_num as zero),
- caller wants all data (would pass in all three *_num as non-zero),
- the HWP special case of wanting CPU and frequency data, but not the
  (meaningless there) governors.

Jan