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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.09.18 at 11:22, <talons.lee@xxxxxxxxx> 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.

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

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

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

> + * 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.

> +/*
> + * 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.

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

Jan



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