Re: [PATCH bpf-next 1/7] bpf: Create links for BPF struct_ops maps.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/15/23 10:44, Stanislav Fomichev wrote:
> On 02/15, Kui-Feng Lee wrote:
>> Thank you for the feedback.
>
>
>> On 2/14/23 18:39, Stanislav Fomichev wrote:
>> > On 02/14, Kui-Feng Lee wrote:
[..]
>> >
>> > > +��� if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS)
>> > > +������� return -EINVAL;
>> > > +
>> > > +��� link = kzalloc(sizeof(*link), GFP_USER);
>> > > +��� if (!link) {
>> > > +������� err = -ENOMEM;
>> > > +������� goto err_out;
>> > > +��� }
>> > > +��� bpf_link_init(link, BPF_LINK_TYPE_STRUCT_OPS,
>> > > &bpf_struct_ops_map_lops, NULL);
>> > > +��� link->map = map;
>> > > +
>> > > +��� err = bpf_link_prime(link, &link_primer);
>> > > +��� if (err)
>> > > +������� goto err_out;
>> > > +
>> > > +��� return bpf_link_settle(&link_primer);
>> > > +
>> > > +err_out:
>> > > +��� bpf_map_put(map);
>> > > +��� kfree(link);
>> > > +��� return err;
>> > > +}
>> > > +
>> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> > > index cda8d00f3762..54e172d8f5d1 100644
>> > > --- a/kernel/bpf/syscall.c
>> > > +++ b/kernel/bpf/syscall.c
>> > > @@ -2738,7 +2738,9 @@ static void bpf_link_free(struct bpf_link 
>> *link)
>> > > ����� if (link->prog) {
>> > > ��������� /* detach BPF program, clean up used resources */
>> > > ��������� link->ops->release(link);
>> > > -������� bpf_prog_put(link->prog);
>> > > +������� if (link->type != BPF_LINK_TYPE_STRUCT_OPS)
>> > > +����������� bpf_prog_put(link->prog);
>> > > +������� /* The struct_ops links clean up map by them-selves. */
>> >
>> > Why not more generic:
>> >
>> > if (link->prog)
>> > ����bpf_prog_put(link->prog);
>> >
>> > ?
>> The `prog` and `map` functions are now occupying the same space. I'm 
>> afraid
>> this check won't work.
>
> Hmm, good point. In this case: why not have separate prog/map pointers
> instead of a union? Are we 100% sure struct_ops is unique enough
> and there won't ever be another map-based links?
>
Good question!

bpf_link is used to attach a single BPF program with a hook now. This 
patch takes things one step further, allowing the attachment of 
struct_ops. We can think of it as attaching a set of BPF programs to a 
pre-existing set of hooks. I would say that bpf_link now represents the 
attachments of a set of BPF programs to a pre-defined set of hooks. The 
size of a set is one for the case of attaching single BPF program.

Is creating a new map-based link indicative of introducing an entirely 
distinct type of map that reflects a set of BPF programs? If so, why 
doesn't struct_ops work? If it happens, we may need to create a function 
to validate the attach_type associated with any given map type.