| Message ID | 20210928083944.780398-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v4] xen: rework `checkpolicy` detection when using "randconfig" | expand |
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
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,
> 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 > >
On Tue, Sep 28, 2021 at 03:34:00PM +0100, Anthony PERARD wrote: > 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? I've sent v5 with this change to the Kconfig file removed.
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
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(-)