On Wed, Dec 14, 2022 at 2:41 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> Martin KaFai Lau <martin.lau@xxxxxxxxx> writes:
>
> > On 12/12/22 6:35 PM, Stanislav Fomichev wrote:
> >> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
> >>
> >> 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
>
