| Message ID | 20211006002853.308945-4-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Typeless/weak ksym for gen_loader + misc fixups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: kpsingh@kernel.org john.fastabend@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | warning | WARNING: line length of 102 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next | success | VM_Test |
On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > 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? Doing something like that in bpf_object__open() or bpf_object__load() would make everything much simpler and we'll have a guarantee that fd == 0 is not going to be allocated (unless someone accidentally or not accidentally does close(0), but that's entirely different story). > + if (fd < 0) { > + err = -errno; > + pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err); > + close(0); > + return err; > + } > + close(0); > + } > > len = sizeof(info); > memset(&info, 0, sizeof(info)); > -- > 2.33.0 >
On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote: > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > 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 leave it lingering, or should I close(0) if we created it on destroy? > Doing something like that in bpf_object__open() or bpf_object__load() > would make everything much simpler and we'll have a guarantee that fd > == 0 is not going to be allocated (unless someone accidentally or not > accidentally does close(0), but that's entirely different story). > > > + if (fd < 0) { > > + err = -errno; > > + pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err); > > + close(0); > > + return err; > > + } > > + close(0); > > + } > > > > len = sizeof(info); > > memset(&info, 0, sizeof(info)); > > -- > > 2.33.0 > > -- Kartikeya
On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote: > > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > 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 > leave it lingering, or should I close(0) if we created it on destroy? I don't mind leaving it open indefinitely, but can you please check that it doesn't trigger LeakSanitizer errors? > > > Doing something like that in bpf_object__open() or bpf_object__load() > > would make everything much simpler and we'll have a guarantee that fd > > == 0 is not going to be allocated (unless someone accidentally or not > > accidentally does close(0), but that's entirely different story). > > > > > + if (fd < 0) { > > > + err = -errno; > > > + pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err); > > > + close(0); > > > + return err; > > > + } > > > + close(0); > > > + } > > > > > > len = sizeof(info); > > > memset(&info, 0, sizeof(info)); > > > -- > > > 2.33.0 > > > > > -- > Kartikeya
On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote: > > > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > 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.
On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote: > > > > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > > > 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).
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko >> <andrii.nakryiko@gmail.com> wrote: >> > >> > On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi >> > <memxor@gmail.com> wrote: >> > > >> > > On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote: >> > > > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> > > > > >> > > > > 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
On Thu, Oct 07, 2021 at 03:54:34PM IST, Toke Høiland-Jørgensen wrote: > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > >> > >> On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko > >> <andrii.nakryiko@gmail.com> wrote: > >> > > >> > On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi > >> > <memxor@gmail.com> wrote: > >> > > > >> > > On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote: > >> > > > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >> > > > > > >> > > > > 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). > Just to make sure I am on the same page: I have to... 1. Add a small wrapper (currently named fd_gt_2, any other suggestions?) that takes in the fd, and dups it to fd >= 3 if in range [0, 2] (and closes original fd in this case). Use this for all fd returning bpf syscalls in bpf.c (btf.c is a typo?). Audit other places directly calling syscall(__NR_bpf, ...). 2. Assume that the situation Alexei mentioned only occurs at startup, or sometime later, not in parallel (which would race with us, and not sure we can deal with it). I'm thinking of a case where such an fd gets passed to an exec'd binary which closes invalids fds on startup (so keeping them >= 3 allows proper inheritance). 3. gen_loader can hit the same case, so short of adding a bpf_sys_fcntl (or the helper only exposing F_DUPFD), next best option is to reserve the three fds from skel_internal.h or gen_trace (in bpftool) and close later after loading is done. Feedback needed on 3 (and whether a generic bpf_sys_dup providing functionality of existing fcntl and dup{,2,3} is better than simply reserving the three fds at load time). > 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: > Thanks, will switch the dup to fcntl in the next version. > 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 > -- Kartikeya
On Thu, Oct 7, 2021 at 11:44 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, Oct 07, 2021 at 03:54:34PM IST, Toke Høiland-Jørgensen wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > > > On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > >> > > >> On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko > > >> <andrii.nakryiko@gmail.com> wrote: > > >> > > > >> > On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi > > >> > <memxor@gmail.com> wrote: > > >> > > > > >> > > On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote: > > >> > > > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > >> > > > > > > >> > > > > 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). > > > > Just to make sure I am on the same page: > > I have to... > 1. Add a small wrapper (currently named fd_gt_2, any other suggestions?) ensure_good_fd() or something? 2 is a tiny detail there. > that takes in the fd, and dups it to fd >= 3 if in range [0, 2] (and closes > original fd in this case). > Use this for all fd returning bpf syscalls in bpf.c (btf.c is a typo?). yep, typo, I meant bpf.c > Audit other places directly calling syscall(__NR_bpf, ...). yep > 2. Assume that the situation Alexei mentioned only occurs at startup, or > sometime later, not in parallel (which would race with us, and not sure > we can deal with it). I'm thinking of a case where such an fd gets passed > to an exec'd binary which closes invalids fds on startup (so keeping them > >= 3 allows proper inheritance). with checking it next to syscall(__NR_bpf) and fcntl suggestion from Toke, why does it matter? > 3. gen_loader can hit the same case, so short of adding a bpf_sys_fcntl (or the > helper only exposing F_DUPFD), next best option is to reserve the three fds from > skel_internal.h or gen_trace (in bpftool) and close later after loading is done. Not sure, will defer to Alexei. > > Feedback needed on 3 (and whether a generic bpf_sys_dup providing functionality of > existing fcntl and dup{,2,3} is better than simply reserving the three fds at > load time). > > > 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: > > > > Thanks, will switch the dup to fcntl in the next version. > > > 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 > > > > -- > Kartikeya
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); + if (fd < 0) { + err = -errno; + pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err); + close(0); + return err; + } + close(0); + } len = sizeof(info); memset(&info, 0, sizeof(info));
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(+)