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

["Anthony PERARD" <anthony.perard@vates.tech>]

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@suse.com>
> >>
> >> 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?

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)

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

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech