Re: [PATCH for-4.22] XSM: guard .sysctl() and .readconsole() hooks

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 6/18/26 10:50, Jan Beulich wrote:
> On 18.06.2026 14:51, Jan Beulich wrote:
> > On 18.06.2026 14:48, Daniel P. Smith wrote:
> > > On 6/18/26 8:42 AM, Jan Beulich wrote:
> > > > On 18.06.2026 14:34, Daniel P. Smith wrote:
> > > > > On 6/18/26 8:23 AM, Jan Beulich wrote:
> > > > > > On 18.06.2026 14:13, Andrew Cooper wrote:
> > > > > > > On 18/06/2026 12:32 pm, Jan Beulich wrote:
> > > > > > > > Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
> > > > > > > > the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
> > > > > > > > hypervisors.
> > > > > > > >
> > > > > > > > While moving the #ifdef for the corresponding xsm_*() wrappers, also move
> > > > > > > > those for xsm_page_offline() (where the hook pointer field already is
> > > > > > > > suitably guarded).
> > > > > > > >
> > > > > > > > Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
> > > > > > > > Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
> > > > > > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > > > Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
> > > > > > > too, which should be able to spot things like this.
> > > > > > >
> > > > > > > This is a regression vs 4.21, so does need including.
> > > > > > Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
> > > > > >
> > > > > > > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, although...
> > > > > > Thanks.
> > > > > >
> > > > > > > > --- a/xen/include/xsm/xsm.h
> > > > > > > > +++ b/xen/include/xsm/xsm.h
> > > > > > > > @@ -61,8 +61,10 @@ struct xsm_ops {
> > > > > > > >     #endif
> > > > > > > >         int (*set_target)(struct domain *d, struct domain *e);
> > > > > > > >         int (*domctl)(struct domain *d, struct xen_domctl *op);
> > > > > > > > +#ifdef CONFIG_SYSCTL
> > > > > > > >         int (*sysctl)(int cmd);
> > > > > > > >         int (*readconsole)(uint32_t clear);
> > > > > > > > +#endif
> > > > > > > ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
> > > > > > >
> > > > > > > I know it will grow the diff, but can we see about collecting them into
> > > > > > > a single region, and in dummy_ops too?  It will shrink the overall
> > > > > > > result, and the order of pointers in this ops structure is uninteresting.
> > > > > > I have a far more consolidating patch in the works, which is how I actually
> > > > > > noticed the issue. I'd prefer to keep things as simple as possible here.
> > > > > By the way, I was going back through this and notices that they are not
> > > > > ifdef out in xsm/dummy.h. Are we relying on them being inlines to ensure
> > > > > that they do not result in dead code?
> > > > I think so. Maybe it's unhelpful that we have
> > > To be clear, you want to leave as is for now (or at least pick up in
> > > another patch)?
> > It's up to you - I can add the #ifdef there, but it doesn't look to strictly
> > be needed.
It's not needed and will create minimal but extra churn on back port, so 
we can forego for now.
> Can you please make explicit which way you'd like it in order to provide an
> ack, so this bug fix can go in?
Acked-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>