Re: [PATCH v1 03/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

Hi,

Ok, so readconsole is done here. I see how if you're also removing the console
handler for the sysctl that's a bit unwiledly to do in one go.

I think my earlier remarks still hold in terms of removal of else branches of
ifdefs.

On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> The following functions is to deal with XEN_SYSCTL_readconsole sub-op, and
> shall be wrapped:
> - xsm_readconsole
> - read_console_ring
>
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
>  xen/drivers/char/console.c |  2 ++
>  xen/include/xen/console.h  |  8 ++++++++
>  xen/include/xsm/dummy.h    | 11 ++++++++---
>  xen/include/xsm/xsm.h      | 11 ++++++++---
>  xen/xsm/dummy.c            |  2 +-
>  xen/xsm/flask/hooks.c      |  4 ++--
>  6 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 2f028c5d44..6e4f3c4659 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -336,6 +336,7 @@ static void conring_puts(const char *str, size_t len)
>          conringc = conringp - conring_size;
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  long read_console_ring(struct xen_sysctl_readconsole *op)
>  {
>      XEN_GUEST_HANDLE_PARAM(char) str;
> @@ -378,6 +379,7 @@ long read_console_ring(struct xen_sysctl_readconsole *op)
>  
>      return 0;
>  }
> +#endif /* CONFIG_SYSCTL */
>  
>  
>  /*
> diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> index 83cbc9fbda..e7d5063d82 100644
> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -7,12 +7,20 @@
>  #ifndef __CONSOLE_H__
>  #define __CONSOLE_H__
>  
> +#include <xen/errno.h>
>  #include <xen/inttypes.h>
>  #include <xen/ctype.h>
>  #include <public/xen.h>
>  
>  struct xen_sysctl_readconsole;

That forward declaration should probably be inside the ifdef

> +#ifdef CONFIG_SYSCTL
>  long read_console_ring(struct xen_sysctl_readconsole *op);
> +#else
> +static inline long read_console_ring(struct xen_sysctl_readconsole *op)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

This is only called from sysctl.c, which will be compiled out. Why is the else
needed?

>  
>  void console_init_preirq(void);
>  void console_init_ring(void);
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index afc54a0b2f..35d084aca7 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -186,18 +186,23 @@ 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);
>  }
> +
> +static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t 
> clear)
> +{
> +    XSM_ASSERT_ACTION(XSM_HOOK);
> +    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
>  
>  static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t 
> clear)
>  {
> -    XSM_ASSERT_ACTION(XSM_HOOK);
> -    return xsm_default_action(action, current->domain, NULL);
> +    return -EOPNOTSUPP;
>  }
> +#endif /* CONFIG_SYSCTL */
>  
>  static XSM_INLINE int cf_check xsm_alloc_security_domain(struct domain *d)
>  {
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 276507b515..d322740de1 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -62,8 +62,8 @@ struct xsm_ops {
>      int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
>  #ifdef CONFIG_SYSCTL
>      int (*sysctl)(int cmd);
> -#endif
>      int (*readconsole)(uint32_t clear);
> +#endif
>  
>      int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
>      int (*evtchn_interdomain)(struct domain *d1, struct evtchn *chn1,
> @@ -266,17 +266,22 @@ static inline int xsm_sysctl(xsm_default_t def, int cmd)
>  {
>      return alternative_call(xsm_ops.sysctl, cmd);
>  }
> +
> +static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
> +{
> +    return alternative_call(xsm_ops.readconsole, clear);
> +}
>  #else
>  static inline int xsm_sysctl(xsm_default_t def, int cmd)
>  {
>      return -EOPNOTSUPP;
>  }
> -#endif
>  
>  static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
>  {
> -    return alternative_call(xsm_ops.readconsole, clear);
> +    return -EOPNOTSUPP;
>  }
> +#endif
>  
>  static inline int xsm_evtchn_unbound(
>      xsm_default_t def, struct domain *d1, struct evtchn *chn, domid_t id2)
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index 0a5fc06bbf..4c97db0c48 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -24,8 +24,8 @@ static const struct xsm_ops __initconst_cf_clobber 
> dummy_ops = {
>      .domctl                        = xsm_domctl,
>  #ifdef CONFIG_SYSCTL
>      .sysctl                        = xsm_sysctl,
> -#endif
>      .readconsole                   = xsm_readconsole,
> +#endif
>  
>      .evtchn_unbound                = xsm_evtchn_unbound,
>      .evtchn_interdomain            = xsm_evtchn_interdomain,
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 7c5e7f5879..7c46657d97 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -934,7 +934,6 @@ static int cf_check flask_sysctl(int cmd)
>          return avc_unknown_permission("sysctl", cmd);
>      }
>  }
> -#endif
>  
>  static int cf_check flask_readconsole(uint32_t clear)
>  {
> @@ -945,6 +944,7 @@ static int cf_check flask_readconsole(uint32_t clear)
>  
>      return domain_has_xen(current->domain, perms);
>  }
> +#endif /* CONFIG_SYSCTL */
>  
>  static inline uint32_t resource_to_perm(uint8_t access)
>  {
> @@ -1888,8 +1888,8 @@ static const struct xsm_ops __initconst_cf_clobber 
> flask_ops = {
>      .domctl = flask_domctl,
>  #ifdef CONFIG_SYSCTL
>      .sysctl = flask_sysctl,
> -#endif
>      .readconsole = flask_readconsole,
> +#endif
>  
>      .evtchn_unbound = flask_evtchn_unbound,
>      .evtchn_interdomain = flask_evtchn_interdomain,

Otherwise, same remarks as in the sysctl hooks for xsm.

Cheers,
Alejandro