| Message ID | 20240417002513.1534535-2-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Update a struct_ops link through a pinned path | expand |
Hi Kui-Feng, kernel test robot noticed the following build errors: [auto build test ERROR on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Kui-Feng-Lee/bpf-enable-the-open-operator-on-a-pinned-path-of-a-struct_osp-link/20240417-082736 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20240417002513.1534535-2-thinker.li%40gmail.com patch subject: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link. config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240418/202404180747.NXWUtZTG-lkp@intel.com/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7089c359a3845323f6f30c44a47dd901f2edfe63) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240418/202404180747.NXWUtZTG-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202404180747.NXWUtZTG-lkp@intel.com/ All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: bpffs_struct_ops_link_open >>> referenced by syscall.c >>> kernel/bpf/syscall.o:(bpf_link_open) in archive vmlinux.a
Hi Kui-Feng, kernel test robot noticed the following build errors: [auto build test ERROR on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Kui-Feng-Lee/bpf-enable-the-open-operator-on-a-pinned-path-of-a-struct_osp-link/20240417-082736 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20240417002513.1534535-2-thinker.li%40gmail.com patch subject: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link. config: arm-aspeed_g5_defconfig (https://download.01.org/0day-ci/archive/20240418/202404181413.1uMDy1xi-lkp@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240418/202404181413.1uMDy1xi-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202404181413.1uMDy1xi-lkp@intel.com/ All errors (new ones prefixed by >>): arm-linux-gnueabi-ld: kernel/bpf/syscall.o: in function `bpf_link_open': >> kernel/bpf/syscall.c:3117:(.text+0xb70): undefined reference to `bpffs_struct_ops_link_open' vim +3117 kernel/bpf/syscall.c 3110 3111 /* Support opening pinned links */ 3112 static int bpf_link_open(struct inode *inode, struct file *filp) 3113 { 3114 struct bpf_link *link = inode->i_private; 3115 3116 if (link->type == BPF_LINK_TYPE_STRUCT_OPS) > 3117 return bpffs_struct_ops_link_open(inode, filp); 3118 3119 return -EOPNOTSUPP; 3120 } 3121
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? Andrii, wdyt about supporting other link types also? > + fops = &bpf_link_fops; > + else > + fops = &bpffs_obj_fops; > + return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops); > } > > static struct dentry * > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 7d392ec83655..f66bc6215faa 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp) > } > #endif > > -static const struct file_operations bpf_link_fops = { > +/* Support opening pinned links */ > +static int bpf_link_open(struct inode *inode, struct file *filp) > +{ > + struct bpf_link *link = inode->i_private; > + > + if (link->type == BPF_LINK_TYPE_STRUCT_OPS) > + return bpffs_struct_ops_link_open(inode, filp); > + > + return -EOPNOTSUPP; > +} > + > +const struct file_operations bpf_link_fops = { > + .open = bpf_link_open, > #ifdef CONFIG_PROC_FS > .show_fdinfo = bpf_link_show_fdinfo, > #endif
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. > > Andrii, wdyt about supporting other link types also? > >> + fops = &bpf_link_fops; >> + else >> + fops = &bpffs_obj_fops; >> + return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops); >> } >> static struct dentry * >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >> index 7d392ec83655..f66bc6215faa 100644 >> --- a/kernel/bpf/syscall.c >> +++ b/kernel/bpf/syscall.c >> @@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct >> seq_file *m, struct file *filp) >> } >> #endif >> -static const struct file_operations bpf_link_fops = { >> +/* Support opening pinned links */ >> +static int bpf_link_open(struct inode *inode, struct file *filp) >> +{ >> + struct bpf_link *link = inode->i_private; >> + >> + if (link->type == BPF_LINK_TYPE_STRUCT_OPS) >> + return bpffs_struct_ops_link_open(inode, filp); >> + >> + return -EOPNOTSUPP; >> +} >> + >> +const struct file_operations bpf_link_fops = { >> + .open = bpf_link_open, >> #ifdef CONFIG_PROC_FS >> .show_fdinfo = bpf_link_show_fdinfo, >> #endif >
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? > >> >> Andrii, wdyt about supporting other link types also? >> >>> + fops = &bpf_link_fops; >>> + else >>> + fops = &bpffs_obj_fops; >>> + return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops); >>> } >>> static struct dentry * >>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >>> index 7d392ec83655..f66bc6215faa 100644 >>> --- a/kernel/bpf/syscall.c >>> +++ b/kernel/bpf/syscall.c >>> @@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct >>> seq_file *m, struct file *filp) >>> } >>> #endif >>> -static const struct file_operations bpf_link_fops = { >>> +/* Support opening pinned links */ >>> +static int bpf_link_open(struct inode *inode, struct file *filp) >>> +{ >>> + struct bpf_link *link = inode->i_private; >>> + >>> + if (link->type == BPF_LINK_TYPE_STRUCT_OPS) >>> + return bpffs_struct_ops_link_open(inode, filp); >>> + >>> + return -EOPNOTSUPP; >>> +} >>> + >>> +const struct file_operations bpf_link_fops = { >>> + .open = bpf_link_open, >>> #ifdef CONFIG_PROC_FS >>> .show_fdinfo = bpf_link_show_fdinfo, >>> #endif >>
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?
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?
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.
On Tue, Apr 23, 2024 at 5:29 PM Martin KaFai Lau <martin.lau@linux.dev> 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
On 4/24/24 17:17, Andrii Nakryiko wrote: > On Tue, Apr 23, 2024 at 5:29 PM Martin KaFai Lau <martin.lau@linux.dev> 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 Understand! Let's drop this patch.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 5034c1b4ded7..23b831ec9b71 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -2160,6 +2160,10 @@ extern const struct super_operations bpf_super_ops; extern const struct file_operations bpf_map_fops; extern const struct file_operations bpf_prog_fops; extern const struct file_operations bpf_iter_fops; +extern const struct file_operations bpf_link_fops; + +/* Required by bpf_link_open() */ +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp); #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \ extern const struct bpf_prog_ops _name ## _prog_ops; \ diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 86c7884abaf8..8be4f755a182 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -1198,3 +1198,13 @@ void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map info->btf_vmlinux_id = btf_obj_id(st_map->btf); } + +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) + fops = &bpf_link_fops; + else + fops = &bpffs_obj_fops; + return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops); } static struct dentry * diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 7d392ec83655..f66bc6215faa 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp) } #endif -static const struct file_operations bpf_link_fops = { +/* Support opening pinned links */ +static int bpf_link_open(struct inode *inode, struct file *filp) +{ + struct bpf_link *link = inode->i_private; + + if (link->type == BPF_LINK_TYPE_STRUCT_OPS) + return bpffs_struct_ops_link_open(inode, filp); + + return -EOPNOTSUPP; +} + +const struct file_operations bpf_link_fops = { + .open = bpf_link_open, #ifdef CONFIG_PROC_FS .show_fdinfo = bpf_link_show_fdinfo, #endif
Add the "open" operator for the inodes of BPF links to allow applications to obtain a file descriptor of a struct_ops link from a pinned path. Applications have the ability to update a struct_ops link with another struct_ops map. However, they were unable to open pinned paths of the links with this patch. This implies that updating a link through its pinned paths was not feasible. This patch adds the "open" operator to bpf_link_ops and uses bpf_link_ops as the i_fop for inodes of struct_ops links. "open" will be called to open the pinned path represented by an inode. Additionally, bpf_link_ops will be used as the f->f_ops of the opened "file" to provide operators for the "file". Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> --- include/linux/bpf.h | 4 ++++ kernel/bpf/bpf_struct_ops.c | 10 ++++++++++ kernel/bpf/inode.c | 11 ++++++++--- kernel/bpf/syscall.c | 14 +++++++++++++- 4 files changed, 35 insertions(+), 4 deletions(-)