[v4] xen: rework `checkpolicy` detection when using "randconfig"

[XEN PATCH v4] xen: rework `checkpolicy` detection when using "randconfig"

Posted by Anthony PERARD 3 years, 2 months ago

This will help prevent the CI loop from having build failures when
`checkpolicy` isn't available when doing "randconfig" jobs.

To prevent "randconfig" from selecting XSM_FLASK_POLICY when
`checkpolicy` isn't available, we will actually override the config
output with the use of KCONFIG_ALLCONFIG.

Doing this way still allow a user/developer to set XSM_FLASK_POLICY
even when "checkpolicy" isn't available. It also prevent the build
system from reset the config when "checkpolicy" isn't available
anymore. And XSM_FLASK_POLICY is still selected automatically when
`checkpolicy` is available.
But this also work well for "randconfig", as it will not select
XSM_FLASK_POLICY when "checkpolicy" is missing.

This patch allows to easily add more override which depends on the
environment.

Also, move the check out of Config.mk and into xen/ build system.
Nothing in tools/ is using that information as it's done by
./configure.

We named the new file ".allconfig.tmp" as ".*.tmp" are already ignored
via .gitignore.

Remove '= y' in Kconfig as it isn't needed, only a value "y" is true,
anything else is considered false.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
v4:
- keep XEN_ prefix for HAS_CHECKPOLICY
- rework .allconfig.tmp file generation, so it is easier to read.
- remove .allconfig.tmp on clean, .*.tmp files aren't all cleaned yet,
  maybe for another time.
- add information about file name choice and Kconfig change in patch
  description.

v3:
- use KCONFIG_ALLCONFIG
- don't override XSM_FLASK_POLICY value unless we do randconfig.
- no more changes to the current behavior of kconfig, only to
  randconfig.

v2 was "[XEN PATCH v2] xen: allow XSM_FLASK_POLICY only if checkpolicy binary is available"
---
 Config.mk          |  6 ------
 xen/Makefile       | 20 +++++++++++++++++---
 xen/common/Kconfig |  2 +-
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/Config.mk b/Config.mk
index e85bf186547f..d5490e35d03d 100644
--- a/Config.mk
+++ b/Config.mk
@@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y
 build_id_linker := --build-id=sha1
 endif
 
-ifndef XEN_HAS_CHECKPOLICY
-    CHECKPOLICY ?= checkpolicy
-    XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && echo y || echo n)
-    export XEN_HAS_CHECKPOLICY
-endif
-
 define buildmakevars2shellvars
     export PREFIX="$(prefix)";                                            \
     export XEN_SCRIPT_DIR="$(XEN_SCRIPT_DIR)";                            \
diff --git a/xen/Makefile b/xen/Makefile
index f47423dacd9a..7c2ffce0fc77 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -17,6 +17,8 @@ export XEN_BUILD_HOST	?= $(shell hostname)
 PYTHON_INTERPRETER	:= $(word 1,$(shell which python3 python python2 2>/dev/null) python)
 export PYTHON		?= $(PYTHON_INTERPRETER)
 
+export CHECKPOLICY	?= checkpolicy
+
 export BASEDIR := $(CURDIR)
 export XEN_ROOT := $(BASEDIR)/..
 
@@ -178,6 +180,8 @@ CFLAGS += $(CLANG_FLAGS)
 export CLANG_FLAGS
 endif
 
+export XEN_HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)
+
 export root-make-done := y
 endif # root-make-done
 
@@ -189,14 +193,24 @@ ifeq ($(config-build),y)
 # *config targets only - make sure prerequisites are updated, and descend
 # in tools/kconfig to make the *config target
 
+# Create a file for KCONFIG_ALLCONFIG which depends on the environment.
+# This will be use by kconfig targets allyesconfig/allmodconfig/allnoconfig/randconfig
+filechk_kconfig_allconfig = \
+    $(if $(findstring n,$(XEN_HAS_CHECKPOLICY)), echo 'CONFIG_XSM_FLASK_POLICY=n';) \
+    $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG);) \
+    :
+
+.allconfig.tmp: FORCE
+	set -e; { $(call filechk_kconfig_allconfig); } > $@
+
 config: FORCE
 	$(MAKE) $(kconfig) $@
 
 # Config.mk tries to include .config file, don't try to remake it
 %/.config: ;
 
-%config: FORCE
-	$(MAKE) $(kconfig) $@
+%config: .allconfig.tmp FORCE
+	$(MAKE) $(kconfig) KCONFIG_ALLCONFIG=$< $@
 
 else # !config-build
 
@@ -368,7 +382,7 @@ _clean: delete-unfresh-files
 		-o -name "*.gcno" -o -name ".*.cmd" -o -name "lib.a" \) -exec rm -f {} \;
 	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
 	rm -f asm-offsets.s include/asm-*/asm-offsets.h
-	rm -f .banner
+	rm -f .banner .allconfig.tmp
 
 .PHONY: _distclean
 _distclean: clean
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index db687b1785e7..eb6c2edb7bfe 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -251,7 +251,7 @@ config XSM_FLASK_AVC_STATS
 
 config XSM_FLASK_POLICY
 	bool "Compile Xen with a built-in FLASK security policy"
-	default y if "$(XEN_HAS_CHECKPOLICY)" = "y"
+	default y if "$(XEN_HAS_CHECKPOLICY)"
 	depends on XSM_FLASK
 	---help---
 	  This includes a default XSM policy in the hypervisor so that the
-- 
Anthony PERARD

Re: [XEN PATCH v4] xen: rework `checkpolicy` detection when using "randconfig"

Posted by Luca Fancellu 3 years, 2 months ago


> On 28 Sep 2021, at 09:39, Anthony PERARD <anthony.perard@citrix.com> wrote:
> 
> This will help prevent the CI loop from having build failures when
> `checkpolicy` isn't available when doing "randconfig" jobs.
> 
> To prevent "randconfig" from selecting XSM_FLASK_POLICY when
> `checkpolicy` isn't available, we will actually override the config
> output with the use of KCONFIG_ALLCONFIG.
> 
> Doing this way still allow a user/developer to set XSM_FLASK_POLICY
> even when "checkpolicy" isn't available. It also prevent the build
> system from reset the config when "checkpolicy" isn't available
> anymore. And XSM_FLASK_POLICY is still selected automatically when
> `checkpolicy` is available.
> But this also work well for "randconfig", as it will not select
> XSM_FLASK_POLICY when "checkpolicy" is missing.
> 
> This patch allows to easily add more override which depends on the
> environment.
> 
> Also, move the check out of Config.mk and into xen/ build system.
> Nothing in tools/ is using that information as it's done by
> ./configure.
> 
> We named the new file ".allconfig.tmp" as ".*.tmp" are already ignored
> via .gitignore.
> 
> Remove '= y' in Kconfig as it isn't needed, only a value "y" is true,
> anything else is considered false.

I don’t know if it is true, I’m having a look here: 
https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt

And the section “Menu dependencies” states that:

An expression can have a value of 'n', 'm' or 'y' (or 0, 1, 2
respectively for calculations).

So it seems to me that m and y are evaluated as true, am I wrong?

Cheers,
Luca

> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v4:
> - keep XEN_ prefix for HAS_CHECKPOLICY
> - rework .allconfig.tmp file generation, so it is easier to read.
> - remove .allconfig.tmp on clean, .*.tmp files aren't all cleaned yet,
>  maybe for another time.
> - add information about file name choice and Kconfig change in patch
>  description.
> 
> v3:
> - use KCONFIG_ALLCONFIG
> - don't override XSM_FLASK_POLICY value unless we do randconfig.
> - no more changes to the current behavior of kconfig, only to
>  randconfig.
> 
> v2 was "[XEN PATCH v2] xen: allow XSM_FLASK_POLICY only if checkpolicy binary is available"
> ---
> Config.mk          |  6 ------
> xen/Makefile       | 20 +++++++++++++++++---
> xen/common/Kconfig |  2 +-
> 3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/Config.mk b/Config.mk
> index e85bf186547f..d5490e35d03d 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y
> build_id_linker := --build-id=sha1
> endif
> 
> -ifndef XEN_HAS_CHECKPOLICY
> -    CHECKPOLICY ?= checkpolicy
> -    XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && echo y || echo n)
> -    export XEN_HAS_CHECKPOLICY
> -endif
> -
> define buildmakevars2shellvars
>     export PREFIX="$(prefix)";                                            \
>     export XEN_SCRIPT_DIR="$(XEN_SCRIPT_DIR)";                            \
> diff --git a/xen/Makefile b/xen/Makefile
> index f47423dacd9a..7c2ffce0fc77 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -17,6 +17,8 @@ export XEN_BUILD_HOST	?= $(shell hostname)
> PYTHON_INTERPRETER	:= $(word 1,$(shell which python3 python python2 2>/dev/null) python)
> export PYTHON		?= $(PYTHON_INTERPRETER)
> 
> +export CHECKPOLICY	?= checkpolicy
> +
> export BASEDIR := $(CURDIR)
> export XEN_ROOT := $(BASEDIR)/..
> 
> @@ -178,6 +180,8 @@ CFLAGS += $(CLANG_FLAGS)
> export CLANG_FLAGS
> endif
> 
> +export XEN_HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)
> +
> export root-make-done := y
> endif # root-make-done
> 
> @@ -189,14 +193,24 @@ ifeq ($(config-build),y)
> # *config targets only - make sure prerequisites are updated, and descend
> # in tools/kconfig to make the *config target
> 
> +# Create a file for KCONFIG_ALLCONFIG which depends on the environment.
> +# This will be use by kconfig targets allyesconfig/allmodconfig/allnoconfig/randconfig
> +filechk_kconfig_allconfig = \
> +    $(if $(findstring n,$(XEN_HAS_CHECKPOLICY)), echo 'CONFIG_XSM_FLASK_POLICY=n';) \
> +    $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG);) \
> +    :
> +
> +.allconfig.tmp: FORCE
> +	set -e; { $(call filechk_kconfig_allconfig); } > $@
> +
> config: FORCE
> 	$(MAKE) $(kconfig) $@
> 
> # Config.mk tries to include .config file, don't try to remake it
> %/.config: ;
> 
> -%config: FORCE
> -	$(MAKE) $(kconfig) $@
> +%config: .allconfig.tmp FORCE
> +	$(MAKE) $(kconfig) KCONFIG_ALLCONFIG=$< $@
> 
> else # !config-build
> 
> @@ -368,7 +382,7 @@ _clean: delete-unfresh-files
> 		-o -name "*.gcno" -o -name ".*.cmd" -o -name "lib.a" \) -exec rm -f {} \;
> 	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
> 	rm -f asm-offsets.s include/asm-*/asm-offsets.h
> -	rm -f .banner
> +	rm -f .banner .allconfig.tmp
> 
> .PHONY: _distclean
> _distclean: clean
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index db687b1785e7..eb6c2edb7bfe 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -251,7 +251,7 @@ config XSM_FLASK_AVC_STATS
> 
> config XSM_FLASK_POLICY
> 	bool "Compile Xen with a built-in FLASK security policy"
> -	default y if "$(XEN_HAS_CHECKPOLICY)" = "y"
> +	default y if "$(XEN_HAS_CHECKPOLICY)"
> 	depends on XSM_FLASK
> 	---help---
> 	  This includes a default XSM policy in the hypervisor so that the
> -- 
> Anthony PERARD
> 
>

Re: [XEN PATCH v4] xen: rework `checkpolicy` detection when using "randconfig"

Posted by Jan Beulich 3 years, 2 months ago

On 28.09.2021 10:39, Anthony PERARD wrote:
> This will help prevent the CI loop from having build failures when
> `checkpolicy` isn't available when doing "randconfig" jobs.
> 
> To prevent "randconfig" from selecting XSM_FLASK_POLICY when
> `checkpolicy` isn't available, we will actually override the config
> output with the use of KCONFIG_ALLCONFIG.
> 
> Doing this way still allow a user/developer to set XSM_FLASK_POLICY
> even when "checkpolicy" isn't available. It also prevent the build
> system from reset the config when "checkpolicy" isn't available
> anymore. And XSM_FLASK_POLICY is still selected automatically when
> `checkpolicy` is available.
> But this also work well for "randconfig", as it will not select
> XSM_FLASK_POLICY when "checkpolicy" is missing.
> 
> This patch allows to easily add more override which depends on the
> environment.
> 
> Also, move the check out of Config.mk and into xen/ build system.
> Nothing in tools/ is using that information as it's done by
> ./configure.
> 
> We named the new file ".allconfig.tmp" as ".*.tmp" are already ignored
> via .gitignore.
> 
> Remove '= y' in Kconfig as it isn't needed, only a value "y" is true,
> anything else is considered false.

Seeing you say this explicitly makes me wonder - is this actually true?
At least when modules are enabled (which our kconfig is capable of even
if we don't use that part of it), "m" is also "kind of" true, and the
related logic really isn't quite boolean iirc.

Everything else looks goot to me now, thanks.

Jan

Re: [XEN PATCH v4] xen: rework `checkpolicy` detection when using "randconfig"

Posted by Anthony PERARD 3 years, 2 months ago

On Tue, Sep 28, 2021 at 03:46:01PM +0200, Jan Beulich wrote:
> On 28.09.2021 10:39, Anthony PERARD wrote:
> > This will help prevent the CI loop from having build failures when
> > `checkpolicy` isn't available when doing "randconfig" jobs.
> > 
> > To prevent "randconfig" from selecting XSM_FLASK_POLICY when
> > `checkpolicy` isn't available, we will actually override the config
> > output with the use of KCONFIG_ALLCONFIG.
> > 
> > Doing this way still allow a user/developer to set XSM_FLASK_POLICY
> > even when "checkpolicy" isn't available. It also prevent the build
> > system from reset the config when "checkpolicy" isn't available
> > anymore. And XSM_FLASK_POLICY is still selected automatically when
> > `checkpolicy` is available.
> > But this also work well for "randconfig", as it will not select
> > XSM_FLASK_POLICY when "checkpolicy" is missing.
> > 
> > This patch allows to easily add more override which depends on the
> > environment.
> > 
> > Also, move the check out of Config.mk and into xen/ build system.
> > Nothing in tools/ is using that information as it's done by
> > ./configure.
> > 
> > We named the new file ".allconfig.tmp" as ".*.tmp" are already ignored
> > via .gitignore.
> > 
> > Remove '= y' in Kconfig as it isn't needed, only a value "y" is true,
> > anything else is considered false.
> 
> Seeing you say this explicitly makes me wonder - is this actually true?

I've check that this was true by empirical testing before sending the
patch. But the documentation isn't clear to me about the meaning of
'default y if "m"'. So would you rather keep '= y' just to stay on the
safe side?

> At least when modules are enabled (which our kconfig is capable of even
> if we don't use that part of it), "m" is also "kind of" true, and the
> related logic really isn't quite boolean iirc.
> 
> Everything else looks goot to me now, thanks.

Thanks,

-- 
Anthony PERARD