26 July 2024 at 14:15, "Yonghong Song" <yonghong.song@xxxxxxxxx> wrote:
>
> On 7/25/24 8:27 PM, leon.hwang@xxxxxxxxx wrote:
>
> >
> > 26 July 2024 at 04:58, "Yonghong Song" <yonghong.song@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > >
> > > 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
