Andrii Nakryiko [off-list ref] writes:

On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote:

quoted

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
                        pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
                        return err;
                }
+               /* Make sure module BTF fd is never 0, as kernel depends on it
+                * being > 0 to distinguish between vmlinux and module BTFs,
+                * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+                */
+               if (!fd) {
+                       fd = dup(0);

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

We can, e.g. using access("/proc/self/fd/0", F_OK), but I think just calling
open unconditonally and doing if (ret > 0) close(ret) is better. Also, do I

yeah, I like this idea, let's go with it

FYI some production environments may detect that FDs 0,1,2 are not
pointing to stdin, stdout, stderr and will force close whatever files are there
and open 0,1,2 with canonical files.

libbpf doesn't have to resort to such measures, but it would be prudent to
make libbpf operate on FDs > 2 for all bpf objects to make sure other
frameworks don't ruin libbpf's view of FDs.

oh well, even without those production complications this would be a
bit fragile, e.g., if the application temporarily opened FD 0 and then
closed it.

Ok, Kumar, can you please do it as a simple helper that would
dup()'ing until we have FD>2, and use it in as few places as possible
to make sure that all FDs (not just module BTF) are covered. I'd
suggest doing that only in low-level helpers in btf.c, I think
libbpf's logic always goes through those anyways (but please
double-check that we don't call bpf syscall directly anywhere else).

FYI, you can use fcntl() with F_DUPFD{,_CLOEXEC} and tell it the minimum
fd number you're interested in for the clone. We do that in libxdp to
protect against fd 0:

https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1184

Given Alexei's comments above, maybe we should be '3' for the last arg
instead of 1...

-Toke