On Fri, Feb 25, 2022 at 9:52 AM Yucong Sun <fallentree@xxxxxx> wrote:
>
> In a previous commit (1), BPF preload process was switched from user
> mode process to use in-kernel light skeleton instead. However, in the
> kernel context the available fd starts from 0, instead of normally 3 for
> user mode process. and the preload process leaked two FDs, taking over
> FD 0 and 1. This which later caused issues when kernel trys to setup
> stdin/stdout/stderr for init process, assuming fd 0,1,2 is available.
>
> As seen here:
>
> Before fix:
> ls -lah /proc/1/fd/*
>
> lrwx------1 root root 64 Feb 23 17:20 /proc/1/fd/0 -> /dev/null
> lrwx------ 1 root root 64 Feb 23 17:20 /proc/1/fd/1 -> /dev/null
> lrwx------ 1 root root 64 Feb 23 17:20 /proc/1/fd/2 -> /dev/console
> lrwx------ 1 root root 64 Feb 23 17:20 /proc/1/fd/6 -> /dev/console
> lrwx------ 1 root root 64 Feb 23 17:20 /proc/1/fd/7 -> /dev/console
>
> After Fix / Normal:
>
> ls -lah /proc/1/fd/*
>
> lrwx------ 1 root root 64 Feb 24 21:23 /proc/1/fd/0 -> /dev/console
> lrwx------ 1 root root 64 Feb 24 21:23 /proc/1/fd/1 -> /dev/console
> lrwx------ 1 root root 64 Feb 24 21:23 /proc/1/fd/2 -> /dev/console
>
> In this patch:
> - skel_closenz was changed to skel_closegez to correctly handle
> FD=0 case.
> - various places detecting FD > 0 was changed to FD >= 0.
> - Call iterators_skel__detach() funciton to release FDs after links
> are obtained.
>
> 1: commit cb80ddc67152 ("bpf: Convert bpf_preload.ko to use light skeleton.")
>
> Fixes: commit cb80ddc67152 ("bpf: Convert bpf_preload.ko to use light skeleton.")
> Signed-off-by: Yucong Sun <fallentree@xxxxxx>
>
> V2 -> V1: rename skel_closenez to skel_closegez, added comment as
> requested.
> ---
> kernel/bpf/preload/bpf_preload_kern.c | 4 ++++
> kernel/bpf/preload/iterators/iterators.lskel.h | 16 +++++++++-------
> tools/bpf/bpftool/gen.c | 9 +++++----
> tools/lib/bpf/skel_internal.h | 8 ++++----
> 4 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/bpf/preload/bpf_preload_kern.c b/kernel/bpf/preload/bpf_preload_kern.c
> index 30207c048d36..3cc8bbfd15b1 100644
> --- a/kernel/bpf/preload/bpf_preload_kern.c
> +++ b/kernel/bpf/preload/bpf_preload_kern.c
> @@ -14,6 +14,8 @@ static void free_links_and_skel(void)
> bpf_link_put(maps_link);
> if (!IS_ERR_OR_NULL(progs_link))
> bpf_link_put(progs_link);
> + /* __detach() was already called before this, __destory() will call it again, but
> + with no effect. */
> iterators_bpf__destroy(skel);
> }
>
> @@ -54,6 +56,8 @@ static int load_skel(void)
> err = PTR_ERR(progs_link);
> goto out;
> }
> + /* Release all FDs */
> + iterators_bpf__detach(skel);
> return 0;
> out:
> free_links_and_skel();
> diff --git a/kernel/bpf/preload/iterators/iterators.lskel.h b/kernel/bpf/preload/iterators/iterators.lskel.h
> index 70f236a82fe1..6a93538fa69f 100644
> --- a/kernel/bpf/preload/iterators/iterators.lskel.h
> +++ b/kernel/bpf/preload/iterators/iterators.lskel.h
> @@ -28,7 +28,7 @@ iterators_bpf__dump_bpf_map__attach(struct iterators_bpf *skel)
> int prog_fd = skel->progs.dump_bpf_map.prog_fd;
> int fd = skel_link_create(prog_fd, 0, BPF_TRACE_ITER);
>
> - if (fd > 0)
> + if (fd >= 0)
> skel->links.dump_bpf_map_fd = fd;
> return fd;
> }
> @@ -39,7 +39,7 @@ iterators_bpf__dump_bpf_prog__attach(struct iterators_bpf *skel)
> int prog_fd = skel->progs.dump_bpf_prog.prog_fd;
> int fd = skel_link_create(prog_fd, 0, BPF_TRACE_ITER);
>
> - if (fd > 0)
> + if (fd >= 0)
> skel->links.dump_bpf_prog_fd = fd;
> return fd;
> }
> @@ -57,8 +57,10 @@ iterators_bpf__attach(struct iterators_bpf *skel)
> static inline void
> iterators_bpf__detach(struct iterators_bpf *skel)
> {
> - skel_closenz(skel->links.dump_bpf_map_fd);
> - skel_closenz(skel->links.dump_bpf_prog_fd);
> + skel_closegez(skel->links.dump_bpf_map_fd);
> + skel->links.dump_bpf_map_fd = -1;
> + skel_closegez(skel->links.dump_bpf_prog_fd);
> + skel->links.dump_bpf_prog_fd = -1;
> }
> static void
> iterators_bpf__destroy(struct iterators_bpf *skel)
> @@ -66,10 +68,10 @@ iterators_bpf__destroy(struct iterators_bpf *skel)
> if (!skel)
> return;
> iterators_bpf__detach(skel);
> - skel_closenz(skel->progs.dump_bpf_map.prog_fd);
> - skel_closenz(skel->progs.dump_bpf_prog.prog_fd);
> + skel_closegez(skel->progs.dump_bpf_map.prog_fd);
> + skel_closegez(skel->progs.dump_bpf_prog.prog_fd);
> skel_free_map_data(skel->rodata, skel->maps.rodata.initial_value, 4096);
> - skel_closenz(skel->maps.rodata.map_fd);
> + skel_closegez(skel->maps.rodata.map_fd);
> skel_free(skel);
> }
> static inline struct iterators_bpf *
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 145734b4fe41..e5e65f507e00 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -469,7 +469,7 @@ static void codegen_attach_detach(struct bpf_object *obj, const char *obj_name)
> codegen("\
> \n\
> \n\
> - if (fd > 0) \n\
> + if (fd >= 0) \n\
> skel->links.%1$s_fd = fd; \n\
> return fd; \n\
> } \n\
> @@ -506,7 +506,8 @@ static void codegen_attach_detach(struct bpf_object *obj, const char *obj_name)
> bpf_object__for_each_program(prog, obj) {
> codegen("\
> \n\
> - skel_closenz(skel->links.%1$s_fd); \n\
> + skel_closegez(skel->links.%1$s_fd); \n\
> + skel->links.%1$s_fd = -1; \n\
> ", bpf_program__name(prog));
> }
>
> @@ -536,7 +537,7 @@ static void codegen_destroy(struct bpf_object *obj, const char *obj_name)
> bpf_object__for_each_program(prog, obj) {
> codegen("\
> \n\
> - skel_closenz(skel->progs.%1$s.prog_fd); \n\
> + skel_closegez(skel->progs.%1$s.prog_fd); \n\
> ", bpf_program__name(prog));
> }
>
> @@ -549,7 +550,7 @@ static void codegen_destroy(struct bpf_object *obj, const char *obj_name)
> ident, bpf_map_mmap_sz(map));
> codegen("\
> \n\
> - skel_closenz(skel->maps.%1$s.map_fd); \n\
> + skel_closegez(skel->maps.%1$s.map_fd); \n\
> ", ident);
> }
> codegen("\
> diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
> index bd6f4505e7b1..89c0b8632254 100644
> --- a/tools/lib/bpf/skel_internal.h
> +++ b/tools/lib/bpf/skel_internal.h
> @@ -204,11 +204,11 @@ static inline void *skel_finalize_map_data(__u64 *init_val, size_t mmap_sz, int
> }
> #endif
>
> -static inline int skel_closenz(int fd)
> +static inline int skel_closegez(int fd)
> {
> - if (fd > 0)
> - return close(fd);
> - return -EINVAL;
> + if (fd < 0)
> + return -EINVAL;
> + return close(fd);
> }
Unfortunately this won't work. Many places in gen_loader.c
rely on fd == 0 being a signal that fd wasn't allocated.
The global data, stack, loader_ctx, etc. All are zero initialized.
Thankfully no need to do any of these changes.
Just closing two link_fd in load_skel() is enough.
