| Message ID | 20220211121145.35237-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add more information into bpffs | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf | success | VM_Test |
| bpf/vmtest-bpf-PR | success | PR summary |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On 2/11/22 1:11 PM, Yafang Shao wrote: > A new member pin_name is added into struct bpf_prog_aux, which will be > set when the prog is set and cleared when the pinned file is removed. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > include/linux/bpf.h | 2 ++ > include/uapi/linux/bpf.h | 1 + > kernel/bpf/inode.c | 20 +++++++++++++++++++- > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 0ceb25b..9cf8055 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -933,6 +933,8 @@ struct bpf_prog_aux { > struct work_struct work; > struct rcu_head rcu; > }; > + > + char pin_name[BPF_PIN_NAME_LEN]; > }; I'm afraid this is not possible. You are assuming a 1:1 relationship between prog and pin location, but it's really a 1:n (prog can be pinned in multiple locations and also across multiple mount instances). Also, you can create hard links of pins which are not handled via bpf_obj_do_pin(). > struct bpf_array_aux { > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index c14fed8..bada5cc 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1217,6 +1217,7 @@ struct bpf_stack_build_id { > }; > > #define BPF_OBJ_NAME_LEN 16U > +#define BPF_PIN_NAME_LEN 64U > > union bpf_attr { > struct { /* anonymous struct used by BPF_MAP_CREATE command */ > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c > index 4477ca8..f1a8811 100644 > --- a/kernel/bpf/inode.c > +++ b/kernel/bpf/inode.c > @@ -437,6 +437,8 @@ static int bpf_iter_link_pin_kernel(struct dentry *parent, > static int bpf_obj_do_pin(const char __user *pathname, void *raw, > enum bpf_type type) > { > + struct bpf_prog_aux *aux; > + struct bpf_prog *prog; > struct dentry *dentry; > struct inode *dir; > struct path path; > @@ -461,6 +463,10 @@ static int bpf_obj_do_pin(const char __user *pathname, void *raw, > > switch (type) { > case BPF_TYPE_PROG: > + prog = raw; > + aux = prog->aux; > + (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN); > + aux->pin_name[BPF_PIN_NAME_LEN - 1] = '\0'; > ret = vfs_mkobj(dentry, mode, bpf_mkprog, raw); > break; > case BPF_TYPE_MAP: > @@ -611,12 +617,24 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root) > > static void bpf_free_inode(struct inode *inode) > { > + struct bpf_prog_aux *aux; > + struct bpf_prog *prog; > enum bpf_type type; > > if (S_ISLNK(inode->i_mode)) > kfree(inode->i_link); > - if (!bpf_inode_type(inode, &type)) > + if (!bpf_inode_type(inode, &type)) { > + switch (type) { > + case BPF_TYPE_PROG: > + prog = inode->i_private; > + aux = prog->aux; > + aux->pin_name[0] = '\0'; > + break; > + default: > + break; > + } > bpf_any_put(inode->i_private, type); > + } > free_inode_nonrcu(inode); > } > >
On 2/11/22 1:43 PM, Daniel Borkmann wrote: > On 2/11/22 1:11 PM, Yafang Shao wrote: >> A new member pin_name is added into struct bpf_prog_aux, which will be >> set when the prog is set and cleared when the pinned file is removed. >> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> >> --- >> include/linux/bpf.h | 2 ++ >> include/uapi/linux/bpf.h | 1 + >> kernel/bpf/inode.c | 20 +++++++++++++++++++- >> 3 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index 0ceb25b..9cf8055 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -933,6 +933,8 @@ struct bpf_prog_aux { >> struct work_struct work; >> struct rcu_head rcu; >> }; >> + >> + char pin_name[BPF_PIN_NAME_LEN]; >> }; > > I'm afraid this is not possible. You are assuming a 1:1 relationship between prog > and pin location, but it's really a 1:n (prog can be pinned in multiple locations > and also across multiple mount instances). Also, you can create hard links of pins > which are not handled via bpf_obj_do_pin(). (Same is also true for BPF maps wrt patch 2.)
Hi Yafang,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf/master]
[also build test WARNING on net/master horms-ipvs/master net-next/master v5.17-rc3 next-20220211]
[cannot apply to bpf-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Yafang-Shao/bpf-Add-more-information-into-bpffs/20220211-201319
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220211/202202112213.WGiJCCYD-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6cd35bc70f99caee380d84f5ba9256ac5fe03860
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yafang-Shao/bpf-Add-more-information-into-bpffs/20220211-201319
git checkout 6cd35bc70f99caee380d84f5ba9256ac5fe03860
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash kernel/bpf/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
kernel/bpf/inode.c: In function 'bpf_obj_do_pin':
>> kernel/bpf/inode.c:469:24: warning: ignoring return value of 'strncpy_from_user' declared with attribute 'warn_unused_result' [-Wunused-result]
469 | (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +469 kernel/bpf/inode.c
437
438 static int bpf_obj_do_pin(const char __user *pathname, void *raw,
439 enum bpf_type type)
440 {
441 struct bpf_prog_aux *aux;
442 struct bpf_prog *prog;
443 struct dentry *dentry;
444 struct inode *dir;
445 struct path path;
446 umode_t mode;
447 int ret;
448
449 dentry = user_path_create(AT_FDCWD, pathname, &path, 0);
450 if (IS_ERR(dentry))
451 return PTR_ERR(dentry);
452
453 mode = S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask());
454
455 ret = security_path_mknod(&path, dentry, mode, 0);
456 if (ret)
457 goto out;
458
459 dir = d_inode(path.dentry);
460 if (dir->i_op != &bpf_dir_iops) {
461 ret = -EPERM;
462 goto out;
463 }
464
465 switch (type) {
466 case BPF_TYPE_PROG:
467 prog = raw;
468 aux = prog->aux;
> 469 (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN);
470 aux->pin_name[BPF_PIN_NAME_LEN - 1] = '\0';
471 ret = vfs_mkobj(dentry, mode, bpf_mkprog, raw);
472 break;
473 case BPF_TYPE_MAP:
474 ret = vfs_mkobj(dentry, mode, bpf_mkmap, raw);
475 break;
476 case BPF_TYPE_LINK:
477 ret = vfs_mkobj(dentry, mode, bpf_mklink, raw);
478 break;
479 default:
480 ret = -EPERM;
481 }
482 out:
483 done_path_create(&path, dentry);
484 return ret;
485 }
486
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, Feb 11, 2022 at 8:43 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 2/11/22 1:11 PM, Yafang Shao wrote: > > A new member pin_name is added into struct bpf_prog_aux, which will be > > set when the prog is set and cleared when the pinned file is removed. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > include/linux/bpf.h | 2 ++ > > include/uapi/linux/bpf.h | 1 + > > kernel/bpf/inode.c | 20 +++++++++++++++++++- > > 3 files changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 0ceb25b..9cf8055 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -933,6 +933,8 @@ struct bpf_prog_aux { > > struct work_struct work; > > struct rcu_head rcu; > > }; > > + > > + char pin_name[BPF_PIN_NAME_LEN]; > > }; > > I'm afraid this is not possible. You are assuming a 1:1 relationship between prog > and pin location, but it's really a 1:n (prog can be pinned in multiple locations > and also across multiple mount instances). Thanks for the explanation! I didn't notice the 1:n relationship before. I will read the code more to try to find if there's a good way to implement it. > Also, you can create hard links of pins > which are not handled via bpf_obj_do_pin(). > Yes, it seems we should introduce bpf_{link, unlink, rename} first instead of the simple_* one. > > struct bpf_array_aux { > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index c14fed8..bada5cc 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -1217,6 +1217,7 @@ struct bpf_stack_build_id { > > }; > > > > #define BPF_OBJ_NAME_LEN 16U > > +#define BPF_PIN_NAME_LEN 64U > > > > union bpf_attr { > > struct { /* anonymous struct used by BPF_MAP_CREATE command */ > > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c > > index 4477ca8..f1a8811 100644 > > --- a/kernel/bpf/inode.c > > +++ b/kernel/bpf/inode.c > > @@ -437,6 +437,8 @@ static int bpf_iter_link_pin_kernel(struct dentry *parent, > > static int bpf_obj_do_pin(const char __user *pathname, void *raw, > > enum bpf_type type) > > { > > + struct bpf_prog_aux *aux; > > + struct bpf_prog *prog; > > struct dentry *dentry; > > struct inode *dir; > > struct path path; > > @@ -461,6 +463,10 @@ static int bpf_obj_do_pin(const char __user *pathname, void *raw, > > > > switch (type) { > > case BPF_TYPE_PROG: > > + prog = raw; > > + aux = prog->aux; > > + (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN); > > + aux->pin_name[BPF_PIN_NAME_LEN - 1] = '\0'; > > ret = vfs_mkobj(dentry, mode, bpf_mkprog, raw); > > break; > > case BPF_TYPE_MAP: > > @@ -611,12 +617,24 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root) > > > > static void bpf_free_inode(struct inode *inode) > > { > > + struct bpf_prog_aux *aux; > > + struct bpf_prog *prog; > > enum bpf_type type; > > > > if (S_ISLNK(inode->i_mode)) > > kfree(inode->i_link); > > - if (!bpf_inode_type(inode, &type)) > > + if (!bpf_inode_type(inode, &type)) { > > + switch (type) { > > + case BPF_TYPE_PROG: > > + prog = inode->i_private; > > + aux = prog->aux; > > + aux->pin_name[0] = '\0'; > > + break; > > + default: > > + break; > > + } > > bpf_any_put(inode->i_private, type); > > + } > > free_inode_nonrcu(inode); > > } > > > > >
On Fri, Feb 11, 2022 at 8:44 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 2/11/22 1:43 PM, Daniel Borkmann wrote: > > On 2/11/22 1:11 PM, Yafang Shao wrote: > >> A new member pin_name is added into struct bpf_prog_aux, which will be > >> set when the prog is set and cleared when the pinned file is removed. > >> > >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > >> --- > >> include/linux/bpf.h | 2 ++ > >> include/uapi/linux/bpf.h | 1 + > >> kernel/bpf/inode.c | 20 +++++++++++++++++++- > >> 3 files changed, 22 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h > >> index 0ceb25b..9cf8055 100644 > >> --- a/include/linux/bpf.h > >> +++ b/include/linux/bpf.h > >> @@ -933,6 +933,8 @@ struct bpf_prog_aux { > >> struct work_struct work; > >> struct rcu_head rcu; > >> }; > >> + > >> + char pin_name[BPF_PIN_NAME_LEN]; > >> }; > > > > I'm afraid this is not possible. You are assuming a 1:1 relationship between prog > > and pin location, but it's really a 1:n (prog can be pinned in multiple locations > > and also across multiple mount instances). Also, you can create hard links of pins > > which are not handled via bpf_obj_do_pin(). > > (Same is also true for BPF maps wrt patch 2.) Thanks again for the reminder.
On Sat, Feb 12, 2022 at 1:59 AM kernel test robot <lkp@intel.com> wrote: > > Hi Yafang, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on bpf/master] > [also build test WARNING on net/master horms-ipvs/master net-next/master v5.17-rc3 next-20220211] > [cannot apply to bpf-next/master] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Yafang-Shao/bpf-Add-more-information-into-bpffs/20220211-201319 > base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master > config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220211/202202112213.WGiJCCYD-lkp@intel.com/config) > compiler: arceb-elf-gcc (GCC) 11.2.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/0day-ci/linux/commit/6cd35bc70f99caee380d84f5ba9256ac5fe03860 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Yafang-Shao/bpf-Add-more-information-into-bpffs/20220211-201319 > git checkout 6cd35bc70f99caee380d84f5ba9256ac5fe03860 > # save the config file to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash kernel/bpf/ > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > kernel/bpf/inode.c: In function 'bpf_obj_do_pin': > >> kernel/bpf/inode.c:469:24: warning: ignoring return value of 'strncpy_from_user' declared with attribute 'warn_unused_result' [-Wunused-result] > 469 | (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > Thanks for the report. I will improve it to avoid this warning. > > vim +469 kernel/bpf/inode.c > > 437 > 438 static int bpf_obj_do_pin(const char __user *pathname, void *raw, > 439 enum bpf_type type) > 440 { > 441 struct bpf_prog_aux *aux; > 442 struct bpf_prog *prog; > 443 struct dentry *dentry; > 444 struct inode *dir; > 445 struct path path; > 446 umode_t mode; > 447 int ret; > 448 > 449 dentry = user_path_create(AT_FDCWD, pathname, &path, 0); > 450 if (IS_ERR(dentry)) > 451 return PTR_ERR(dentry); > 452 > 453 mode = S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()); > 454 > 455 ret = security_path_mknod(&path, dentry, mode, 0); > 456 if (ret) > 457 goto out; > 458 > 459 dir = d_inode(path.dentry); > 460 if (dir->i_op != &bpf_dir_iops) { > 461 ret = -EPERM; > 462 goto out; > 463 } > 464 > 465 switch (type) { > 466 case BPF_TYPE_PROG: > 467 prog = raw; > 468 aux = prog->aux; > > 469 (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN); > 470 aux->pin_name[BPF_PIN_NAME_LEN - 1] = '\0'; > 471 ret = vfs_mkobj(dentry, mode, bpf_mkprog, raw); > 472 break; > 473 case BPF_TYPE_MAP: > 474 ret = vfs_mkobj(dentry, mode, bpf_mkmap, raw); > 475 break; > 476 case BPF_TYPE_LINK: > 477 ret = vfs_mkobj(dentry, mode, bpf_mklink, raw); > 478 break; > 479 default: > 480 ret = -EPERM; > 481 } > 482 out: > 483 done_path_create(&path, dentry); > 484 return ret; > 485 } > 486 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 0ceb25b..9cf8055 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -933,6 +933,8 @@ struct bpf_prog_aux { struct work_struct work; struct rcu_head rcu; }; + + char pin_name[BPF_PIN_NAME_LEN]; }; struct bpf_array_aux { diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index c14fed8..bada5cc 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1217,6 +1217,7 @@ struct bpf_stack_build_id { }; #define BPF_OBJ_NAME_LEN 16U +#define BPF_PIN_NAME_LEN 64U union bpf_attr { struct { /* anonymous struct used by BPF_MAP_CREATE command */ diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c index 4477ca8..f1a8811 100644 --- a/kernel/bpf/inode.c +++ b/kernel/bpf/inode.c @@ -437,6 +437,8 @@ static int bpf_iter_link_pin_kernel(struct dentry *parent, static int bpf_obj_do_pin(const char __user *pathname, void *raw, enum bpf_type type) { + struct bpf_prog_aux *aux; + struct bpf_prog *prog; struct dentry *dentry; struct inode *dir; struct path path; @@ -461,6 +463,10 @@ static int bpf_obj_do_pin(const char __user *pathname, void *raw, switch (type) { case BPF_TYPE_PROG: + prog = raw; + aux = prog->aux; + (void) strncpy_from_user(aux->pin_name, pathname, BPF_PIN_NAME_LEN); + aux->pin_name[BPF_PIN_NAME_LEN - 1] = '\0'; ret = vfs_mkobj(dentry, mode, bpf_mkprog, raw); break; case BPF_TYPE_MAP: @@ -611,12 +617,24 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root) static void bpf_free_inode(struct inode *inode) { + struct bpf_prog_aux *aux; + struct bpf_prog *prog; enum bpf_type type; if (S_ISLNK(inode->i_mode)) kfree(inode->i_link); - if (!bpf_inode_type(inode, &type)) + if (!bpf_inode_type(inode, &type)) { + switch (type) { + case BPF_TYPE_PROG: + prog = inode->i_private; + aux = prog->aux; + aux->pin_name[0] = '\0'; + break; + default: + break; + } bpf_any_put(inode->i_private, type); + } free_inode_nonrcu(inode); }
A new member pin_name is added into struct bpf_prog_aux, which will be set when the prog is set and cleared when the pinned file is removed. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/linux/bpf.h | 2 ++ include/uapi/linux/bpf.h | 1 + kernel/bpf/inode.c | 20 +++++++++++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-)