Re: [PATCH bpf-next v2] bpf: Fix issue with bpf preload module taking over stdout/stdin of kernel.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux