| Message ID | 91bb3d4e-46e6-c210-2610-c4771996adfb@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxc: remove / adjust xc_get_cpufreq_para()'s BUILD_BUG_ON()s | expand |
On 23.08.23 15:47, Jan Beulich wrote: > The full structures cannot match in layout, as soon as a 32-bit tool > stack build comes into play. But it also doesn't need to; the part of > the layouts that needs to match is merely the union that we memcpy() > from the sysctl structure to the xc one. Keep (in adjusted form) only > the relevant ones. > > Since the whole block needs touching anyway, move it closer to the > respective memcpy() and use a wrapper macro to limit verbosity. > > Fixes: 2381dfab083f ("xen/sysctl: Nest cpufreq scaling options") > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
On Wed, Aug 23, 2023 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: > > The full structures cannot match in layout, as soon as a 32-bit tool > stack build comes into play. But it also doesn't need to; the part of > the layouts that needs to match is merely the union that we memcpy() > from the sysctl structure to the xc one. Keep (in adjusted form) only > the relevant ones. > > Since the whole block needs touching anyway, move it closer to the > respective memcpy() and use a wrapper macro to limit verbosity. > > Fixes: 2381dfab083f ("xen/sysctl: Nest cpufreq scaling options") > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/tools/libs/ctrl/xc_pm.c > +++ b/tools/libs/ctrl/xc_pm.c > @@ -248,45 +248,6 @@ int xc_get_cpufreq_para(xc_interface *xc > sys_para->freq_num = user_para->freq_num; > sys_para->gov_num = user_para->gov_num; > > - /* Sanity check struct layout */ > - BUILD_BUG_ON(sizeof(*user_para) != sizeof(*sys_para)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), cpu_num) != > - offsetof(typeof(*sys_para), cpu_num)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), freq_num) != > - offsetof(typeof(*sys_para), freq_num)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), gov_num) != > - offsetof(typeof(*sys_para), gov_num)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), affected_cpus) != > - offsetof(typeof(*sys_para), affected_cpus)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_available_frequencies) != > - offsetof(typeof(*sys_para), scaling_available_frequencies)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_available_governors) != > - offsetof(typeof(*sys_para), scaling_available_governors)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_driver) != > - offsetof(typeof(*sys_para), scaling_driver)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_cur_freq) != > - offsetof(typeof(*sys_para), cpuinfo_cur_freq)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_max_freq) != > - offsetof(typeof(*sys_para), cpuinfo_max_freq)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_min_freq) != > - offsetof(typeof(*sys_para), cpuinfo_min_freq)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_cur_freq) != > - offsetof(typeof(*sys_para), u.s.scaling_cur_freq)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_governor) != > - offsetof(typeof(*sys_para), u.s.scaling_governor)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_max_freq) != > - offsetof(typeof(*sys_para), u.s.scaling_max_freq)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_min_freq) != > - offsetof(typeof(*sys_para), u.s.scaling_min_freq)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.u.userspace) != > - offsetof(typeof(*sys_para), u.s.u.userspace)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.u.ondemand) != > - offsetof(typeof(*sys_para), u.s.u.ondemand)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), u.cppc_para) != > - offsetof(typeof(*sys_para), u.cppc_para)); > - BUILD_BUG_ON(offsetof(typeof(*user_para), turbo_enabled) != > - offsetof(typeof(*sys_para), turbo_enabled)); > - > ret = xc_sysctl(xch, &sysctl); > if ( ret ) > { > @@ -316,6 +277,22 @@ int xc_get_cpufreq_para(xc_interface *xc > BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != > sizeof(((struct xen_get_cpufreq_para *)0)->u)); This check... > + /* Sanity check layout of the union subject to memcpy() below. */ > + BUILD_BUG_ON(sizeof(user_para->u) != sizeof(sys_para->u)); And this check are the same? Your newer one is nicer, so maybe drop the first? > +#define CHK_FIELD(fld) \ > + BUILD_BUG_ON(offsetof(typeof(user_para->u), fld) != \ > + offsetof(typeof(sys_para->u), fld)) > + > + CHK_FIELD(s.scaling_cur_freq); > + CHK_FIELD(s.scaling_governor); > + CHK_FIELD(s.scaling_max_freq); > + CHK_FIELD(s.scaling_min_freq); > + CHK_FIELD(s.u.userspace); > + CHK_FIELD(s.u.ondemand); > + CHK_FIELD(cppc_para); > + > +#undef CHK_FIELD > + > memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u)); > } > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> Sorry about the breakage. I started looking at this locally, but you beat me. Thanks, Jason
On 23.08.2023 15:57, Jason Andryuk wrote: > On Wed, Aug 23, 2023 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >> @@ -316,6 +277,22 @@ int xc_get_cpufreq_para(xc_interface *xc >> BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != >> sizeof(((struct xen_get_cpufreq_para *)0)->u)); > > This check... > >> + /* Sanity check layout of the union subject to memcpy() below. */ >> + BUILD_BUG_ON(sizeof(user_para->u) != sizeof(sys_para->u)); > > And this check are the same? Your newer one is nicer, so maybe drop the first? Oh, indeed. Will do (and Jürgen, I'll assume this won't invalidate your R-b). >> +#define CHK_FIELD(fld) \ >> + BUILD_BUG_ON(offsetof(typeof(user_para->u), fld) != \ >> + offsetof(typeof(sys_para->u), fld)) >> + >> + CHK_FIELD(s.scaling_cur_freq); >> + CHK_FIELD(s.scaling_governor); >> + CHK_FIELD(s.scaling_max_freq); >> + CHK_FIELD(s.scaling_min_freq); >> + CHK_FIELD(s.u.userspace); >> + CHK_FIELD(s.u.ondemand); >> + CHK_FIELD(cppc_para); >> + >> +#undef CHK_FIELD >> + >> memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u)); >> } >> > > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> Thanks. Jan
On 23.08.23 16:07, Jan Beulich wrote: > On 23.08.2023 15:57, Jason Andryuk wrote: >> On Wed, Aug 23, 2023 at 9:47 AM Jan Beulich <jbeulich@suse.com> wrote: >>> @@ -316,6 +277,22 @@ int xc_get_cpufreq_para(xc_interface *xc >>> BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != >>> sizeof(((struct xen_get_cpufreq_para *)0)->u)); >> >> This check... >> >>> + /* Sanity check layout of the union subject to memcpy() below. */ >>> + BUILD_BUG_ON(sizeof(user_para->u) != sizeof(sys_para->u)); >> >> And this check are the same? Your newer one is nicer, so maybe drop the first? > > Oh, indeed. Will do (and Jürgen, I'll assume this won't invalidate your > R-b). Correct. Juergen
--- a/tools/libs/ctrl/xc_pm.c +++ b/tools/libs/ctrl/xc_pm.c @@ -248,45 +248,6 @@ int xc_get_cpufreq_para(xc_interface *xc sys_para->freq_num = user_para->freq_num; sys_para->gov_num = user_para->gov_num; - /* Sanity check struct layout */ - BUILD_BUG_ON(sizeof(*user_para) != sizeof(*sys_para)); - BUILD_BUG_ON(offsetof(typeof(*user_para), cpu_num) != - offsetof(typeof(*sys_para), cpu_num)); - BUILD_BUG_ON(offsetof(typeof(*user_para), freq_num) != - offsetof(typeof(*sys_para), freq_num)); - BUILD_BUG_ON(offsetof(typeof(*user_para), gov_num) != - offsetof(typeof(*sys_para), gov_num)); - BUILD_BUG_ON(offsetof(typeof(*user_para), affected_cpus) != - offsetof(typeof(*sys_para), affected_cpus)); - BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_available_frequencies) != - offsetof(typeof(*sys_para), scaling_available_frequencies)); - BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_available_governors) != - offsetof(typeof(*sys_para), scaling_available_governors)); - BUILD_BUG_ON(offsetof(typeof(*user_para), scaling_driver) != - offsetof(typeof(*sys_para), scaling_driver)); - BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_cur_freq) != - offsetof(typeof(*sys_para), cpuinfo_cur_freq)); - BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_max_freq) != - offsetof(typeof(*sys_para), cpuinfo_max_freq)); - BUILD_BUG_ON(offsetof(typeof(*user_para), cpuinfo_min_freq) != - offsetof(typeof(*sys_para), cpuinfo_min_freq)); - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_cur_freq) != - offsetof(typeof(*sys_para), u.s.scaling_cur_freq)); - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_governor) != - offsetof(typeof(*sys_para), u.s.scaling_governor)); - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_max_freq) != - offsetof(typeof(*sys_para), u.s.scaling_max_freq)); - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.scaling_min_freq) != - offsetof(typeof(*sys_para), u.s.scaling_min_freq)); - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.u.userspace) != - offsetof(typeof(*sys_para), u.s.u.userspace)); - BUILD_BUG_ON(offsetof(typeof(*user_para), u.s.u.ondemand) != - offsetof(typeof(*sys_para), u.s.u.ondemand)); - BUILD_BUG_ON(offsetof(typeof(*user_para), u.cppc_para) != - offsetof(typeof(*sys_para), u.cppc_para)); - BUILD_BUG_ON(offsetof(typeof(*user_para), turbo_enabled) != - offsetof(typeof(*sys_para), turbo_enabled)); - ret = xc_sysctl(xch, &sysctl); if ( ret ) { @@ -316,6 +277,22 @@ int xc_get_cpufreq_para(xc_interface *xc BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != sizeof(((struct xen_get_cpufreq_para *)0)->u)); + /* Sanity check layout of the union subject to memcpy() below. */ + BUILD_BUG_ON(sizeof(user_para->u) != sizeof(sys_para->u)); +#define CHK_FIELD(fld) \ + BUILD_BUG_ON(offsetof(typeof(user_para->u), fld) != \ + offsetof(typeof(sys_para->u), fld)) + + CHK_FIELD(s.scaling_cur_freq); + CHK_FIELD(s.scaling_governor); + CHK_FIELD(s.scaling_max_freq); + CHK_FIELD(s.scaling_min_freq); + CHK_FIELD(s.u.userspace); + CHK_FIELD(s.u.ondemand); + CHK_FIELD(cppc_para); + +#undef CHK_FIELD + memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u)); }
The full structures cannot match in layout, as soon as a 32-bit tool stack build comes into play. But it also doesn't need to; the part of the layouts that needs to match is merely the union that we memcpy() from the sysctl structure to the xc one. Keep (in adjusted form) only the relevant ones. Since the whole block needs touching anyway, move it closer to the respective memcpy() and use a wrapper macro to limit verbosity. Fixes: 2381dfab083f ("xen/sysctl: Nest cpufreq scaling options") Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com>