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

[Yonghong Song <yhs@fb.com>]

On 2/24/22 1:49 PM, Yucong Sun 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_closenez 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: https://github.com/kernel-patches/bpf/commit/cb80ddc67152e72f28ff6ea8517acdf875d7381d
> 
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---
>   kernel/bpf/preload/bpf_preload_kern.c          |  1 +
>   kernel/bpf/preload/iterators/iterators.lskel.h | 16 +++++++++-------
>   tools/bpf/bpftool/gen.c                        |  9 +++++----
>   tools/lib/bpf/skel_internal.h                  |  8 ++++----
>   4 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/bpf/preload/bpf_preload_kern.c b/kernel/bpf/preload/bpf_preload_kern.c
> index 30207c048d36..c6bb1e72e0f1 100644
> --- a/kernel/bpf/preload/bpf_preload_kern.c
> +++ b/kernel/bpf/preload/bpf_preload_kern.c
> @@ -54,6 +54,7 @@ static int load_skel(void)
>   		err = PTR_ERR(progs_link);
>   		goto out;
>   	}
> +	iterators_bpf__detach(skel);

In fini, we have:

static void __exit fini(void)
{
         bpf_preload_ops = NULL;
         free_links_and_skel();
}

static void free_links_and_skel(void)
{
         if (!IS_ERR_OR_NULL(maps_link))
                 bpf_link_put(maps_link);
         if (!IS_ERR_OR_NULL(progs_link))
                 bpf_link_put(progs_link);
         iterators_bpf__destroy(skel);
}

Since you did iterators_bpf__detach(skel) in load_skel(),
in fini(), we don't need iterators_bpf__destroy(skel), right?

>   	return 0;
>   out:
>   	free_links_and_skel();
[...]