On Tue, Apr 23, 2024 at 5:29 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 4/23/24 10:16 AM, Kui-Feng Lee wrote:
> >
> >
> > On 4/22/24 16:43, Martin KaFai Lau wrote:
> >> On 4/22/24 10:30 AM, Kui-Feng Lee wrote:
> >>>
> >>>
> >>> On 4/22/24 10:12, Kui-Feng Lee wrote:
> >>>>
> >>>>
> >>>> On 4/19/24 17:05, Martin KaFai Lau wrote:
> >>>>> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
> >>>>>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
> >>>>>> +{
> >>>>>> + struct bpf_struct_ops_link *link = inode->i_private;
> >>>>>> +
> >>>>>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
> >>>>>> + bpf_link_inc(&link->link);
> >>>>>> + filp->private_data = link;
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> >>>>>> index af5d2ffadd70..b020d761ab0a 100644
> >>>>>> --- a/kernel/bpf/inode.c
> >>>>>> +++ b/kernel/bpf/inode.c
> >>>>>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry, umode_t
> >>>>>> mode, void *arg)
> >>>>>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
> >>>>>> {
> >>>>>> + const struct file_operations *fops;
> >>>>>> struct bpf_link *link = arg;
> >>>>>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
> >>>>>> - bpf_link_is_iter(link) ?
> >>>>>> - &bpf_iter_fops : &bpffs_obj_fops);
> >>>>>> + if (bpf_link_is_iter(link))
> >>>>>> + fops = &bpf_iter_fops;
> >>>>>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
> >>>>>
> >>>>> Open a pinned link and then update should not be specific to struct_ops
> >>>>> link. e.g. should be useful to the cgroup link also?
> >>>>
> >>>> It could be. Here, I played safe in case it creates any unwanted side
> >>>> effect for links of unknown types.
> >>>
> >>> By the way, may I put it in a follow up patch if we want cgroup links?
> >>
> >> This does not feel right. It is not struct_ops specific.
> >>
> >> Before we dive in further, there is BPF_OBJ_GET which can get a fd of a pinned
> >> bpf obj (prog, map, and link). Take a look at bpf_link__open() in libbpf. Does
> >> it work for the use case that needs to update the link?
> >>
> > It should work.
> > So, this patch is not necessary. However, it is still a nice and
> > intuitive feature. WDYT?
>
> There is already BPF_OBJ_GET which works for all major bpf obj types (prog, map,
> and link). Having open only works for link and only works for one link type
> (struct_ops) is not very convincing.
>
> Beside, I am not sure how the file flags (e.g. rdonly...etc) should be handled.
> I don't know enough in this area, so I will defer to others to comment in
> general the usefulness and the approach.
>
>
Didn't see this discussion before replying on the other patch. But I
agree with Martin, we already use the BPF_OBJ_GET method, and we don't
support directly open()'ing progs/maps, so I don't think we should do
this for links either.
pw-bot: cr
