Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir)

Michael Ellerman
2020-03-19T03:15:15Z

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