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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

Sorry for the late reply, got side-tracked for a while

> -----Original Message-----
> From: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> Sent: Thursday, March 13, 2025 8:03 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v1 03/19] xen/sysctl: wrap around
> XEN_SYSCTL_readconsole
>
> 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?
>

Because I wrapped the sysctl.c in the last commit.
If removing the else condition here, the compilation will fail on this commit.
So either I add #ifdef into read_console_ring function body, or in the last 
commit,
I draw back all these unnecessary else conditions, or combine all commits into 
one
Any preference? Or any other suggestion?

> >
> >  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