| Message ID | df676738-19e7-47e6-977f-25d6d13ccc50@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | libxc/PM: correct (not just) error handling in xc_get_cpufreq_para() | expand |
On Thu, Mar 27, 2025 at 02:32:04PM +0100, Jan Beulich wrote: > HWP work made the cleanup of the "available governors" array > conditional, neglecting the fact that the condition used may not be the I don't know why the cleanup was made conditional, as long as the bounce buffer is declared with DECLARE_NAMED_HYPERCALL_BOUNCE(), xc_hypercall_bounce_post() can be called without issue (even if ..bounce_pre isn't used). Maybe it's all those "unlock_*" labels that is misleading, a single "out" label which does the cleanup unconditionally would be more than enough. > 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. > > --- a/tools/libs/ctrl/xc_pm.c > +++ b/tools/libs/ctrl/xc_pm.c > @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc > DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors, > user_para->scaling_available_governors, > user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), XC_HYPERCALL_BUFFER_BOUNCE_BOTH); > - > - bool has_num = user_para->cpu_num && > - user_para->freq_num && > - user_para->gov_num; > + unsigned int in_gov_num = user_para->gov_num; > + bool has_num = user_para->cpu_num && user_para->freq_num; > > if ( has_num ) I think there's an issue here, this condition used to be true if `gov_num` was not 0, even if `cpu_num` and `freq_num` was 0. That's not the case anymore. Shouldn't `has_num` use also the value from `gov_num`? Do we actually need `has_num`? Thanks,
On 28.03.2025 11:51, Anthony PERARD wrote: > On Thu, Mar 27, 2025 at 02:32:04PM +0100, Jan Beulich wrote: >> HWP work made the cleanup of the "available governors" array >> conditional, neglecting the fact that the condition used may not be the > > I don't know why the cleanup was made conditional, as long as the bounce > buffer is declared with DECLARE_NAMED_HYPERCALL_BOUNCE(), > xc_hypercall_bounce_post() can be called without issue (even if > ..bounce_pre isn't used). Maybe it's all those "unlock_*" labels that is > misleading, a single "out" label which does the cleanup > unconditionally would be more than enough. Oh, yet more cleanup to do there (independently of course). >> 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. >> >> --- a/tools/libs/ctrl/xc_pm.c >> +++ b/tools/libs/ctrl/xc_pm.c >> @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc >> DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors, >> user_para->scaling_available_governors, >> user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), XC_HYPERCALL_BUFFER_BOUNCE_BOTH); >> - >> - bool has_num = user_para->cpu_num && >> - user_para->freq_num && >> - user_para->gov_num; >> + unsigned int in_gov_num = user_para->gov_num; >> + bool has_num = user_para->cpu_num && user_para->freq_num; >> >> if ( has_num ) > > I think there's an issue here, this condition used to be true if > `gov_num` was not 0, even if `cpu_num` and `freq_num` was 0. That's not > the case anymore. Shouldn't `has_num` use also the value from > `gov_num`? Do we actually need `has_num`? Raising this question is where all of this started: https://lists.xen.org/archives/html/xen-devel/2025-03/msg01870.html. IOW with Penny's change I think the need for has_num will disappear, the latest. At this point, requesting the governors being optional, only ->gov_num shouldn't affect has_num. Once requesting frequencies becomes optional too, has_num would become a mere alias of ->cpu_num. Jan
--- a/tools/libs/ctrl/xc_pm.c +++ b/tools/libs/ctrl/xc_pm.c @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors, user_para->scaling_available_governors, user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), XC_HYPERCALL_BUFFER_BOUNCE_BOTH); - - bool has_num = user_para->cpu_num && - user_para->freq_num && - user_para->gov_num; + unsigned int in_gov_num = user_para->gov_num; + bool has_num = user_para->cpu_num && user_para->freq_num; if ( has_num ) { if ( (!user_para->affected_cpus) || (!user_para->scaling_available_frequencies) || - (user_para->gov_num && !user_para->scaling_available_governors) ) + (in_gov_num && !user_para->scaling_available_governors) ) { errno = EINVAL; return -1; } - if ( xc_hypercall_bounce_pre(xch, affected_cpus) ) - goto unlock_1; - if ( xc_hypercall_bounce_pre(xch, scaling_available_frequencies) ) + ret = xc_hypercall_bounce_pre(xch, affected_cpus); + if ( ret ) + return ret; + ret = xc_hypercall_bounce_pre(xch, scaling_available_frequencies); + if ( ret ) goto unlock_2; - if ( user_para->gov_num && - xc_hypercall_bounce_pre(xch, scaling_available_governors) ) + if ( in_gov_num ) + ret = xc_hypercall_bounce_pre(xch, scaling_available_governors); + if ( ret ) goto unlock_3; set_xen_guest_handle(sys_para->affected_cpus, affected_cpus); set_xen_guest_handle(sys_para->scaling_available_frequencies, scaling_available_frequencies); - if ( user_para->gov_num ) + if ( in_gov_num ) set_xen_guest_handle(sys_para->scaling_available_governors, scaling_available_governors); } @@ -246,7 +247,7 @@ int xc_get_cpufreq_para(xc_interface *xc sysctl.u.pm_op.cpuid = cpuid; sys_para->cpu_num = user_para->cpu_num; sys_para->freq_num = user_para->freq_num; - sys_para->gov_num = user_para->gov_num; + sys_para->gov_num = in_gov_num; ret = xc_sysctl(xch, &sysctl); if ( ret ) @@ -256,12 +257,11 @@ int xc_get_cpufreq_para(xc_interface *xc user_para->cpu_num = sys_para->cpu_num; user_para->freq_num = sys_para->freq_num; user_para->gov_num = sys_para->gov_num; - ret = -errno; } if ( has_num ) goto unlock_4; - goto unlock_1; + return ret; } else { @@ -298,13 +298,13 @@ int xc_get_cpufreq_para(xc_interface *xc } unlock_4: - if ( user_para->gov_num ) + if ( in_gov_num ) xc_hypercall_bounce_post(xch, scaling_available_governors); unlock_3: xc_hypercall_bounce_post(xch, scaling_available_frequencies); unlock_2: xc_hypercall_bounce_post(xch, affected_cpus); -unlock_1: + return ret; }
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>