Message-ID: <c94b7005-70d3-a5ae-fc5b-a7cf5b2ea35d@isovalent.com>
Date:   Tue, 26 Jan 2021 15:41:50 +0000
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        David Gow <davidgow@...gle.com>,
        Masahiro Yamada <masahiroy@...nel.org>
Subject: Re: [PATCH] bpf: fix build for BPF preload when $(O) points to a
 relative path

2021-01-26 11:24 UTC+0000 ~ Quentin Monnet <quentin@...valent.com>
> 2021-01-25 16:32 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
>> On Mon, Jan 25, 2021 at 7:49 AM Quentin Monnet <quentin@...valent.com> wrote:
>>>
>>> Building the kernel with CONFIG_BPF_PRELOAD, and by providing a relative
>>> path for the output directory, may fail with the following error:
>>>
>>>   $ make O=build bindeb-pkg
>>>   ...
>>>   /.../linux/tools/scripts/Makefile.include:5: *** O=build does not exist.  Stop.
>>>   make[7]: *** [/.../linux/kernel/bpf/preload/Makefile:9: kernel/bpf/preload/libbpf.a] Error 2
>>>   make[6]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf/preload] Error 2
>>>   make[5]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf] Error 2
>>>   make[4]: *** [/.../linux/Makefile:1799: kernel] Error 2
>>>   make[4]: *** Waiting for unfinished jobs....
>>>
>>> In the case above, for the "bindeb-pkg" target, the error is produced by
>>> the "dummy" check in Makefile.include, called from libbpf's Makefile.
>>> This check changes directory to $(PWD) before checking for the existence
>>> of $(O). But at this step we have $(PWD) pointing to "/.../linux/build",
>>> and $(O) pointing to "build". So the Makefile.include tries in fact to
>>> assert the existence of a directory named "/.../linux/build/build",
>>> which does not exist.
>>>
>>> By contrast, other tools called from the main Linux Makefile get the
>>> variable set to $(abspath $(objtree)), where $(objtree) is ".". We can
>>> update the Makefile for kernel/bpf/preload to set $(O) to the same
>>> value, to permit compiling with a relative path for output. Note that
>>> apart from the Makefile.include, the variable $(O) is not used in
>>> libbpf's build system.
>>>
>>> Note that the error does not occur for all make targets and
>>> architectures combinations.
>>>
>>> - On x86, "make O=build vmlinux" appears to work fine.
>>>   $(PWD) points to "/.../linux/tools", but $(O) points to the absolute
>>>   path "/.../linux/build" and the test succeeds.
>>> - On UML, it has been reported to fail with a message similar to the
>>>   above (see [0]).
>>> - On x86, "make O=build bindeb-pkg" fails, as described above.
>>>
>>> It is unsure where the different values for $(O) and $(PWD) come from
>>> (likely some recursive make with different arguments at some point), and
>>> because several targets are broken, it feels safer to fix the $(O) value
>>> passed to libbpf rather than to hunt down all changes to the variable.
>>>
>>> David Gow previously posted a slightly different version of this patch
>>> as a RFC [0], two months ago or so.
>>>
>>> [0] https://lore.kernel.org/bpf/20201119085022.3606135-1-davidgow@google.com/t/#u
>>>
>>> Cc: Andrii Nakryiko <andrii.nakryiko@...il.com>
>>> Cc: Brendan Higgins <brendanhiggins@...gle.com>
>>> Cc: David Gow <davidgow@...gle.com>
>>> Reported-by: David Gow <davidgow@...gle.com>
>>> Signed-off-by: Quentin Monnet <quentin@...valent.com>
>>> ---
>>
>> I still think it would benefit everyone to figure out where this is
>> breaking (given Linux Makefile explicitly tries to handle such
>> relative path situation for O=, I believe), but this is trivial
>> enough, so:
>>
>> Acked-by: Andrii Nakryiko <andrii@...nel.org>
> 
> Agreed, I'll try to spend a bit more time on this when I can. But it
> would be nice to have the fix in the meantime. Thanks for the review and
> ack.

+Cc Masahiro Yamada

Looking further into this, my understanding is the following.

tools/scripts/Makefile.include contains this check:

	dummy := $(if $(shell cd $(PWD); test -d $(O) || \
		echo $(O)),$(error O=$(O) does not exist),)
	ABSOLUTE_O := $(shell cd $(PWD); cd $(O) ; pwd)

Note the use of $(PWD). As I understand, it is the shell environment
variable, as it was set when the initial "make" command was run. This
seems to be passed down to recursive calls to make. So if I type

	$ cd /linux
	$ make O=build vmlinux

Then I get $(PWD) set to "/linux" and $(O) set to "build". The Makefile
executes a submake from the output directory:

	# Invoke a second make in the output directory, passing relevant
	# variables
	__sub-make:
		$(Q)$(MAKE) -C $(abs_objtree) \
			-f $(abs_srctree)/Makefile $(MAKECMDGOALS)

But the variables are preserved. So far, so good.

When I try to build "bindeb-pkg" instead:

	$ cd /linux
	$ make O=build bindeb-pkg

Then I initially set $(PWD) and $(O) to the same values. They are
preserved after the call to the submake, after we have changed to the
output directory. But if I understand correctly, the "bindeb-pkg" target
writes a new Makefile as build/debian/rules in scripts/package/mkdebian,
and calls it _indirectly_ through dpkg-buildpackage, which does _not_
preserve $(PWD) (instead, it is reset to /linux/build, the current
directory when calling the script). I end up with $(O) set to "build",
and $(PWD) set to "/linux/build". The "dummy" check called for libbpf
fails to find "/linux/build/build".

Can we avoid using $(PWD) in the first place? I'm not sure how. It was
added in commit be40920fbf10 ("tools: Let O= makes handle a relative
path with -C option") to accommodate building perf with "-C", so we
could not replace $(PWD) with $$PWD (shell value at the time the
directive is executed) for example, the values will be different.

Can we unset $(O) so that, when we call dpkg-buildpackage, it reflects
the output directory relatively to /linux/build/? Or pass a value for
$(PWD), so it is preserved? Maybe, I don't know. It could be done in
scripts/package/mkdebian for example, by passing "O=''" to the make
call. It seems that the packages build well with this change. But then
we might need to check and update the other packages too (RPM, snap,
perf archives), and identify if something similar might be happening for
UML. I'm not sure this is worth the trouble at this point, if all we
want is to fix the eBPF preloads? But I'm open to discussion if this is
really the path we want to go.

Fixing $(O) to pass the dummy check is easier. However, when reading the
commits I noticed that my patch is incorrect. It would break on
something like "make O=~/build bindeb-pkg", because abspath does not
resolve special shell characters like "~". See also commit 028568d84da3
("kbuild: revert $(realpath ...) to $(shell cd ... && /bin/pwd)"): this
is why tools/scripts/Makefile.include still has an "ABSOLUTE_O"
variable. So instead of setting "O=$(abspath .)", I'll send a v2 with
Andrii's suggestion to use $(LIBBPF_OUT).

Quentin

Powered by blists - more mailing lists