On 2/15/23 12:24 PM, Kui-Feng Lee wrote:
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.
I also prefer separating 'prog' and 'map' in the link. It may be only struct_ops
link that has no prog now but the future link type may not have prog also, so
testing link->prog is less tie-up to a specific link type. Once separated, it
makes sense to push the link's specific field to a new struct, so the following
(from Stan) makes more sense:
struct bpt_struct_ops_link {
struct bpf_link link;
struct bpf_map *map;
};
In bpf_link_free(), there is no need to do an extra check for 'link->type !=
BPF_LINK_TYPE_STRUCT_OPS'. bpf_struct_ops_map_link_release() can be done
together in bpf_struct_ops_map_link_dealloc().
