XSM SILO boot time spew

XSM SILO boot time spew

[Andrew Cooper]

Hello,

I've noticed that the SILO code causes the following debug spew:

(XEN) XSM Framework v1.0.0 initialized
(XEN) Initialising XSM SILO mode
(XEN) dummy.c:31: Had to override the security_domaininfo security
operation with the dummy one.
(XEN) dummy.c:32: Had to override the domain_create security operation
with the dummy one.
...
(XEN) dummy.c:158: Had to override the xen_version security operation
with the dummy one.
(XEN) dummy.c:159: Had to override the domain_resource_map security
operation with the dummy one.
(XEN) microcode: CPU0 updated from revision 0x1a to 0x25, date = 2018-04-02
(XEN) xstate: size: 0x340 and states: 0x7

which is a side effect of silo_xsm_ops only implementing a few of the
hooks, falling back to dummy for the rest.

Presumably we don't want to special case SILO when overriding the
hooks.  Would having silo_init() explicitly copy from dummy be ok?

~Andrew

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

Re: XSM SILO boot time spew

[Xin Li (Talons)]

In patchset v4, we call register_xsm() to setup silo module.
This debug log is to check if some ops not overrided by the module.
I thought this is OK, since the log level is debug.

I think calling register_xsm() is good,
if we do want to suppress this debug log explicitly,
we can check if ops eqauls silo_xsm_ops in macro set_to_dummy_if_null().

The follow diff shows what I am suggesting, is it OK?

diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 3b192b5c31..b94fc43961 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -734,6 +734,7 @@ extern const unsigned int xsm_flask_init_policy_size;
 #endif
 
 #ifdef CONFIG_XSM_SILO
+extern struct xsm_operations silo_xsm_ops;
 extern void silo_init(void);
 #else
 static inline void silo_init(void) {}
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 06a674fad0..5af990514f 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -15,12 +15,20 @@
 
 struct xsm_operations dummy_xsm_ops;
 
+#ifdef CONFIG_XSM_SILO
+#define check_xsm_module(ops)                                          \
+    if (ops != &dummy_xsm_ops && ops != &silo_xsm_ops)
+#else
+#define check_xsm_module(ops)                                          \
+    if (ops != &dummy_xsm_ops)
+#endif
+
 #define set_to_dummy_if_null(ops, function)                            \
     do {                                                               \
         if ( !ops->function )                                          \
         {                                                              \
             ops->function = xsm_##function;                            \
-            if (ops != &dummy_xsm_ops)                                 \
+            check_xsm_module(ops)                                      \
                 dprintk(XENLOG_DEBUG, "Had to override the " #function \
                     " security operation with the dummy one.\n");      \
         }                                                              \
diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
index 4850756a3d..d2e6724e26 100644
--- a/xen/xsm/silo.c
+++ b/xen/xsm/silo.c
@@ -81,7 +81,7 @@ static int silo_grant_copy(struct domain *d1, struct domain *d2)
     return -EPERM;
 }
 
-static struct xsm_operations silo_xsm_ops = {
+struct xsm_operations silo_xsm_ops = {
     .evtchn_unbound = silo_evtchn_unbound,
     .evtchn_interdomain = silo_evtchn_interdomain,
     .grant_mapref = silo_grant_mapref,

________________________________________
From: Andrew Cooper
Sent: Wednesday, October 31, 2018 8:35 PM
To: Xen-devel List
Cc: Daniel De Graaf; Xin Li (Talons)
Subject: XSM SILO boot time spew

Hello,

I've noticed that the SILO code causes the following debug spew:

(XEN) XSM Framework v1.0.0 initialized
(XEN) Initialising XSM SILO mode
(XEN) dummy.c:31: Had to override the security_domaininfo security
operation with the dummy one.
(XEN) dummy.c:32: Had to override the domain_create security operation
with the dummy one.
...
(XEN) dummy.c:158: Had to override the xen_version security operation
with the dummy one.
(XEN) dummy.c:159: Had to override the domain_resource_map security
operation with the dummy one.
(XEN) microcode: CPU0 updated from revision 0x1a to 0x25, date = 2018-04-02
(XEN) xstate: size: 0x340 and states: 0x7

which is a side effect of silo_xsm_ops only implementing a few of the
hooks, falling back to dummy for the rest.

Presumably we don't want to special case SILO when overriding the
hooks.  Would having silo_init() explicitly copy from dummy be ok?

~Andrew

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

Re: XSM SILO boot time spew

[Daniel De Graaf]

On 10/31/2018 11:19 PM, Xin Li (Talons) wrote:
> In patchset v4, we call register_xsm() to setup silo module.
> This debug log is to check if some ops not overrided by the module.
> I thought this is OK, since the log level is debug.
> 
> I think calling register_xsm() is good,
> if we do want to suppress this debug log explicitly,
> we can check if ops eqauls silo_xsm_ops in macro set_to_dummy_if_null().
> 
> The follow diff shows what I am suggesting, is it OK?

If SILO is a good example of what a potential third XSM module would look
like, it's probably better to just remove the printing and allow the
dummy module's hooks to fill in any null values in the xsm_operations
structure.  The printing part was written with FLASK and ACM in mind,
which both intended to hook everything and might add new hooks without
changing the other.

Another possible solution would be to add a bool parameter to register_xsm
that disables the warnings instead of checking the pointer value, but that
feels like overkill to me; we still only have two XSM modules.

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

Re: XSM SILO boot time spew

[Xin Li (Talons)]

> -----Original Message-----
> From: Daniel De Graaf [mailto:dgdegra@tycho.nsa.gov]
> Sent: Thursday, November 8, 2018 1:53 AM
> To: Xin Li (Talons) <xin.li@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>
> Cc: Xen-devel List <xen-devel@lists.xen.org>
> Subject: Re: XSM SILO boot time spew

> If SILO is a good example of what a potential third XSM module would look like,
> it's probably better to just remove the printing and allow the dummy module's
> hooks to fill in any null values in the xsm_operations structure.  The printing
> part was written with FLASK and ACM in mind, which both intended to hook
> everything and might add new hooks without changing the other.
> 
> Another possible solution would be to add a bool parameter to register_xsm
> that disables the warnings instead of checking the pointer value, but that feels
> like overkill to me; we still only have two XSM modules.

OK. I will just remove the printing.

Best regards

Xin(Talons) Li
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: XSM SILO boot time spew

[Jan Beulich]

>>> On 07.11.18 at 18:52, <dgdegra@tycho.nsa.gov> wrote:
> On 10/31/2018 11:19 PM, Xin Li (Talons) wrote:
>> In patchset v4, we call register_xsm() to setup silo module.
>> This debug log is to check if some ops not overrided by the module.
>> I thought this is OK, since the log level is debug.
>> 
>> I think calling register_xsm() is good,
>> if we do want to suppress this debug log explicitly,
>> we can check if ops eqauls silo_xsm_ops in macro set_to_dummy_if_null().
>> 
>> The follow diff shows what I am suggesting, is it OK?
> 
> If SILO is a good example of what a potential third XSM module would look
> like, it's probably better to just remove the printing and allow the
> dummy module's hooks to fill in any null values in the xsm_operations
> structure.  The printing part was written with FLASK and ACM in mind,
> which both intended to hook everything and might add new hooks without
> changing the other.
> 
> Another possible solution would be to add a bool parameter to register_xsm
> that disables the warnings instead of checking the pointer value, but that
> feels like overkill to me; we still only have two XSM modules.

Why? Retaining the log message for the FLASK case seems quite
desirable, and comparing pointers to special ops structures seems
quite obviously worse to me. Of course a patch to drop the logging
altogether was already posted, so you're free to ack that one and
the discussion would be ended.

Jan



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

Re: XSM SILO boot time spew

[Andrew Cooper]

On 08/11/2018 10:05, Jan Beulich wrote:
>>>> On 07.11.18 at 18:52, <dgdegra@tycho.nsa.gov> wrote:
>> On 10/31/2018 11:19 PM, Xin Li (Talons) wrote:
>>> In patchset v4, we call register_xsm() to setup silo module.
>>> This debug log is to check if some ops not overrided by the module.
>>> I thought this is OK, since the log level is debug.
>>>
>>> I think calling register_xsm() is good,
>>> if we do want to suppress this debug log explicitly,
>>> we can check if ops eqauls silo_xsm_ops in macro set_to_dummy_if_null().
>>>
>>> The follow diff shows what I am suggesting, is it OK?
>> If SILO is a good example of what a potential third XSM module would look
>> like, it's probably better to just remove the printing and allow the
>> dummy module's hooks to fill in any null values in the xsm_operations
>> structure.  The printing part was written with FLASK and ACM in mind,
>> which both intended to hook everything and might add new hooks without
>> changing the other.
>>
>> Another possible solution would be to add a bool parameter to register_xsm
>> that disables the warnings instead of checking the pointer value, but that
>> feels like overkill to me; we still only have two XSM modules.
> Why? Retaining the log message for the FLASK case seems quite
> desirable, and comparing pointers to special ops structures seems
> quite obviously worse to me. Of course a patch to drop the logging
> altogether was already posted, so you're free to ack that one and
> the discussion would be ended.

So I was thinking about this.  I think I can arrange for the compiler to
check the full-ness of the dummy and flask ops, and remove all of this
runtime logic.  This would be by having an interestingly typed sentinel
at either end of the structure, then using a straight comma separated
list of functions in the initialiser.

Any change to the ops structure without a matching change in the flask
and dummy ops will result in a function pointer type failure.

~Andrew

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