* [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) @ 2020-03-13 21:24 Shuah Khan 2020-03-13 23:18 ` Kees Cook 2020-03-16 12:12 ` Michael Ellerman 0 siblings, 2 replies; 8+ messages in thread From: Shuah Khan @ 2020-03-13 21:24 UTC (permalink / raw) To: shuah, keescook, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx Cc: Shuah Khan, khilman, mpe, linux-kselftest, linux-kernel, netdev, bpf Fix seccomp relocatable builds. This is a simple fix to use the right lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h header, and defining LDFLAGS for pthread lib. Removes custom clean rule which is no longer necessary with the use of TEST_GEN_PROGS. Uses $(OUTPUT) defined in lib.mk to handle build relocation. The following use-cases work with this change: In seccomp directory: make all and make clean From top level from main Makefile: make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \ CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> --- Changes since v2: -- Using TEST_GEN_PROGS is sufficient to generate objects. Addresses review comments from Kees Cook. tools/testing/selftests/seccomp/Makefile | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile index 1760b3e39730..a0388fd2c3f2 100644 --- a/tools/testing/selftests/seccomp/Makefile +++ b/tools/testing/selftests/seccomp/Makefile @@ -1,17 +1,15 @@ # SPDX-License-Identifier: GPL-2.0 -all: - -include ../lib.mk +CFLAGS += -Wl,-no-as-needed -Wall +LDFLAGS += -lpthread .PHONY: all clean -BINARIES := seccomp_bpf seccomp_benchmark -CFLAGS += -Wl,-no-as-needed -Wall +include ../lib.mk + +# OUTPUT set by lib.mk +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h - $(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@ +$(TEST_GEN_PROGS): ../kselftest_harness.h -TEST_PROGS += $(BINARIES) -EXTRA_CLEAN := $(BINARIES) +all: $(TEST_GEN_PROGS) -all: $(BINARIES) -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) 2020-03-13 21:24 [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) Shuah Khan @ 2020-03-13 23:18 ` Kees Cook 2020-03-13 23:38 ` Shuah Khan 2020-03-16 12:12 ` Michael Ellerman 1 sibling, 1 reply; 8+ messages in thread From: Kees Cook @ 2020-03-13 23:18 UTC (permalink / raw) To: Shuah Khan Cc: shuah, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx, khilman, mpe, linux-kselftest, linux-kernel, netdev, bpf On Fri, Mar 13, 2020 at 03:24:04PM -0600, Shuah Khan wrote: > Fix seccomp relocatable builds. This is a simple fix to use the right > lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h > header, and defining LDFLAGS for pthread lib. > > Removes custom clean rule which is no longer necessary with the use of > TEST_GEN_PROGS. > > Uses $(OUTPUT) defined in lib.mk to handle build relocation. > > The following use-cases work with this change: > > In seccomp directory: > make all and make clean > > From top level from main Makefile: > make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \ > CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> > --- > > Changes since v2: > -- Using TEST_GEN_PROGS is sufficient to generate objects. > Addresses review comments from Kees Cook. > > tools/testing/selftests/seccomp/Makefile | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile > index 1760b3e39730..a0388fd2c3f2 100644 > --- a/tools/testing/selftests/seccomp/Makefile > +++ b/tools/testing/selftests/seccomp/Makefile > @@ -1,17 +1,15 @@ > # SPDX-License-Identifier: GPL-2.0 > -all: > - > -include ../lib.mk > +CFLAGS += -Wl,-no-as-needed -Wall > +LDFLAGS += -lpthread > > .PHONY: all clean Isn't this line redundant to ../lib.mk's? > > -BINARIES := seccomp_bpf seccomp_benchmark > -CFLAGS += -Wl,-no-as-needed -Wall > +include ../lib.mk > + > +# OUTPUT set by lib.mk > +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark > > -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h > - $(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@ > +$(TEST_GEN_PROGS): ../kselftest_harness.h > > -TEST_PROGS += $(BINARIES) > -EXTRA_CLEAN := $(BINARIES) > +all: $(TEST_GEN_PROGS) And isn't this one too? I think if those are removed it should all still work? Regardless: Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > -all: $(BINARIES) > -- > 2.20.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) 2020-03-13 23:18 ` Kees Cook @ 2020-03-13 23:38 ` Shuah Khan 0 siblings, 0 replies; 8+ messages in thread From: Shuah Khan @ 2020-03-13 23:38 UTC (permalink / raw) To: Kees Cook Cc: shuah, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx, khilman, mpe, linux-kselftest, linux-kernel, netdev, bpf, Shuah Khan On 3/13/20 5:18 PM, Kees Cook wrote: > On Fri, Mar 13, 2020 at 03:24:04PM -0600, Shuah Khan wrote: >> Fix seccomp relocatable builds. This is a simple fix to use the right >> lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h >> header, and defining LDFLAGS for pthread lib. >> >> Removes custom clean rule which is no longer necessary with the use of >> TEST_GEN_PROGS. >> >> Uses $(OUTPUT) defined in lib.mk to handle build relocation. >> >> The following use-cases work with this change: >> >> In seccomp directory: >> make all and make clean >> >> From top level from main Makefile: >> make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \ >> CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp >> >> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> >> --- >> >> Changes since v2: >> -- Using TEST_GEN_PROGS is sufficient to generate objects. >> Addresses review comments from Kees Cook. >> >> tools/testing/selftests/seccomp/Makefile | 18 ++++++++---------- >> 1 file changed, 8 insertions(+), 10 deletions(-) >> >> diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile >> index 1760b3e39730..a0388fd2c3f2 100644 >> --- a/tools/testing/selftests/seccomp/Makefile >> +++ b/tools/testing/selftests/seccomp/Makefile >> @@ -1,17 +1,15 @@ >> # SPDX-License-Identifier: GPL-2.0 >> -all: >> - >> -include ../lib.mk >> +CFLAGS += -Wl,-no-as-needed -Wall >> +LDFLAGS += -lpthread >> >> .PHONY: all clean > > Isn't this line redundant to ../lib.mk's? > >> >> -BINARIES := seccomp_bpf seccomp_benchmark >> -CFLAGS += -Wl,-no-as-needed -Wall >> +include ../lib.mk >> + >> +# OUTPUT set by lib.mk >> +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark >> >> -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h >> - $(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@ >> +$(TEST_GEN_PROGS): ../kselftest_harness.h >> >> -TEST_PROGS += $(BINARIES) >> -EXTRA_CLEAN := $(BINARIES) >> +all: $(TEST_GEN_PROGS) > > And isn't this one too? make in seccomp directory won't work. lib.mk won't build it. One reason why I wanted to clearly call this out as CUSTOM program. But it does make sense to reduce additional EXTRA_CLEAN by just using TEST_GEN_PROGS. > > I think if those are removed it should all still work? Regardless: > diff Makefile diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile index a0388fd2c3f2..d3d256654cb1 100644 --- a/tools/testing/selftests/seccomp/Makefile +++ b/tools/testing/selftests/seccomp/Makefile @@ -2,7 +2,7 @@ CFLAGS += -Wl,-no-as-needed -Wall LDFLAGS += -lpthread -.PHONY: all clean +#.PHONY: all clean include ../lib.mk @@ -11,5 +11,5 @@ TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark $(TEST_GEN_PROGS): ../kselftest_harness.h -all: $(TEST_GEN_PROGS) +#$all: $(TEST_GEN_PROGS) With this: make make: Nothing to be done for 'all'. > Reviewed-by: Kees Cook <keescook@chromium.org> > I am addressing your comment about other tests that de[end on kselftest_harness.h. I have a few patches ready to be sent. thanks, -- Shuah ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) 2020-03-13 21:24 [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) Shuah Khan 2020-03-13 23:18 ` Kees Cook @ 2020-03-16 12:12 ` Michael Ellerman 2020-03-16 21:05 ` Kees Cook 1 sibling, 1 reply; 8+ messages in thread From: Michael Ellerman @ 2020-03-16 12:12 UTC (permalink / raw) To: Shuah Khan, shuah, keescook, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx Cc: Shuah Khan, khilman, linux-kselftest, linux-kernel, netdev, bpf Shuah Khan <skhan@linuxfoundation.org> writes: > Fix seccomp relocatable builds. This is a simple fix to use the right > lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h > header, and defining LDFLAGS for pthread lib. > > Removes custom clean rule which is no longer necessary with the use of > TEST_GEN_PROGS. > > Uses $(OUTPUT) defined in lib.mk to handle build relocation. > > The following use-cases work with this change: > > In seccomp directory: > make all and make clean > > From top level from main Makefile: > make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \ > CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> > --- > > Changes since v2: > -- Using TEST_GEN_PROGS is sufficient to generate objects. > Addresses review comments from Kees Cook. > > tools/testing/selftests/seccomp/Makefile | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile > index 1760b3e39730..a0388fd2c3f2 100644 > --- a/tools/testing/selftests/seccomp/Makefile > +++ b/tools/testing/selftests/seccomp/Makefile > @@ -1,17 +1,15 @@ > # SPDX-License-Identifier: GPL-2.0 > -all: > - > -include ../lib.mk > +CFLAGS += -Wl,-no-as-needed -Wall > +LDFLAGS += -lpthread > > .PHONY: all clean > > -BINARIES := seccomp_bpf seccomp_benchmark > -CFLAGS += -Wl,-no-as-needed -Wall > +include ../lib.mk > + > +# OUTPUT set by lib.mk > +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark > > -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h > - $(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@ > +$(TEST_GEN_PROGS): ../kselftest_harness.h > > -TEST_PROGS += $(BINARIES) > -EXTRA_CLEAN := $(BINARIES) > +all: $(TEST_GEN_PROGS) > > -all: $(BINARIES) It shouldn't be that complicated. We just need to define TEST_GEN_PROGS before including lib.mk, and then add the dependency on the harness after we include lib.mk (so that TEST_GEN_PROGS has been updated to prefix $(OUTPUT)). eg: # SPDX-License-Identifier: GPL-2.0 CFLAGS += -Wl,-no-as-needed -Wall LDFLAGS += -lpthread TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark include ../lib.mk $(TEST_GEN_PROGS): ../kselftest_harness.h Normal in-tree build: selftests$ make TARGETS=seccomp make[1]: Entering directory '/home/michael/linux/tools/testing/selftests/seccomp' gcc -Wl,-no-as-needed -Wall -lpthread seccomp_bpf.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_bpf gcc -Wl,-no-as-needed -Wall -lpthread seccomp_benchmark.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_benchmark make[1]: Leaving directory '/home/michael/linux/tools/testing/selftests/seccomp' selftests$ ls -l seccomp/ total 388 -rw-rw-r-- 1 michael michael 41 Jan 9 12:00 config -rw-rw-r-- 1 michael michael 201 Mar 16 23:04 Makefile -rwxrwxr-x 1 michael michael 70824 Mar 16 23:07 seccomp_benchmark* -rw-rw-r-- 1 michael michael 2289 Feb 17 21:39 seccomp_benchmark.c -rwxrwxr-x 1 michael michael 290520 Mar 16 23:07 seccomp_bpf* -rw-rw-r-- 1 michael michael 94778 Mar 5 23:33 seccomp_bpf.c O= build: selftests$ make TARGETS=seccomp O=$PWD/build make[1]: Entering directory '/home/michael/linux/tools/testing/selftests/seccomp' gcc -Wl,-no-as-needed -Wall -lpthread seccomp_bpf.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/build/seccomp/seccomp_bpf gcc -Wl,-no-as-needed -Wall -lpthread seccomp_benchmark.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/build/seccomp/seccomp_benchmark make[1]: Leaving directory '/home/michael/linux/tools/testing/selftests/seccomp' selftests$ ls -l build/seccomp/ total 280 -rwxrwxr-x 1 michael michael 70824 Mar 16 23:05 seccomp_benchmark* -rwxrwxr-x 1 michael michael 290520 Mar 16 23:05 seccomp_bpf* Build in the directory itself: selftests$ cd seccomp seccomp$ make gcc -Wl,-no-as-needed -Wall -lpthread seccomp_bpf.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_bpf gcc -Wl,-no-as-needed -Wall -lpthread seccomp_benchmark.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_benchmark seccomp$ ls -l total 388 -rw-rw-r-- 1 michael michael 41 Jan 9 12:00 config -rw-rw-r-- 1 michael michael 201 Mar 16 23:04 Makefile -rwxrwxr-x 1 michael michael 70824 Mar 16 23:06 seccomp_benchmark* -rw-rw-r-- 1 michael michael 2289 Feb 17 21:39 seccomp_benchmark.c -rwxrwxr-x 1 michael michael 290520 Mar 16 23:06 seccomp_bpf* -rw-rw-r-- 1 michael michael 94778 Mar 5 23:33 seccomp_bpf.c cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) 2020-03-16 12:12 ` Michael Ellerman @ 2020-03-16 21:05 ` Kees Cook 2020-03-19 3:15 ` Michael Ellerman 0 siblings, 1 reply; 8+ messages in thread From: Kees Cook @ 2020-03-16 21:05 UTC (permalink / raw) To: Michael Ellerman Cc: Shuah Khan, shuah, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx, khilman, linux-kselftest, linux-kernel, netdev, bpf On Mon, Mar 16, 2020 at 11:12:57PM +1100, Michael Ellerman wrote: > Shuah Khan <skhan@linuxfoundation.org> writes: > > Fix seccomp relocatable builds. This is a simple fix to use the right > > lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h > > header, and defining LDFLAGS for pthread lib. > > > > Removes custom clean rule which is no longer necessary with the use of > > TEST_GEN_PROGS. > > > > Uses $(OUTPUT) defined in lib.mk to handle build relocation. > > > > The following use-cases work with this change: > > > > In seccomp directory: > > make all and make clean > > > > From top level from main Makefile: > > make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \ > > CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp > > > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> > > --- > > > > Changes since v2: > > -- Using TEST_GEN_PROGS is sufficient to generate objects. > > Addresses review comments from Kees Cook. > > > > tools/testing/selftests/seccomp/Makefile | 18 ++++++++---------- > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile > > index 1760b3e39730..a0388fd2c3f2 100644 > > --- a/tools/testing/selftests/seccomp/Makefile > > +++ b/tools/testing/selftests/seccomp/Makefile > > @@ -1,17 +1,15 @@ > > # SPDX-License-Identifier: GPL-2.0 > > -all: > > - > > -include ../lib.mk > > +CFLAGS += -Wl,-no-as-needed -Wall > > +LDFLAGS += -lpthread > > > > .PHONY: all clean > > > > -BINARIES := seccomp_bpf seccomp_benchmark > > -CFLAGS += -Wl,-no-as-needed -Wall > > +include ../lib.mk > > + > > +# OUTPUT set by lib.mk > > +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark > > > > -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h > > - $(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@ > > +$(TEST_GEN_PROGS): ../kselftest_harness.h > > > > -TEST_PROGS += $(BINARIES) > > -EXTRA_CLEAN := $(BINARIES) > > +all: $(TEST_GEN_PROGS) > > > > -all: $(BINARIES) > > > It shouldn't be that complicated. We just need to define TEST_GEN_PROGS > before including lib.mk, and then add the dependency on the harness > after we include lib.mk (so that TEST_GEN_PROGS has been updated to > prefix $(OUTPUT)). > > eg: > > # SPDX-License-Identifier: GPL-2.0 > CFLAGS += -Wl,-no-as-needed -Wall > LDFLAGS += -lpthread > > TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark > > include ../lib.mk > > $(TEST_GEN_PROGS): ../kselftest_harness.h Exactly. This (with an extra comment) is precisely what I suggested during v2 review: https://lore.kernel.org/lkml/202003041815.B8C73DEC@keescook/ -Kees > > > Normal in-tree build: > > selftests$ make TARGETS=seccomp > make[1]: Entering directory '/home/michael/linux/tools/testing/selftests/seccomp' > gcc -Wl,-no-as-needed -Wall -lpthread seccomp_bpf.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_bpf > gcc -Wl,-no-as-needed -Wall -lpthread seccomp_benchmark.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_benchmark > make[1]: Leaving directory '/home/michael/linux/tools/testing/selftests/seccomp' > > selftests$ ls -l seccomp/ > total 388 > -rw-rw-r-- 1 michael michael 41 Jan 9 12:00 config > -rw-rw-r-- 1 michael michael 201 Mar 16 23:04 Makefile > -rwxrwxr-x 1 michael michael 70824 Mar 16 23:07 seccomp_benchmark* > -rw-rw-r-- 1 michael michael 2289 Feb 17 21:39 seccomp_benchmark.c > -rwxrwxr-x 1 michael michael 290520 Mar 16 23:07 seccomp_bpf* > -rw-rw-r-- 1 michael michael 94778 Mar 5 23:33 seccomp_bpf.c > > > O= build: > > selftests$ make TARGETS=seccomp O=$PWD/build > make[1]: Entering directory '/home/michael/linux/tools/testing/selftests/seccomp' > gcc -Wl,-no-as-needed -Wall -lpthread seccomp_bpf.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/build/seccomp/seccomp_bpf > gcc -Wl,-no-as-needed -Wall -lpthread seccomp_benchmark.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/build/seccomp/seccomp_benchmark > make[1]: Leaving directory '/home/michael/linux/tools/testing/selftests/seccomp' > > selftests$ ls -l build/seccomp/ > total 280 > -rwxrwxr-x 1 michael michael 70824 Mar 16 23:05 seccomp_benchmark* > -rwxrwxr-x 1 michael michael 290520 Mar 16 23:05 seccomp_bpf* > > > Build in the directory itself: > selftests$ cd seccomp > seccomp$ make > gcc -Wl,-no-as-needed -Wall -lpthread seccomp_bpf.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_bpf > gcc -Wl,-no-as-needed -Wall -lpthread seccomp_benchmark.c ../kselftest_harness.h -o /home/michael/linux/tools/testing/selftests/seccomp/seccomp_benchmark > > seccomp$ ls -l > total 388 > -rw-rw-r-- 1 michael michael 41 Jan 9 12:00 config > -rw-rw-r-- 1 michael michael 201 Mar 16 23:04 Makefile > -rwxrwxr-x 1 michael michael 70824 Mar 16 23:06 seccomp_benchmark* > -rw-rw-r-- 1 michael michael 2289 Feb 17 21:39 seccomp_benchmark.c > -rwxrwxr-x 1 michael michael 290520 Mar 16 23:06 seccomp_bpf* > -rw-rw-r-- 1 michael michael 94778 Mar 5 23:33 seccomp_bpf.c > > > cheers -- Kees Cook ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) 2020-03-16 21:05 ` Kees Cook @ 2020-03-19 3:15 ` Michael Ellerman 2020-03-23 20:18 ` Shuah Khan 0 siblings, 1 reply; 8+ messages in thread From: Michael Ellerman @ 2020-03-19 3:15 UTC (permalink / raw) To: Kees Cook Cc: Shuah Khan, shuah, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx, khilman, linux-kselftest, linux-kernel, netdev, bpf Kees Cook <keescook@chromium.org> writes: > On Mon, Mar 16, 2020 at 11:12:57PM +1100, Michael Ellerman wrote: >> Shuah Khan <skhan@linuxfoundation.org> writes: >> > Fix seccomp relocatable builds. This is a simple fix to use the right >> > lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h >> > header, and defining LDFLAGS for pthread lib. >> > >> > Removes custom clean rule which is no longer necessary with the use of >> > TEST_GEN_PROGS. >> > >> > Uses $(OUTPUT) defined in lib.mk to handle build relocation. >> > >> > The following use-cases work with this change: >> > >> > In seccomp directory: >> > make all and make clean >> > >> > From top level from main Makefile: >> > make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \ >> > CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp >> > >> > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> >> > --- >> > >> > Changes since v2: >> > -- Using TEST_GEN_PROGS is sufficient to generate objects. >> > Addresses review comments from Kees Cook. >> > >> > tools/testing/selftests/seccomp/Makefile | 18 ++++++++---------- >> > 1 file changed, 8 insertions(+), 10 deletions(-) >> > >> > diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile >> > index 1760b3e39730..a0388fd2c3f2 100644 >> > --- a/tools/testing/selftests/seccomp/Makefile >> > +++ b/tools/testing/selftests/seccomp/Makefile >> > @@ -1,17 +1,15 @@ >> > # SPDX-License-Identifier: GPL-2.0 >> > -all: >> > - >> > -include ../lib.mk >> > +CFLAGS += -Wl,-no-as-needed -Wall >> > +LDFLAGS += -lpthread >> > >> > .PHONY: all clean >> > >> > -BINARIES := seccomp_bpf seccomp_benchmark >> > -CFLAGS += -Wl,-no-as-needed -Wall >> > +include ../lib.mk >> > + >> > +# OUTPUT set by lib.mk >> > +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark >> > >> > -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h >> > - $(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@ >> > +$(TEST_GEN_PROGS): ../kselftest_harness.h >> > >> > -TEST_PROGS += $(BINARIES) >> > -EXTRA_CLEAN := $(BINARIES) >> > +all: $(TEST_GEN_PROGS) >> > >> > -all: $(BINARIES) >> >> >> It shouldn't be that complicated. We just need to define TEST_GEN_PROGS >> before including lib.mk, and then add the dependency on the harness >> after we include lib.mk (so that TEST_GEN_PROGS has been updated to >> prefix $(OUTPUT)). >> >> eg: >> >> # SPDX-License-Identifier: GPL-2.0 >> CFLAGS += -Wl,-no-as-needed -Wall >> LDFLAGS += -lpthread >> >> TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark >> >> include ../lib.mk >> >> $(TEST_GEN_PROGS): ../kselftest_harness.h > > Exactly. This (with an extra comment) is precisely what I suggested during > v2 review: > https://lore.kernel.org/lkml/202003041815.B8C73DEC@keescook/ Oh sorry, I missed that. OK so I think we know what the right solution is. cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) 2020-03-19 3:15 ` Michael Ellerman @ 2020-03-23 20:18 ` Shuah Khan 2020-03-23 20:50 ` Kees Cook 0 siblings, 1 reply; 8+ messages in thread From: Shuah Khan @ 2020-03-23 20:18 UTC (permalink / raw) To: Michael Ellerman, Kees Cook Cc: shuah, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx, khilman, linux-kselftest, linux-kernel, netdev, bpf Hi Michael and Kees, On 3/18/20 9:15 PM, Michael Ellerman wrote: > Kees Cook <keescook@chromium.org> writes: >> On Mon, Mar 16, 2020 at 11:12:57PM +1100, Michael Ellerman wrote: >>> Shuah Khan <skhan@linuxfoundation.org> writes: >>>> Fix seccomp relocatable builds. This is a simple fix to use the right >>>> lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h >>>> header, and defining LDFLAGS for pthread lib. >>>> >>>> Removes custom clean rule which is no longer necessary with the use of >>>> TEST_GEN_PROGS. >>>> >>>> Uses $(OUTPUT) defined in lib.mk to handle build relocation. >>>> >>>> The following use-cases work with this change: >>>> >>>> In seccomp directory: >>>> make all and make clean >>>> >>>> From top level from main Makefile: >>>> make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \ >>>> CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp >>>> >>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> >>>> --- >>>> >>>> Changes since v2: >>>> -- Using TEST_GEN_PROGS is sufficient to generate objects. >>>> Addresses review comments from Kees Cook. >>>> >>>> tools/testing/selftests/seccomp/Makefile | 18 ++++++++---------- >>>> 1 file changed, 8 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile >>>> index 1760b3e39730..a0388fd2c3f2 100644 >>>> --- a/tools/testing/selftests/seccomp/Makefile >>>> +++ b/tools/testing/selftests/seccomp/Makefile >>>> @@ -1,17 +1,15 @@ >>>> # SPDX-License-Identifier: GPL-2.0 >>>> -all: >>>> - >>>> -include ../lib.mk >>>> +CFLAGS += -Wl,-no-as-needed -Wall >>>> +LDFLAGS += -lpthread >>>> >>>> .PHONY: all clean >>>> >>>> -BINARIES := seccomp_bpf seccomp_benchmark >>>> -CFLAGS += -Wl,-no-as-needed -Wall >>>> +include ../lib.mk >>>> + >>>> +# OUTPUT set by lib.mk >>>> +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark >>>> >>>> -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h >>>> - $(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@ >>>> +$(TEST_GEN_PROGS): ../kselftest_harness.h >>>> >>>> -TEST_PROGS += $(BINARIES) >>>> -EXTRA_CLEAN := $(BINARIES) >>>> +all: $(TEST_GEN_PROGS) >>>> >>>> -all: $(BINARIES) >>> >>> >>> It shouldn't be that complicated. We just need to define TEST_GEN_PROGS >>> before including lib.mk, and then add the dependency on the harness >>> after we include lib.mk (so that TEST_GEN_PROGS has been updated to >>> prefix $(OUTPUT)). >>> >>> eg: >>> >>> # SPDX-License-Identifier: GPL-2.0 >>> CFLAGS += -Wl,-no-as-needed -Wall >>> LDFLAGS += -lpthread >>> >>> TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark >>> >>> include ../lib.mk >>> >>> $(TEST_GEN_PROGS): ../kselftest_harness.h >> >> Exactly. This (with an extra comment) is precisely what I suggested during >> v2 review: >> https://lore.kernel.org/lkml/202003041815.B8C73DEC@keescook/ > > Oh sorry, I missed that. Sorry. I missed it as well. > > OK so I think we know what the right solution is. > I am picking this back up after time off. The proposed change by you works for seccomp. There are at least 10+ tests that have dependencies on kselftest_harness.h and several that have dependency on kselftest.h and kselftest_module.h Enforcing this local header dependency in lib.mk makes sense so we don't have to change the test make files. Add dependency to libk.mk on local headers. This enforces the dependency blindly even when a test doesn't include the file, with the benefit of a simpler enforcement without requiring individual tests to have special rule for it. The following two changes work. You both have better make foo than I do. Can you see any issues with this proposal? I can send patch to do this, so we can do a larger test. -------------------------------------------------------------- diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 3ed0134a764d..54caa9a4ec8a 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -137,7 +137,7 @@ endif # Selftest makefiles can override those targets by setting # OVERRIDE_TARGETS = 1. ifeq ($(OVERRIDE_TARGETS),) -$(OUTPUT)/%:%.c +$(OUTPUT)/%:%.c ../kselftest_harness.h ../kselftest.h $(LINK.c) $^ $(LDLIBS) -o $@ $(OUTPUT)/%.o:%.S diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile index a0388fd2c3f2..0ebfe8b0e147 100644 --- a/tools/testing/selftests/seccomp/Makefile +++ b/tools/testing/selftests/seccomp/Makefile @@ -2,14 +2,5 @@ CFLAGS += -Wl,-no-as-needed -Wall LDFLAGS += -lpthread -.PHONY: all clean - +TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark include ../lib.mk - -# OUTPUT set by lib.mk -TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark - -$(TEST_GEN_PROGS): ../kselftest_harness.h - -all: $(TEST_GEN_PROGS) - -------------------------------------------------------------- thanks, -- Shuah ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) 2020-03-23 20:18 ` Shuah Khan @ 2020-03-23 20:50 ` Kees Cook 0 siblings, 0 replies; 8+ messages in thread From: Kees Cook @ 2020-03-23 20:50 UTC (permalink / raw) To: Shuah Khan Cc: Michael Ellerman, shuah, luto, wad, daniel, kafai, yhs, andriin, gregkh, tglx, khilman, linux-kselftest, linux-kernel, netdev, bpf On Mon, Mar 23, 2020 at 02:18:29PM -0600, Shuah Khan wrote: > The following two changes work. You both have better make foo than > I do. Can you see any issues with this proposal? I can send patch > to do this, so we can do a larger test. > > -------------------------------------------------------------- > diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk > index 3ed0134a764d..54caa9a4ec8a 100644 > --- a/tools/testing/selftests/lib.mk > +++ b/tools/testing/selftests/lib.mk > @@ -137,7 +137,7 @@ endif > # Selftest makefiles can override those targets by setting > # OVERRIDE_TARGETS = 1. > ifeq ($(OVERRIDE_TARGETS),) > -$(OUTPUT)/%:%.c > +$(OUTPUT)/%:%.c ../kselftest_harness.h ../kselftest.h > $(LINK.c) $^ $(LDLIBS) -o $@ I don't think this will work because some tests are in subdirectories. The Makefile needs to know what the top-level directory of the selftest tree is (which I think is $(selfdir) ?) I think this might be more complete: $(OUTPUT)/%: %.c $(selfdir)/kselftest_harness.h $(selfdir)/kselftest.h $(LINK.c) $^ $(LDLIBS) -o $@ > diff --git a/tools/testing/selftests/seccomp/Makefile > b/tools/testing/selftests/seccomp/Makefile > index a0388fd2c3f2..0ebfe8b0e147 100644 > --- a/tools/testing/selftests/seccomp/Makefile > +++ b/tools/testing/selftests/seccomp/Makefile > @@ -2,14 +2,5 @@ > CFLAGS += -Wl,-no-as-needed -Wall > LDFLAGS += -lpthread > > -.PHONY: all clean > - > +TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark > include ../lib.mk > - > -# OUTPUT set by lib.mk > -TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark > - > -$(TEST_GEN_PROGS): ../kselftest_harness.h > - > -all: $(TEST_GEN_PROGS) > - This part looks right. :) -- Kees Cook ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-03-23 20:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-13 21:24 [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir) Shuah Khan 2020-03-13 23:18 ` Kees Cook 2020-03-13 23:38 ` Shuah Khan 2020-03-16 12:12 ` Michael Ellerman 2020-03-16 21:05 ` Kees Cook 2020-03-19 3:15 ` Michael Ellerman 2020-03-23 20:18 ` Shuah Khan 2020-03-23 20:50 ` Kees Cook
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).