Re: [PATCH bpf-next v4 06/15] bpf: Support consuming XDP HW metadata from fext programs

From: Stanislav Fomichev <sdf@google.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Martin KaFai Lau <martin.lau@linux.dev>,
	ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	song@kernel.org, yhs@fb.com, john.fastabend@gmail.com,
	kpsingh@kernel.org, haoluo@google.com, jolsa@kernel.org,
	bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v4 06/15] bpf: Support consuming XDP HW metadata from fext programs
Date: Wed, 14 Dec 2022 10:43:05 -0800	[thread overview]
Message-ID: <CAKH8qBudP_oZ55ZQe=j+VyOLycvdr+ec7P4pr+ztN9Y-Gv-Waw@mail.gmail.com> (raw)
In-Reply-To: <87cz8mgtcg.fsf@toke.dk>

On Wed, Dec 14, 2022 at 2:41 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Martin KaFai Lau <martin.lau@linux.dev> writes:
>
> > On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
> >>
> >> Instead of rejecting the attaching of PROG_TYPE_EXT programs to XDP
> >> programs that consume HW metadata, implement support for propagating the
> >> offload information. The extension program doesn't need to set a flag or
> >> ifindex, it these will just be propagated from the target by the verifier.
> >
> > s/it/because/ ... these will just be propagated....
>
> Yeah, or just drop 'it' :)
>
> >> We need to create a separate offload object for the extension program,
> >> though, since it can be reattached to a different program later (which
> >> means we can't just inhering the offload information from the target).
> >
> > hmm.... inheriting?
>
> Think I meant to write "we can't just inherit"
>
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index 11c558be4992..8686475f0dbe 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -3021,6 +3021,14 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> >>                      goto out_put_prog;
> >>              }
> >>
> >> +            if (bpf_prog_is_dev_bound(prog->aux) &&
> >
> >
> >> +                (bpf_prog_is_offloaded(tgt_prog->aux) ||
> >> +                 !bpf_prog_is_dev_bound(tgt_prog->aux) ||
> >> +                 !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {
> >
> > hmm... tgt_prog->aux->offload does not look safe without taking bpf_devs_lock.
> > offload could be NULL, no?
> >
> > It probably needs a bpf_prog_dev_bound_match(prog, tgt_prog) which takes the lock.
>
> Hmm, right, I was kinda expecting that this would not go away while
> tgt_prog was alive, but I see now that's not the case due to the
> unregister hook. So yeah, needs locking (same below) :)

Agreed, thanks! These seem easy enough to address on my side, so I'll
take care of them (and will keep your attribution).

> -Toke
>