Stanislav Fomichev <sdf@xxxxxxxxxx> writes:
> On Thu, Dec 8, 2022 at 2:29 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>
>> Stanislav Fomichev <sdf@xxxxxxxxxx> writes:
>>
>> > Please see the first patch in the series for the overall
>> > design and use-cases.
>> >
>> > Changes since v3:
>> >
>> > - Rework prog->bound_netdev refcounting (Jakub/Marin)
>> >
>> > Now it's based on the offload.c framework. It mostly fits, except
>> > I had to automatically insert a HT entry for the netdev. In the
>> > offloaded case, the netdev is added via a call to
>> > bpf_offload_dev_netdev_register from the driver init path; with
>> > a dev-bound programs, we have to manually add (and remove) the entry.
>> >
>> > As suggested by Toke, I'm also prohibiting putting dev-bound programs
>> > into prog-array map; essentially prohibiting tail calling into it.
>> > I'm also disabling freplace of the dev-bound programs. Both of those
>> > restrictions can be loosened up eventually.
>>
>> I thought it would be a shame that we don't support at least freplace
>> programs from the get-go (as that would exclude libxdp from taking
>> advantage of this). So see below for a patch implementing this :)
>>
>> -Toke
>
> Damn, now I need to write a selftest :-)
> But seriously, thank you for taking care of this, will try to include
> preserving SoB!
Cool, thanks! I just realised I made on mistake in the attach check,
though:
[...]
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index b345a273f7d0..606e6de5f716 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(tgt_prog->aux) &&
>> + (bpf_prog_is_offloaded(tgt_prog->aux) ||
>> + !bpf_prog_is_dev_bound(prog->aux) ||
>> + !bpf_offload_dev_match(prog, tgt_prog->aux->offload->netdev))) {
This should switch the order of the is_dev_bound() checks, like:
+ 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))) {
I.e., first check bpf_prog_is_dev_bound(prog->aux) (the program being
attached), and only perform the other checks if we're attaching
something that has been verified as being dev-bound. It should be fine
to attach a non-devbound function to a devbound parent program (since
that non-devbound function can't call any of the kfuncs).
-Toke
