xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] xsm: add a default policy to .init.data
@ 2016-06-29 15:09 Daniel De Graaf
  2016-06-30 13:45 ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel De Graaf @ 2016-06-29 15:09 UTC (permalink / raw)
  To: xen-devel; +Cc: Doug Goldstein, Daniel De Graaf, Ian Jackson, Jan Beulich

This adds a Kconfig option and support for including the XSM policy from
tools/flask/policy in the hypervisor so that the bootloader does not
need to provide a policy to get sane behavior from an XSM-enabled
hypervisor.  The policy provided by the bootloader, if present, will
override the built-in policy.

The XSM policy is not moved out of tools because that remains the
primary location for installing and configuring the policy.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---

Changes from v2 (dropped acks and reviewed-by):
 - Drop linker script changes, use python binary-to-C file script
 - Make the config option always include the policy if selected
 - Note the new conditional dependency on checkpolicy in INSTALL


 INSTALL                             |  6 +++++-
 docs/misc/xen-command-line.markdown | 16 +++++++++-------
 docs/misc/xsm-flask.txt             | 30 +++++++++++++++---------------
 xen/common/Kconfig                  | 16 ++++++++++++++++
 xen/xsm/flask/.gitignore            |  1 +
 xen/xsm/flask/Makefile              | 11 +++++++++++
 xen/xsm/flask/gen-policy.py         | 19 +++++++++++++++++++
 xen/xsm/xsm_core.c                  | 22 +++++++++++++++++++++-
 8 files changed, 97 insertions(+), 24 deletions(-)
 create mode 100644 xen/xsm/flask/.gitignore
 create mode 100644 xen/xsm/flask/gen-policy.py

diff --git a/INSTALL b/INSTALL
index 616a67a..703cfe7 100644
--- a/INSTALL
+++ b/INSTALL
@@ -272,7 +272,11 @@ PYTHON_PREFIX_ARG=
 The hypervisor may be build with XSM/Flask support, which can be changed
 by running:
 make -C xen menuconfig
-and enabling XSM/Flask in the 'Common Features' menu.
+and enabling XSM/Flask in the 'Common Features' menu.  By default, this
+selection will also enable compiling a built-in policy, which requires
+the SELinux policy compiler (checkpolicy) to build.  The location of
+this executable can be set using its environment variable.
+CHECKPOLICY=
 
 Do a build for coverage.
 coverage=y
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 2a088ca..5500242 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -712,13 +712,15 @@ enabled by running either:
   with untrusted guests.  If a policy is provided by the bootloader, it will be
   loaded; errors will be reported to the ring buffer but will not prevent
   booting.  The policy can be changed to enforcing mode using "xl setenforce".
-* `enforcing`: This requires a security policy to be provided by the bootloader
-  and will enter enforcing mode prior to the creation of domain 0.  If a valid
-  policy is not provided, the hypervisor will not continue booting.
-* `late`: This disables loading of the security policy from the bootloader.
-  FLASK will be enabled but will not enforce access controls until a policy is
-  loaded by a domain using "xl loadpolicy".  Once a policy is loaded, FLASK will
-  run in enforcing mode unless "xl setenforce" has changed that setting.
+* `enforcing`: This will cause the security server to enter enforcing mode prior
+  to the creation of domain 0.  If an valid policy is not provided by the
+  bootloader and no built-in policy is present, the hypervisor will not continue
+  booting.
+* `late`: This disables loading of the built-in security policy or the policy
+  provided by the bootloader.  FLASK will be enabled but will not enforce access
+  controls until a policy is loaded by a domain using "xl loadpolicy".  Once a
+  policy is loaded, FLASK will run in enforcing mode unless "xl setenforce" has
+  changed that setting.
 * `disabled`: This causes the XSM framework to revert to the dummy module.  The
   dummy module provides the same security policy as is used when compiling the
   hypervisor without support for XSM.  The xsm\_op hypercall can also be used to
diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt
index 2f42585..62f15dd 100644
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -141,21 +141,21 @@ only type enforcement is used and the user and role are set to system_u and
 system_r for all domains.
 
 The FLASK security framework is mostly configured using a security policy file.
-This policy file is not normally generated during the Xen build process because
-it relies on the SELinux compiler "checkpolicy"; run
-
-	make -C tools/flask/policy
-
-to compile the example policy included with Xen. The policy is generated from
-definition files under this directory. Most changes to security policy will
-involve creating or modifying modules found in tools/flask/policy/modules/.  The
-modules.conf file there defines what modules are enabled and has short
-descriptions of each module.
-
-The XSM policy file needs to be copied to /boot and loaded as a module by grub.
-The exact position of the module does not matter as long as it is after the Xen
-kernel; it is normally placed either just above the dom0 kernel or at the end.
-Once dom0 is running, the policy can be reloaded using "xl loadpolicy".
+It relies on the SELinux compiler "checkpolicy"; if this is available, the
+policy will be compiled as part of the tools build.  If hypervisor support for a
+built-in policy is enabled ("Compile Xen with a built-in security policy"), the
+policy will be built during the hypervisor build.
+
+The policy is generated from definition files in tools/flask/policy.  Most
+changes to security policy will involve creating or modifying modules found in
+tools/flask/policy/modules/.  The modules.conf file there defines what modules
+are enabled and has short descriptions of each module.
+
+If not using the built-in policy, the XSM policy file needs to be copied to
+/boot and loaded as a module by grub.  The exact position and filename of the
+module does not matter as long as it is after the Xen kernel; it is normally
+placed either just above the dom0 kernel or at the end.  Once dom0 is running,
+the policy can be reloaded using "xl loadpolicy".
 
 The example policy included with Xen demonstrates most of the features of FLASK
 that can be used without dom0 disaggregation. The main types for domUs are:
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index daab832..f15ea78 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -143,6 +143,22 @@ config FLASK_AVC_STATS
 
 	  If unsure, say Y.
 
+config XSM_POLICY
+	bool "Compile Xen with a built-in security policy"
+	default y
+	depends on XSM
+	---help---
+	  This includes a default XSM policy in the hypervisor so that the
+	  bootloader does not need to load a policy to get sane behavior from an
+	  XSM-enabled hypervisor.  If this is disabled, a policy must be
+	  provided by the bootloader or by Domain 0.  Even if this is enabled, a
+	  policy provided by the bootloader will override it.
+
+	  This requires that the SELinux policy compiler (checkpolicy) be
+	  available when compiling the hypervisor.
+
+	  If unsure, say Y.
+
 # Enable schedulers
 menu "Schedulers"
 	visible if EXPERT = "y"
diff --git a/xen/xsm/flask/.gitignore b/xen/xsm/flask/.gitignore
new file mode 100644
index 0000000..024edbe
--- /dev/null
+++ b/xen/xsm/flask/.gitignore
@@ -0,0 +1 @@
+/policy.c
diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index 12fc3a9..6335a72 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -27,6 +27,17 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
 $(AV_H_FILES): $(AV_H_DEPEND)
 	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
 
+obj-$(CONFIG_XSM_POLICY) += policy.o
+
+POLICY_SRC := $(XEN_ROOT)/tools/flask/policy/xenpolicy-$(XEN_FULLVERSION)
+
+policy.bin: FORCE
+	$(MAKE) -C $(XEN_ROOT)/tools/flask/policy
+	cmp -s $(POLICY_SRC) $@ || cp $(POLICY_SRC) $@
+
+policy.c: policy.bin gen-policy.py
+	$(PYTHON) gen-policy.py < $< > $@
+
 .PHONY: clean
 clean::
 	rm -f $(ALL_H_FILES) *.o $(DEPS)
diff --git a/xen/xsm/flask/gen-policy.py b/xen/xsm/flask/gen-policy.py
new file mode 100644
index 0000000..9602b0b
--- /dev/null
+++ b/xen/xsm/flask/gen-policy.py
@@ -0,0 +1,19 @@
+#!/usr/bin/env python
+import sys
+
+policy_size = 0
+
+sys.stdout.write("""
+/* This file is autogenerated by gen_policy.py */
+#include <xen/init.h>
+
+const unsigned char xsm_init_policy[] __init = {
+""")
+
+for char in sys.stdin.read():
+    sys.stdout.write(" 0x%02x," % ord(char))
+    policy_size = policy_size + 1
+    if policy_size % 13 == 0:
+        sys.stdout.write("\n")
+
+sys.stdout.write("\n};\nconst int __init xsm_init_policy_size = %d;\n" % policy_size)
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index 8df1a3c..93c7d43 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -36,6 +36,24 @@ static inline int verify(struct xsm_operations *ops)
     return 0;
 }
 
+#ifdef CONFIG_XSM_POLICY
+extern char xsm_init_policy[];
+extern int xsm_init_policy_size;
+#else
+#define xsm_init_policy 0
+#endif
+
+static void __init xsm_policy_init(void)
+{
+#ifdef CONFIG_XSM_POLICY
+    if ( policy_size == 0 )
+    {
+        policy_buffer = xsm_init_policy;
+        policy_size = xsm_init_policy_size;
+    }
+#endif
+}
+
 static int __init xsm_core_init(void)
 {
     if ( verify(&dummy_xsm_ops) )
@@ -46,6 +64,7 @@ static int __init xsm_core_init(void)
     }
 
     xsm_ops = &dummy_xsm_ops;
+    xsm_policy_init();
     flask_init();
 
     return 0;
@@ -98,7 +117,8 @@ int __init xsm_dt_init(void)
 
     ret = xsm_core_init();
 
-    xfree(policy_buffer);
+    if ( policy_buffer != xsm_init_policy )
+        xfree(policy_buffer);
 
     return ret;
 }
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] xsm: add a default policy to .init.data
  2016-06-29 15:09 [PATCH v3] xsm: add a default policy to .init.data Daniel De Graaf
@ 2016-06-30 13:45 ` Konrad Rzeszutek Wilk
  2016-06-30 14:01   ` Daniel De Graaf
  0 siblings, 1 reply; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-06-30 13:45 UTC (permalink / raw)
  To: Daniel De Graaf; +Cc: xen-devel, Doug Goldstein, Ian Jackson, Jan Beulich

On Wed, Jun 29, 2016 at 11:09:01AM -0400, Daniel De Graaf wrote:
> This adds a Kconfig option and support for including the XSM policy from
> tools/flask/policy in the hypervisor so that the bootloader does not
> need to provide a policy to get sane behavior from an XSM-enabled
> hypervisor.  The policy provided by the bootloader, if present, will
> override the built-in policy.
> 
> The XSM policy is not moved out of tools because that remains the
> primary location for installing and configuring the policy.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
> 
> Changes from v2 (dropped acks and reviewed-by):
>  - Drop linker script changes, use python binary-to-C file script
>  - Make the config option always include the policy if selected
>  - Note the new conditional dependency on checkpolicy in INSTALL

I liked the previous patch of putting in it in __init section.
Is that something this patch could do? Ah, n/m. I see that
the python script generates the binary with __init!

Secondly I was wondering why the suggestion I had - which was to check
of the 'checkpolicy' availability - and if not found - then
hide the Kconfig option was not mentioned?
.. snip...
> +sys.stdout.write("\n};\nconst int __init xsm_init_policy_size = %d;\n" % policy_size)
> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index 8df1a3c..93c7d43 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -36,6 +36,24 @@ static inline int verify(struct xsm_operations *ops)
>      return 0;
>  }
>  
> +#ifdef CONFIG_XSM_POLICY
> +extern char xsm_init_policy[];

> +extern int xsm_init_policy_size;
> +#else
> +#define xsm_init_policy 0
> +#endif
> +
> +static void __init xsm_policy_init(void)
> +{
> +#ifdef CONFIG_XSM_POLICY
> +    if ( policy_size == 0 )
> +    {
> +        policy_buffer = xsm_init_policy;
> +        policy_size = xsm_init_policy_size;
> +    }
> +#endif
> +}
> +

This all looks like it could go in a header file?

>  static int __init xsm_core_init(void)
>  {
>      if ( verify(&dummy_xsm_ops) )
> @@ -46,6 +64,7 @@ static int __init xsm_core_init(void)
>      }
>  
>      xsm_ops = &dummy_xsm_ops;
> +    xsm_policy_init();
>      flask_init();
>  
>      return 0;
> @@ -98,7 +117,8 @@ int __init xsm_dt_init(void)
>  
>      ret = xsm_core_init();
>  
> -    xfree(policy_buffer);
> +    if ( policy_buffer != xsm_init_policy )
> +        xfree(policy_buffer);
>  
>      return ret;
>  }
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] xsm: add a default policy to .init.data
  2016-06-30 13:45 ` Konrad Rzeszutek Wilk
@ 2016-06-30 14:01   ` Daniel De Graaf
  2016-06-30 15:13     ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel De Graaf @ 2016-06-30 14:01 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: xen-devel, Doug Goldstein, Ian Jackson, Jan Beulich

On 06/30/2016 09:45 AM, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 29, 2016 at 11:09:01AM -0400, Daniel De Graaf wrote:
>> This adds a Kconfig option and support for including the XSM policy from
>> tools/flask/policy in the hypervisor so that the bootloader does not
>> need to provide a policy to get sane behavior from an XSM-enabled
>> hypervisor.  The policy provided by the bootloader, if present, will
>> override the built-in policy.
>>
>> The XSM policy is not moved out of tools because that remains the
>> primary location for installing and configuring the policy.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>
>> Changes from v2 (dropped acks and reviewed-by):
>>  - Drop linker script changes, use python binary-to-C file script
>>  - Make the config option always include the policy if selected
>>  - Note the new conditional dependency on checkpolicy in INSTALL
>
> I liked the previous patch of putting in it in __init section.
> Is that something this patch could do? Ah, n/m. I see that
> the python script generates the binary with __init!
>
> Secondly I was wondering why the suggestion I had - which was to check
> of the 'checkpolicy' availability - and if not found - then
> hide the Kconfig option was not mentioned?

At the moment, since FLASK is the only XSM implementation and it is not
enabled by default, if someone enables XSM they will need to generate a
security policy at some point: either during the hypervisor build or in
the tools build.  Otherwise, they will need to disable FLASK at runtime
or (worst case) run without a policy at all, which means all domains can
act as dom0.  If you plan to disable FLASK at runtime, it is better to
compile without XSM enabled so you don't pay the cost of the function
calls to the XSM hooks on (almost) every hypercall.

Otherwise, I was planning to just change the default from always "y" to
"y" if checkpolicy was found and "n" if not.  This would end up running
"checkpolicy -h" on every (recursive) Make invocation if placed in the
top-level Config.mk - I'm unsure if that would be a problem or not.

> .. snip...
>> +sys.stdout.write("\n};\nconst int __init xsm_init_policy_size = %d;\n" % policy_size)
>> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
>> index 8df1a3c..93c7d43 100644
>> --- a/xen/xsm/xsm_core.c
>> +++ b/xen/xsm/xsm_core.c
>> @@ -36,6 +36,24 @@ static inline int verify(struct xsm_operations *ops)
>>      return 0;
>>  }
>>
>> +#ifdef CONFIG_XSM_POLICY
>> +extern char xsm_init_policy[];
>
>> +extern int xsm_init_policy_size;
>> +#else
>> +#define xsm_init_policy 0
>> +#endif
>> +
>> +static void __init xsm_policy_init(void)
>> +{
>> +#ifdef CONFIG_XSM_POLICY
>> +    if ( policy_size == 0 )
>> +    {
>> +        policy_buffer = xsm_init_policy;
>> +        policy_size = xsm_init_policy_size;
>> +    }
>> +#endif
>> +}
>> +
>
> This all looks like it could go in a header file?

Sure, I could move it to xsm.h.  I thought it might be clearer to have the
assignment and the xfree() check in the same file.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] xsm: add a default policy to .init.data
  2016-06-30 14:01   ` Daniel De Graaf
@ 2016-06-30 15:13     ` Konrad Rzeszutek Wilk
  2016-07-01  7:19       ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-06-30 15:13 UTC (permalink / raw)
  To: Daniel De Graaf; +Cc: xen-devel, Doug Goldstein, Ian Jackson, Jan Beulich

On Thu, Jun 30, 2016 at 10:01:18AM -0400, Daniel De Graaf wrote:
> On 06/30/2016 09:45 AM, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jun 29, 2016 at 11:09:01AM -0400, Daniel De Graaf wrote:
> > > This adds a Kconfig option and support for including the XSM policy from
> > > tools/flask/policy in the hypervisor so that the bootloader does not
> > > need to provide a policy to get sane behavior from an XSM-enabled
> > > hypervisor.  The policy provided by the bootloader, if present, will
> > > override the built-in policy.
> > > 
> > > The XSM policy is not moved out of tools because that remains the
> > > primary location for installing and configuring the policy.
> > > 
> > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > > ---
> > > 
> > > Changes from v2 (dropped acks and reviewed-by):
> > >  - Drop linker script changes, use python binary-to-C file script
> > >  - Make the config option always include the policy if selected
> > >  - Note the new conditional dependency on checkpolicy in INSTALL
> > 
> > I liked the previous patch of putting in it in __init section.
> > Is that something this patch could do? Ah, n/m. I see that
> > the python script generates the binary with __init!
> > 
> > Secondly I was wondering why the suggestion I had - which was to check
> > of the 'checkpolicy' availability - and if not found - then
> > hide the Kconfig option was not mentioned?
> 
> At the moment, since FLASK is the only XSM implementation and it is not
> enabled by default, if someone enables XSM they will need to generate a
> security policy at some point: either during the hypervisor build or in
> the tools build.  Otherwise, they will need to disable FLASK at runtime
> or (worst case) run without a policy at all, which means all domains can
> act as dom0.  If you plan to disable FLASK at runtime, it is better to
> compile without XSM enabled so you don't pay the cost of the function
> calls to the XSM hooks on (almost) every hypercall.
> 
> Otherwise, I was planning to just change the default from always "y" to
> "y" if checkpolicy was found and "n" if not.  This would end up running
> "checkpolicy -h" on every (recursive) Make invocation if placed in the
> top-level Config.mk - I'm unsure if that would be a problem or not.

We already do quite a lot of other checks in there. I am not sure if
an extra one would be so bad. Do other maintainers care about it?
> 
> > .. snip...
> > > +sys.stdout.write("\n};\nconst int __init xsm_init_policy_size = %d;\n" % policy_size)
> > > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> > > index 8df1a3c..93c7d43 100644
> > > --- a/xen/xsm/xsm_core.c
> > > +++ b/xen/xsm/xsm_core.c
> > > @@ -36,6 +36,24 @@ static inline int verify(struct xsm_operations *ops)
> > >      return 0;
> > >  }
> > > 
> > > +#ifdef CONFIG_XSM_POLICY
> > > +extern char xsm_init_policy[];
> > 
> > > +extern int xsm_init_policy_size;
> > > +#else
> > > +#define xsm_init_policy 0
> > > +#endif
> > > +
> > > +static void __init xsm_policy_init(void)
> > > +{
> > > +#ifdef CONFIG_XSM_POLICY
> > > +    if ( policy_size == 0 )
> > > +    {
> > > +        policy_buffer = xsm_init_policy;
> > > +        policy_size = xsm_init_policy_size;
> > > +    }
> > > +#endif
> > > +}
> > > +
> > 
> > This all looks like it could go in a header file?
> 
> Sure, I could move it to xsm.h.  I thought it might be clearer to have the
> assignment and the xfree() check in the same file.

You are the maintainer of that code, so it is your call.

Having externs in C code is not that great (from my perspective),
so if you move all of this to a header file that should get easier?

But as said - you are the maintainer and I am just voicing my
preferences - and if the code looks "nicer" your way - by all means
do it that way! I am not going to get offended :-)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] xsm: add a default policy to .init.data
  2016-06-30 15:13     ` Konrad Rzeszutek Wilk
@ 2016-07-01  7:19       ` Jan Beulich
  2016-07-05 15:37         ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-07-01  7:19 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Daniel De Graaf
  Cc: Ian Jackson, Doug Goldstein, xen-devel

>>> On 30.06.16 at 17:13, <konrad.wilk@oracle.com> wrote:
> On Thu, Jun 30, 2016 at 10:01:18AM -0400, Daniel De Graaf wrote:
>> On 06/30/2016 09:45 AM, Konrad Rzeszutek Wilk wrote:
>> > On Wed, Jun 29, 2016 at 11:09:01AM -0400, Daniel De Graaf wrote:
>> > > --- a/xen/xsm/xsm_core.c
>> > > +++ b/xen/xsm/xsm_core.c
>> > > @@ -36,6 +36,24 @@ static inline int verify(struct xsm_operations *ops)
>> > >      return 0;
>> > >  }
>> > > 
>> > > +#ifdef CONFIG_XSM_POLICY
>> > > +extern char xsm_init_policy[];
>> > 
>> > > +extern int xsm_init_policy_size;
>> > > +#else
>> > > +#define xsm_init_policy 0
>> > > +#endif
>> > > +
>> > > +static void __init xsm_policy_init(void)
>> > > +{
>> > > +#ifdef CONFIG_XSM_POLICY
>> > > +    if ( policy_size == 0 )
>> > > +    {
>> > > +        policy_buffer = xsm_init_policy;
>> > > +        policy_size = xsm_init_policy_size;
>> > > +    }
>> > > +#endif
>> > > +}
>> > > +
>> > 
>> > This all looks like it could go in a header file?
>> 
>> Sure, I could move it to xsm.h.  I thought it might be clearer to have the
>> assignment and the xfree() check in the same file.
> 
> You are the maintainer of that code, so it is your call.
> 
> Having externs in C code is not that great (from my perspective),
> so if you move all of this to a header file that should get easier?

I agree, fwiw, for the extern-s to be moved to a header. That
header should then also be included by the generated source
file producing the symbols. I don't, however, see why the
function should go to a header.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] xsm: add a default policy to .init.data
  2016-07-01  7:19       ` Jan Beulich
@ 2016-07-05 15:37         ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-07-05 15:37 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ian Jackson, Daniel De Graaf, Doug Goldstein, xen-devel

On Fri, Jul 01, 2016 at 01:19:51AM -0600, Jan Beulich wrote:
> >>> On 30.06.16 at 17:13, <konrad.wilk@oracle.com> wrote:
> > On Thu, Jun 30, 2016 at 10:01:18AM -0400, Daniel De Graaf wrote:
> >> On 06/30/2016 09:45 AM, Konrad Rzeszutek Wilk wrote:
> >> > On Wed, Jun 29, 2016 at 11:09:01AM -0400, Daniel De Graaf wrote:
> >> > > --- a/xen/xsm/xsm_core.c
> >> > > +++ b/xen/xsm/xsm_core.c
> >> > > @@ -36,6 +36,24 @@ static inline int verify(struct xsm_operations *ops)
> >> > >      return 0;
> >> > >  }
> >> > > 
> >> > > +#ifdef CONFIG_XSM_POLICY
> >> > > +extern char xsm_init_policy[];
> >> > 
> >> > > +extern int xsm_init_policy_size;
> >> > > +#else
> >> > > +#define xsm_init_policy 0
> >> > > +#endif
> >> > > +
> >> > > +static void __init xsm_policy_init(void)
> >> > > +{
> >> > > +#ifdef CONFIG_XSM_POLICY
> >> > > +    if ( policy_size == 0 )
> >> > > +    {
> >> > > +        policy_buffer = xsm_init_policy;
> >> > > +        policy_size = xsm_init_policy_size;
> >> > > +    }
> >> > > +#endif
> >> > > +}
> >> > > +
> >> > 
> >> > This all looks like it could go in a header file?
> >> 
> >> Sure, I could move it to xsm.h.  I thought it might be clearer to have the
> >> assignment and the xfree() check in the same file.
> > 
> > You are the maintainer of that code, so it is your call.
> > 
> > Having externs in C code is not that great (from my perspective),
> > so if you move all of this to a header file that should get easier?
> 
> I agree, fwiw, for the extern-s to be moved to a header. That
> header should then also be included by the generated source
> file producing the symbols. I don't, however, see why the
> function should go to a header.

I was thinking that since the function is a nop under #CONFIG_XSM_POLICY
you could just make it an inline static function, like:

#ifdef CONFIG_XSM_POLICY
static inline void  xsm_policy_init(void)
{...
}
#else
#define xsm_policy_init(void) ;
#endif

Or such.


But this is really minor, and pls ignore it if means doing more work than
neccessary.

> 
> Jan
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-07-05 15:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-29 15:09 [PATCH v3] xsm: add a default policy to .init.data Daniel De Graaf
2016-06-30 13:45 ` Konrad Rzeszutek Wilk
2016-06-30 14:01   ` Daniel De Graaf
2016-06-30 15:13     ` Konrad Rzeszutek Wilk
2016-07-01  7:19       ` Jan Beulich
2016-07-05 15:37         ` Konrad Rzeszutek Wilk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).