| Message ID | 20230602085239.91138-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Support ->fill_link_info for kprobe prog | expand |
On 06/02, Yafang Shao wrote: > By adding support for ->fill_link_info to the kprobe_multi link, users will > be able to inspect it using `bpftool link show`. This enhancement will > expose both the count of probed functions and their respective addresses to > the user. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > include/uapi/linux/bpf.h | 4 ++++ > kernel/trace/bpf_trace.c | 26 ++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 4 ++++ > 3 files changed, 34 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index a7b5e91..22c8168 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -6438,6 +6438,10 @@ struct bpf_link_info { > __s32 priority; > __u32 flags; > } netfilter; > + struct { > + __u64 addrs; > + __u32 count; > + } kprobe_multi; > }; > } __attribute__((aligned(8))); > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 2bc41e6..ec53bc9 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2548,9 +2548,35 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link) > kfree(kmulti_link); > } > > +static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link, > + struct bpf_link_info *info) > +{ > + u64 *uaddrs = (u64 *)u64_to_user_ptr(info->kprobe_multi.addrs); Maybe tag this as __user as well? u64 __user *uaddrs = u64_to_user_ptr(info->kprobe_multi.addrs); copy_to_user expects __user tagged memory, so most likely sparse tool will complain on your current approach. > + struct bpf_kprobe_multi_link *kmulti_link; > + u32 ucount = info->kprobe_multi.count; > + > + if (!uaddrs ^ !ucount) > + return -EINVAL; > + > + kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link); > + if (!uaddrs) { > + info->kprobe_multi.count = kmulti_link->cnt; > + return 0; > + } > + > + if (!ucount) > + return 0; > + if (ucount != kmulti_link->cnt) > + return -EINVAL; [..] > + if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64))) > + return -EFAULT; I'm not super familiar with this part, so maybe stupid question: do we need to hold any locks during the copy? IOW, can kmulti_link->addrs be updated concurrently?
Hi Yafang,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Yafang-Shao/bpf-Support-fill_link_info-for-kprobe_multi/20230602-165455
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230602085239.91138-2-laoar.shao%40gmail.com
patch subject: [PATCH bpf-next 1/6] bpf: Support ->fill_link_info for kprobe_multi
config: i386-randconfig-s002-20230601 (https://download.01.org/0day-ci/archive/20230603/202306031438.0X2HPFQl-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/270f3eb6c142f1f0ec7d800b8ecaab1b101682a0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yafang-Shao/bpf-Support-fill_link_info-for-kprobe_multi/20230602-165455
git checkout 270f3eb6c142f1f0ec7d800b8ecaab1b101682a0
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash kernel/trace/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306031438.0X2HPFQl-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> kernel/trace/bpf_trace.c:2554:24: sparse: sparse: cast removes address space '__user' of expression
>> kernel/trace/bpf_trace.c:2571:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void [noderef] __user *to @@ got unsigned long long [usertype] *uaddrs @@
kernel/trace/bpf_trace.c:2571:26: sparse: expected void [noderef] __user *to
kernel/trace/bpf_trace.c:2571:26: sparse: got unsigned long long [usertype] *uaddrs
kernel/trace/bpf_trace.c:2492:21: sparse: sparse: dereference of noderef expression
kernel/trace/bpf_trace.c:2496:66: sparse: sparse: dereference of noderef expression
vim +/__user +2554 kernel/trace/bpf_trace.c
2550
2551 static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
2552 struct bpf_link_info *info)
2553 {
> 2554 u64 *uaddrs = (u64 *)u64_to_user_ptr(info->kprobe_multi.addrs);
2555 struct bpf_kprobe_multi_link *kmulti_link;
2556 u32 ucount = info->kprobe_multi.count;
2557
2558 if (!uaddrs ^ !ucount)
2559 return -EINVAL;
2560
2561 kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
2562 if (!uaddrs) {
2563 info->kprobe_multi.count = kmulti_link->cnt;
2564 return 0;
2565 }
2566
2567 if (!ucount)
2568 return 0;
2569 if (ucount != kmulti_link->cnt)
2570 return -EINVAL;
> 2571 if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64)))
2572 return -EFAULT;
2573 return 0;
2574 }
2575
On Sat, Jun 3, 2023 at 12:46 AM Stanislav Fomichev <sdf@google.com> wrote: > > On 06/02, Yafang Shao wrote: > > By adding support for ->fill_link_info to the kprobe_multi link, users will > > be able to inspect it using `bpftool link show`. This enhancement will > > expose both the count of probed functions and their respective addresses to > > the user. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > include/uapi/linux/bpf.h | 4 ++++ > > kernel/trace/bpf_trace.c | 26 ++++++++++++++++++++++++++ > > tools/include/uapi/linux/bpf.h | 4 ++++ > > 3 files changed, 34 insertions(+) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index a7b5e91..22c8168 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -6438,6 +6438,10 @@ struct bpf_link_info { > > __s32 priority; > > __u32 flags; > > } netfilter; > > + struct { > > + __u64 addrs; > > + __u32 count; > > + } kprobe_multi; > > }; > > } __attribute__((aligned(8))); > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 2bc41e6..ec53bc9 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -2548,9 +2548,35 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link) > > kfree(kmulti_link); > > } > > > > +static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link, > > + struct bpf_link_info *info) > > +{ > > + u64 *uaddrs = (u64 *)u64_to_user_ptr(info->kprobe_multi.addrs); > > Maybe tag this as __user as well? > > u64 __user *uaddrs = u64_to_user_ptr(info->kprobe_multi.addrs); > > copy_to_user expects __user tagged memory, so most likely sparse tool > will complain on your current approach. Thanks for pointing it out. Will correct it. > > > + struct bpf_kprobe_multi_link *kmulti_link; > > + u32 ucount = info->kprobe_multi.count; > > + > > + if (!uaddrs ^ !ucount) > > + return -EINVAL; > > + > > + kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link); > > + if (!uaddrs) { > > + info->kprobe_multi.count = kmulti_link->cnt; > > + return 0; > > + } > > + > > + if (!ucount) > > + return 0; > > + if (ucount != kmulti_link->cnt) > > + return -EINVAL; > > [..] > > > + if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64))) > > + return -EFAULT; > > I'm not super familiar with this part, so maybe stupid question: > do we need to hold any locks during the copy? IOW, can kmulti_link->addrs > be updated concurrently? No, we can't update kmulti_link->addrs concurrently. Once a fprobe is registered, we can't update it unless we unregister it. When we reach the ->fill_link_info, it can't be released, so it is safe. -- Regards Yafang
On Sat, Jun 3, 2023 at 2:38 PM kernel test robot <lkp@intel.com> wrote: > > Hi Yafang, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on bpf-next/master] > > url: https://github.com/intel-lab-lkp/linux/commits/Yafang-Shao/bpf-Support-fill_link_info-for-kprobe_multi/20230602-165455 > base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master > patch link: https://lore.kernel.org/r/20230602085239.91138-2-laoar.shao%40gmail.com > patch subject: [PATCH bpf-next 1/6] bpf: Support ->fill_link_info for kprobe_multi > config: i386-randconfig-s002-20230601 (https://download.01.org/0day-ci/archive/20230603/202306031438.0X2HPFQl-lkp@intel.com/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > reproduce: > # apt-get install sparse > # sparse version: v0.6.4-39-gce1a6720-dirty > # https://github.com/intel-lab-lkp/linux/commit/270f3eb6c142f1f0ec7d800b8ecaab1b101682a0 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Yafang-Shao/bpf-Support-fill_link_info-for-kprobe_multi/20230602-165455 > git checkout 270f3eb6c142f1f0ec7d800b8ecaab1b101682a0 > # save the config file > mkdir build_dir && cp config build_dir/.config > make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig > make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash kernel/trace/ > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202306031438.0X2HPFQl-lkp@intel.com/ > > sparse warnings: (new ones prefixed by >>) > >> kernel/trace/bpf_trace.c:2554:24: sparse: sparse: cast removes address space '__user' of expression > >> kernel/trace/bpf_trace.c:2571:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void [noderef] __user *to @@ got unsigned long long [usertype] *uaddrs @@ > kernel/trace/bpf_trace.c:2571:26: sparse: expected void [noderef] __user *to > kernel/trace/bpf_trace.c:2571:26: sparse: got unsigned long long [usertype] *uaddrs > kernel/trace/bpf_trace.c:2492:21: sparse: sparse: dereference of noderef expression > kernel/trace/bpf_trace.c:2496:66: sparse: sparse: dereference of noderef expression > > vim +/__user +2554 kernel/trace/bpf_trace.c > > 2550 > 2551 static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link, > 2552 struct bpf_link_info *info) > 2553 { > > 2554 u64 *uaddrs = (u64 *)u64_to_user_ptr(info->kprobe_multi.addrs); Thanks for the report. Will fix it. > 2555 struct bpf_kprobe_multi_link *kmulti_link; > 2556 u32 ucount = info->kprobe_multi.count; > 2557 > 2558 if (!uaddrs ^ !ucount) > 2559 return -EINVAL; > 2560 > 2561 kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link); > 2562 if (!uaddrs) { > 2563 info->kprobe_multi.count = kmulti_link->cnt; > 2564 return 0; > 2565 } > 2566 > 2567 if (!ucount) > 2568 return 0; > 2569 if (ucount != kmulti_link->cnt) > 2570 return -EINVAL; > > 2571 if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64))) > 2572 return -EFAULT; > 2573 return 0; > 2574 } > 2575 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index a7b5e91..22c8168 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -6438,6 +6438,10 @@ struct bpf_link_info { __s32 priority; __u32 flags; } netfilter; + struct { + __u64 addrs; + __u32 count; + } kprobe_multi; }; } __attribute__((aligned(8))); diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 2bc41e6..ec53bc9 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2548,9 +2548,35 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link) kfree(kmulti_link); } +static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link, + struct bpf_link_info *info) +{ + u64 *uaddrs = (u64 *)u64_to_user_ptr(info->kprobe_multi.addrs); + struct bpf_kprobe_multi_link *kmulti_link; + u32 ucount = info->kprobe_multi.count; + + if (!uaddrs ^ !ucount) + return -EINVAL; + + kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link); + if (!uaddrs) { + info->kprobe_multi.count = kmulti_link->cnt; + return 0; + } + + if (!ucount) + return 0; + if (ucount != kmulti_link->cnt) + return -EINVAL; + if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64))) + return -EFAULT; + return 0; +} + static const struct bpf_link_ops bpf_kprobe_multi_link_lops = { .release = bpf_kprobe_multi_link_release, .dealloc = bpf_kprobe_multi_link_dealloc, + .fill_link_info = bpf_kprobe_multi_link_fill_link_info, }; static void bpf_kprobe_multi_cookie_swap(void *a, void *b, int size, const void *priv) diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index a7b5e91..22c8168 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -6438,6 +6438,10 @@ struct bpf_link_info { __s32 priority; __u32 flags; } netfilter; + struct { + __u64 addrs; + __u32 count; + } kprobe_multi; }; } __attribute__((aligned(8)));
By adding support for ->fill_link_info to the kprobe_multi link, users will be able to inspect it using `bpftool link show`. This enhancement will expose both the count of probed functions and their respective addresses to the user. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/uapi/linux/bpf.h | 4 ++++ kernel/trace/bpf_trace.c | 26 ++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 4 ++++ 3 files changed, 34 insertions(+)