Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

["Xin Li (Talons)" <xin.li@citrix.com>]

Thanks Jan.

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, October 2, 2018 5:33 PM
> To: Xin Li <talons.lee@gmail.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ming Lu
> <ming.lu@citrix.com>; Sergey Dyasli <sergey.dyasli@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Xin Li (Talons) <xin.li@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xen.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel
> de Graaf <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 29.09.18 at 11:22, <talons.lee@gmail.com> wrote:
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -48,7 +48,12 @@ void __xsm_action_mismatch_detected(void);
> >   * There is no xsm_default_t argument available, so the value from the
> assertion
> >   * is used to initialize the variable.
> >   */
> > +#ifdef CONFIG_XSM_SILO
> > +#define XSM_INLINE __attribute__ ((unused))
> 
> Please don't open-code __maybe_unused. Furthermore I question the
> dependency on CONFIG_XSM_SILO here: Afaict you want this for just the single
> new inclusion site, without affecting any others.

OK, use __maybe_unused directly and remove this #ifdef.

> 
> > --- a/xen/include/xsm/xsm.h
> > +++ b/xen/include/xsm/xsm.h
> > @@ -733,6 +733,12 @@ extern const unsigned char
> > xsm_flask_init_policy[];  extern const unsigned int
> > xsm_flask_init_policy_size;  #endif
> >
> > +#ifdef CONFIG_XSM_SILO
> > +extern void silo_init(void);
> > +#else
> > +static inline void silo_init(void) {} #endif
> 
> Along the lines of one of my remarks on patch 1, I think you would better get
> away without the inline variant, by adding suitable #ifdef-s to eliminate the risk
> of wrong use of the new enumerator.

I can remove the inline function, and add #idef in xsm_core.c
But, this way does not match the current code style(like flask_init()).
So I would like to keep this way.

> 
> > --- a/xen/xsm/dummy.c
> > +++ b/xen/xsm/dummy.c
> > @@ -11,7 +11,6 @@
> >   */
> >
> >  #define XSM_NO_WRAPPERS
> > -#define XSM_INLINE /* */
> >  #include <xsm/dummy.h>
> 
> The change looks unmotivated here, perhaps because of the questionable
> CONFIG_XSM_SILO dependency further up. That said, it looks as if the #define
> could go away currently, as being redundant with what dummy.h already has.
> That would then better be a small separate prereq patch imo.

This is intended, to avoid compile error, can't refine XSM_INLINE.
Do you mean a separate patch to just remove this line?

> 
> > --- /dev/null
> > +++ b/xen/xsm/silo.c
> > @@ -0,0 +1,109 @@
> >
> +/***************************************************************
> *****
> > +**********
> > + * xsm/silo.c
> > + *
> > + * SILO module for XSM(Xen Security Modules)
> 
> Would you mind adding the missing blank here?

Do you mean add one space after XSM?
* SILO module for XSM (Xen Security Modules)

> 
> > + * Copyright (c) 2018 Citrix Systems Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > +WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY
> > +or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > +License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > +along with
> > + * this program; If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#define XSM_NO_WRAPPERS
> > +
> > +#include <xsm/dummy.h>
> 
> Please switch around the blank and the #define lines, matching how dummy.c is
> written. This helps readers understand that the #define is specifically in
> preparation of the #include.

OK. Remove the line before #include.

 
> > +/*
> > + * Check if inter-domain communication is allowed.
> > + * Return true when pass check.
> > + */
> > +static bool silo_mode_dom_check(const struct domain *ldom,
> > +                                const struct domain *rdom) {
> > +    const struct domain *cur_dom = current->domain;
> 
> We commonly call such variables currd.

OK. Rename this var.

> 
> > +    return (is_control_domain(cur_dom) || is_control_domain(ldom) ||
> > +            is_control_domain(rdom) || ldom == rdom); }
> > +
> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> > +                               domid_t id2) {
> > +    int rc = -EPERM;
> > +    struct domain *d2 = rcu_lock_domain_by_any_id(id2);
> > +
> > +    if ( d2 == NULL )
> 
> We generally try to use the shorter !d2 in new code. But it's a matter of
> personal preference, I agree.

Thanks. I will just keep it.

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel