Re: [PATCH v1 02/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

Hi,

On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
>  xen/include/xsm/dummy.h | 7 +++++++
>  xen/include/xsm/xsm.h   | 9 +++++++++
>  xen/xsm/dummy.c         | 2 ++
>  xen/xsm/flask/hooks.c   | 4 ++++
>  4 files changed, 22 insertions(+)
>
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index a8d06de6b0..afc54a0b2f 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -180,11 +180,18 @@ static XSM_INLINE int cf_check xsm_domctl(
>      }
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>  {
>      XSM_ASSERT_ACTION(XSM_PRIV);
>      return xsm_default_action(action, current->domain, NULL);
>  }
> +#else
> +static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

Doesn't this need to be -ENOSYS instead?

I'd put the ifdefs inside the function (making the signature common) and
then have the body ifdef-ed. But rather than that, I suspect the `else` branch
can just go away because...

>  
>  static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
>  {
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 8c33b055fc..276507b515 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -60,7 +60,9 @@ struct xsm_ops {
>      int (*sysctl_scheduler_op)(int op);
>      int (*set_target)(struct domain *d, struct domain *e);
>      int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
> +#ifdef CONFIG_SYSCTL
>      int (*sysctl)(int cmd);
> +#endif
>      int (*readconsole)(uint32_t clear);

... either you remove this field or make the dummy handler. Doing both seems
redundant.

The dummy handler would return -ENOTSUPP, so the field is benign (in which case
I don't really get why it must go). But if the field is gone, there's no need
for the handler to begin with.

All in all, removing the else branch in xsm_sysctl would make everything
consistent. Same in the files below.

Also, you may want to add the readconsole hook (and its handler) since that's
a specific sysctl that would also be disabled by !CONFIG_SYSCTL.

>  
>      int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
> @@ -259,10 +261,17 @@ static inline int xsm_domctl(xsm_default_t def, struct domain *d,
>      return alternative_call(xsm_ops.domctl, d, cmd, ssidref);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static inline int xsm_sysctl(xsm_default_t def, int cmd)
>  {
>      return alternative_call(xsm_ops.sysctl, cmd);
>  }
> +#else
> +static inline int xsm_sysctl(xsm_default_t def, int cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

Same as above

>  
>  static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
>  {

> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index ce6fbdc6c5..0a5fc06bbf 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c

Same remarks here as in the header.

> @@ -22,7 +22,9 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
>      .sysctl_scheduler_op           = xsm_sysctl_scheduler_op,
>      .set_target                    = xsm_set_target,
>      .domctl                        = xsm_domctl,
> +#ifdef CONFIG_SYSCTL
>      .sysctl                        = xsm_sysctl,
> +#endif
>      .readconsole                   = xsm_readconsole,
>  
>      .evtchn_unbound                = xsm_evtchn_unbound,
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 389707a164..7c5e7f5879 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -856,6 +856,7 @@ static int cf_check flask_domctl(struct domain *d, unsigned int cmd,
>      }
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static int cf_check flask_sysctl(int cmd)
>  {
>      switch ( cmd )
> @@ -933,6 +934,7 @@ static int cf_check flask_sysctl(int cmd)
>          return avc_unknown_permission("sysctl", cmd);
>      }
>  }
> +#endif
>  
>  static int cf_check flask_readconsole(uint32_t clear)
>  {
> @@ -1884,7 +1886,9 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
>      .sysctl_scheduler_op = flask_sysctl_scheduler_op,
>      .set_target = flask_set_target,
>      .domctl = flask_domctl,
> +#ifdef CONFIG_SYSCTL
>      .sysctl = flask_sysctl,
> +#endif
>      .readconsole = flask_readconsole,

readconsole ought to be included, imo. And its handler wiped out as well.

>  
>      .evtchn_unbound = flask_evtchn_unbound,

Cheers,
Alejandro