On Tue, Jul 12, 2022 at 4:02 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> On Tue, Jul 12, 2022 at 3:09 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Tue, Jul 5, 2022 at 5:26 PM Song Liu <song@xxxxxxxxxx> wrote:
> > >
> > > syzbot reported a few issues with bpf_prog_pack [1], [2]. These are
> > > triggered when the program passed initial JIT in jit_subprogs(), but
> > > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is
> > > called before bpf_jit_binary_pack_finalize(), and the whole 2MB page is
> > > freed.
> > >
> > > Fix this with a custom bpf_jit_free() for x86_64, which calls
> > > bpf_jit_binary_pack_finalize() if necessary. Also, with custom
> > > bpf_jit_free(), bpf_prog_aux->use_bpf_prog_pack is not needed any more,
> > > remove it.
> > >
> > > Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc")
> > > [1] https://syzkaller.appspot.com/bug?extid=2f649ec6d2eea1495a8f
> > > [2] https://syzkaller.appspot.com/bug?extid=87f65c75f4a72db05445
> > > Reported-by: syzbot+2f649ec6d2eea1495a8f@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Reported-by: syzbot+87f65c75f4a72db05445@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: Song Liu <song@xxxxxxxxxx>
> > > ---
> > > arch/x86/net/bpf_jit_comp.c | 25 +++++++++++++++++++++++++
> > > include/linux/bpf.h | 1 -
> > > include/linux/filter.h | 8 ++++++++
> > > kernel/bpf/core.c | 29 ++++++++++++-----------------
> > > 4 files changed, 45 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index c98b8c0ed3b8..c3dca4c97e48 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -2492,3 +2492,28 @@ void *bpf_arch_text_copy(void *dst, void *src, size_t len)
> > > return ERR_PTR(-EINVAL);
> > > return dst;
> > > }
> > > +
> > > +void bpf_jit_free(struct bpf_prog *prog)
> > > +{
> > > + if (prog->jited) {
> > > + struct x64_jit_data *jit_data = prog->aux->jit_data;
> > > + struct bpf_binary_header *hdr;
> > > +
> > > + /*
> > > + * If we fail the final pass of JIT (from jit_subprogs),
> > > + * the program may not be finalized yet. Call finalize here
> > > + * before freeing it.
> > > + */
> > > + if (jit_data) {
> > > + bpf_jit_binary_pack_finalize(prog, jit_data->header,
> > > + jit_data->rw_header);
> > > + kvfree(jit_data->addrs);
> > > + kfree(jit_data);
> > > + }
> >
> > It looks like a workaround for missed cleanup on the JIT side.
> > When bpf_int_jit_compile() fails it is supposed to free jit_data
> > immediately.
> >
> > > passed initial JIT in jit_subprogs(), but
> > > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is
> > > called before bpf_jit_binary_pack_finalize()
> >
> > It feels that bpf_int_jit_compile() should call
> > bpf_jit_binary_pack_finalize() instead in the path where
> > it's failing.
> > I could be missing details on what exactly
> > "failed final pass of JIT" means.
>
> This only happens with multiple subprogs. In jit_subprogs(), we
> first call bpf_int_jit_compile() on each sub program. And then,
> we call it on each sub program again. jit_data is not freed in the
> first call of bpf_int_jit_compile(). Similarly we don't call
> bpf_jit_binary_pack_finalize() in the first call of bpf_int_jit_compile().
>
> If bpf_int_jit_compile() failed for one sub program, we will call
> bpf_jit_binary_pack_finalize() for this sub program. However,
> we don't have a chance to call it for other sub programs. Then
> we will hit "goto out_free" in jit_subprogs(), and call bpf_jit_free
> on some subprograms that haven't got bpf_jit_binary_pack_finalize()
> yet. So, I think bpf_jit_free is the best place we can add the extra
> check and call bpf_jit_binary_pack_finalize().
>
> Does this make sense?
Got it. I've amended the commit log with the above details.
Considering that we're at rc6 and the amount of conflicts
this patch would cause between bpf and bpf-next trees
I pushed it to bpf-next after applying manually.
Thanks.
