Re: [PATCH bpf-next 1/2] bpf: Fix updating attached freplace prog to PROG_ARRAY map

["Leon Hwang" <leon.hwang@linux.dev>]

26 July 2024 at 14:15, "Yonghong Song" <yonghong.song@linux.dev> wrote:

> 
> On 7/25/24 8:27 PM, leon.hwang@linux.dev wrote:
> 
> > 
> > 26 July 2024 at 04:58, "Yonghong Song" <yonghong.song@linux.dev> wrote:
> > 
> > > 
> > > On 7/24/24 5:32 PM, Leon Hwang wrote:
> > > 
> > 
> >  The commit f7866c3587337731 ("bpf: Fix null pointer dereference in
> > 
> >  resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed the following panic,
> > 
> >  which was caused by updating attached freplace prog to PROG_ARRAY map.
> > 
> > > 
> > > I am confused here. You mentioned that commit f7866c3587337731
> > > 
> > >  fixed the panic below. But looking at commit message:
> > > 
> > >  https://lore.kernel.org/bpf/20240711145819.254178-2-wutengda@huaweicloud.com
> > > 
> > >  it does not seem the case.
> > > 
> > 
> >  The commit fixed this panic meanwhile.
> > 
> >  This panic seems confusing. I'll remove it in patch v2.
> > 
> 
> [...]
> 
> > 
> > ---
> > 
> >  include/linux/bpf_verifier.h | 4 ++--
> > 
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> >  diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > 
> >  index 5cea15c81b8a8..387e034e73d0e 100644
> > 
> >  --- a/include/linux/bpf_verifier.h
> > 
> >  +++ b/include/linux/bpf_verifier.h
> > 
> >  @@ -874,8 +874,8 @@ static inline u32 type_flag(u32 type)
> > 
> >  /* only use after check_attach_btf_id() */
> > 
> >  static inline enum bpf_prog_type resolve_prog_type(const struct bpf_prog *prog)
> > 
> >  {
> > 
> >  - return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->dst_prog) ?
> > 
> >  - prog->aux->dst_prog->type : prog->type;
> > 
> >  + return prog->type == BPF_PROG_TYPE_EXT ?
> > 
> >  + prog->aux->saved_dst_prog_type : prog->type;
> > 
> > > 
> > > If prog->aux->dst_prog is NULL, is it possible that prog->aux->saved_dst_prog_type
> > > 
> > >  (0, corresponding to BPF_PROG_TYPE_UNSPEC) could be returned? Do we need to do
> > > 
> > >  return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->saved_dst_prog_type) ?
> > > 
> > >  prog->aux->saved_dst_prog_type : prog->type;
> > > 
> > >  Maybe I missed something here?
> > > 
> > 
> >  It seems better to check prog->aux->saved_dst_prog_type. But I don't think so.
> > 
> >  prog->aux->saved_dst_prog_type is set in check_attach_btf_id(). And there is no
> > 
> >  resolve_prog_type() before check_attach_btf_id() in bpf_check().
> > 
> >  Therefore, resolve_prog_type() must be called after check_attach_btf_id().
> > 
> 
> In check_attach_btf_id(), I see
> 
>  if (tgt_prog) {
> 
>  prog->aux->saved_dst_prog_type = tgt_prog->type;
> 
>  prog->aux->saved_dst_attach_type = tgt_prog->expected_attach_type;
> 
>  }
> 
> So it is possible prog->aux->saved_dst_prog_type is 0 (default value).
> 
> I don't know that if tgt_prog is NULL, whether later resolve_prog_type()
> 
> will be called or not. Need more checking here.
>

This is the case that commit f7866c3587337731 ("bpf: Fix null pointer dereference
in resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed, which is loading freplace
prog without tgt_prog.

With this patch, while loading freplace prog without tgt_prog, resolve_prog_type()
returns 0 instead of BPF_PROG_TYPE_EXT.

It's better to return a meaningful prog type in resolve_prog_type() anyway.

I accept your suggestion.

Thanks,
Leon