Re: [PATCH bpf-next v4 06/15] bpf: Support consuming XDP HW metadata from fext programs - Toke Høiland-Jørgensen

From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Stanislav Fomichev <sdf@google.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 23:19:24 +0100	[thread overview]
Message-ID: <87k02tfx1v.fsf@toke.dk> (raw)
In-Reply-To: <CAKH8qBudP_oZ55ZQe=j+VyOLycvdr+ec7P4pr+ztN9Y-Gv-Waw@mail.gmail.com>

Stanislav Fomichev <sdf@google.com> writes:

> 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).

Awesome, thanks!

-Toke