| Message ID | 20241031210938.1696639-3-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [trace/for-next,1/3] bpf: put bpf_link's program when link is safe to be deallocated | expand |
Hi Andrii,
kernel test robot noticed the following build errors:
[auto build test ERROR on trace/for-next]
url: https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-decouple-BPF-link-attach-hook-and-BPF-program-sleepable-semantics/20241101-051131
base: https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
patch link: https://lore.kernel.org/r/20241031210938.1696639-3-andrii%40kernel.org
patch subject: [PATCH trace/for-next 3/3] bpf: ensure RCU Tasks Trace GP for sleepable raw tracepoint BPF links
config: i386-buildonly-randconfig-001-20241101 (https://download.01.org/0day-ci/archive/20241101/202411011258.IemsLYSp-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411011258.IemsLYSp-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/202411011258.IemsLYSp-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from kernel/bpf/syscall.c:4:
In file included from include/linux/bpf.h:21:
In file included from include/linux/kallsyms.h:13:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> kernel/bpf/syscall.c:3866:5: error: call to undeclared function 'tracepoint_is_faultable'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
3866 | tracepoint_is_faultable(btp->tp));
| ^
kernel/bpf/syscall.c:5876:30: warning: bitwise operation between different enumeration types ('enum bpf_arg_type' and 'enum bpf_type_flag') [-Wenum-enum-conversion]
5876 | .arg2_type = ARG_PTR_TO_MEM | MEM_RDONLY,
| ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
kernel/bpf/syscall.c:5926:41: warning: bitwise operation between different enumeration types ('enum bpf_arg_type' and 'enum bpf_type_flag') [-Wenum-enum-conversion]
5926 | .arg4_type = ARG_PTR_TO_FIXED_SIZE_MEM | MEM_UNINIT | MEM_ALIGNED,
| ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
3 warnings and 1 error generated.
vim +/tracepoint_is_faultable +3866 kernel/bpf/syscall.c
3818
3819 static int bpf_raw_tp_link_attach(struct bpf_prog *prog,
3820 const char __user *user_tp_name, u64 cookie)
3821 {
3822 struct bpf_link_primer link_primer;
3823 struct bpf_raw_tp_link *link;
3824 struct bpf_raw_event_map *btp;
3825 const char *tp_name;
3826 char buf[128];
3827 int err;
3828
3829 switch (prog->type) {
3830 case BPF_PROG_TYPE_TRACING:
3831 case BPF_PROG_TYPE_EXT:
3832 case BPF_PROG_TYPE_LSM:
3833 if (user_tp_name)
3834 /* The attach point for this category of programs
3835 * should be specified via btf_id during program load.
3836 */
3837 return -EINVAL;
3838 if (prog->type == BPF_PROG_TYPE_TRACING &&
3839 prog->expected_attach_type == BPF_TRACE_RAW_TP) {
3840 tp_name = prog->aux->attach_func_name;
3841 break;
3842 }
3843 return bpf_tracing_prog_attach(prog, 0, 0, 0);
3844 case BPF_PROG_TYPE_RAW_TRACEPOINT:
3845 case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
3846 if (strncpy_from_user(buf, user_tp_name, sizeof(buf) - 1) < 0)
3847 return -EFAULT;
3848 buf[sizeof(buf) - 1] = 0;
3849 tp_name = buf;
3850 break;
3851 default:
3852 return -EINVAL;
3853 }
3854
3855 btp = bpf_get_raw_tracepoint(tp_name);
3856 if (!btp)
3857 return -ENOENT;
3858
3859 link = kzalloc(sizeof(*link), GFP_USER);
3860 if (!link) {
3861 err = -ENOMEM;
3862 goto out_put_btp;
3863 }
3864 bpf_link_init_sleepable(&link->link, BPF_LINK_TYPE_RAW_TRACEPOINT,
3865 &bpf_raw_tp_link_lops, prog,
> 3866 tracepoint_is_faultable(btp->tp));
3867 link->btp = btp;
3868 link->cookie = cookie;
3869
3870 err = bpf_link_prime(&link->link, &link_primer);
3871 if (err) {
3872 kfree(link);
3873 goto out_put_btp;
3874 }
3875
3876 err = bpf_probe_register(link->btp, link);
3877 if (err) {
3878 bpf_link_cleanup(&link_primer);
3879 goto out_put_btp;
3880 }
3881
3882 return bpf_link_settle(&link_primer);
3883
3884 out_put_btp:
3885 bpf_put_raw_tracepoint(btp);
3886 return err;
3887 }
3888
Hi Andrii, kernel test robot noticed the following build errors: [auto build test ERROR on trace/for-next] url: https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-decouple-BPF-link-attach-hook-and-BPF-program-sleepable-semantics/20241101-051131 base: https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next patch link: https://lore.kernel.org/r/20241031210938.1696639-3-andrii%40kernel.org patch subject: [PATCH trace/for-next 3/3] bpf: ensure RCU Tasks Trace GP for sleepable raw tracepoint BPF links config: x86_64-buildonly-randconfig-001-20241101 (https://download.01.org/0day-ci/archive/20241101/202411011255.GYntOfN5-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411011255.GYntOfN5-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/202411011255.GYntOfN5-lkp@intel.com/ All errors (new ones prefixed by >>): kernel/bpf/syscall.c: In function 'bpf_raw_tp_link_attach': >> kernel/bpf/syscall.c:3866:33: error: implicit declaration of function 'tracepoint_is_faultable' [-Werror=implicit-function-declaration] 3866 | tracepoint_is_faultable(btp->tp)); | ^~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +/tracepoint_is_faultable +3866 kernel/bpf/syscall.c 3818 3819 static int bpf_raw_tp_link_attach(struct bpf_prog *prog, 3820 const char __user *user_tp_name, u64 cookie) 3821 { 3822 struct bpf_link_primer link_primer; 3823 struct bpf_raw_tp_link *link; 3824 struct bpf_raw_event_map *btp; 3825 const char *tp_name; 3826 char buf[128]; 3827 int err; 3828 3829 switch (prog->type) { 3830 case BPF_PROG_TYPE_TRACING: 3831 case BPF_PROG_TYPE_EXT: 3832 case BPF_PROG_TYPE_LSM: 3833 if (user_tp_name) 3834 /* The attach point for this category of programs 3835 * should be specified via btf_id during program load. 3836 */ 3837 return -EINVAL; 3838 if (prog->type == BPF_PROG_TYPE_TRACING && 3839 prog->expected_attach_type == BPF_TRACE_RAW_TP) { 3840 tp_name = prog->aux->attach_func_name; 3841 break; 3842 } 3843 return bpf_tracing_prog_attach(prog, 0, 0, 0); 3844 case BPF_PROG_TYPE_RAW_TRACEPOINT: 3845 case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE: 3846 if (strncpy_from_user(buf, user_tp_name, sizeof(buf) - 1) < 0) 3847 return -EFAULT; 3848 buf[sizeof(buf) - 1] = 0; 3849 tp_name = buf; 3850 break; 3851 default: 3852 return -EINVAL; 3853 } 3854 3855 btp = bpf_get_raw_tracepoint(tp_name); 3856 if (!btp) 3857 return -ENOENT; 3858 3859 link = kzalloc(sizeof(*link), GFP_USER); 3860 if (!link) { 3861 err = -ENOMEM; 3862 goto out_put_btp; 3863 } 3864 bpf_link_init_sleepable(&link->link, BPF_LINK_TYPE_RAW_TRACEPOINT, 3865 &bpf_raw_tp_link_lops, prog, > 3866 tracepoint_is_faultable(btp->tp)); 3867 link->btp = btp; 3868 link->cookie = cookie; 3869 3870 err = bpf_link_prime(&link->link, &link_primer); 3871 if (err) { 3872 kfree(link); 3873 goto out_put_btp; 3874 } 3875 3876 err = bpf_probe_register(link->btp, link); 3877 if (err) { 3878 bpf_link_cleanup(&link_primer); 3879 goto out_put_btp; 3880 } 3881 3882 return bpf_link_settle(&link_primer); 3883 3884 out_put_btp: 3885 bpf_put_raw_tracepoint(btp); 3886 return err; 3887 } 3888
Just to confirm, I ran the reproducer from [1] after combining this
series with Mathieu's from [2] and it ran for 20m with no issues.
[1]: https://lore.kernel.org/bpf/67121037.050a0220.10f4f4.000f.GAE@google.com/
[2]: https://lore.kernel.org/bpf/20241031152056.744137-1-mathieu.desnoyers@efficios.com/T/#u
Tested-by: Jordan Rife <jrife@google.com>
On Fri, Nov 1, 2024 at 8:03 AM Jordan Rife <jrife@google.com> wrote: > > Just to confirm, I ran the reproducer from [1] after combining this > series with Mathieu's from [2] and it ran for 20m with no issues. > > [1]: https://lore.kernel.org/bpf/67121037.050a0220.10f4f4.000f.GAE@google.com/ > [2]: https://lore.kernel.org/bpf/20241031152056.744137-1-mathieu.desnoyers@efficios.com/T/#u > > Tested-by: Jordan Rife <jrife@google.com> Great, thank you, Jordan! I was going to ask you specifically to double-check, as I couldn't repro the original issue locally with my setup (and I was too lazy to mess with custom images and stuff). I need to send a fixed up v2, I'll add your tested-by.
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 0f5540627911..db2a987504b2 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -35,6 +35,7 @@ #include <linux/rcupdate_trace.h> #include <linux/memcontrol.h> #include <linux/trace_events.h> +#include <linux/tracepoint.h> #include <net/netfilter/nf_bpf_link.h> #include <net/netkit.h> @@ -3845,8 +3846,9 @@ static int bpf_raw_tp_link_attach(struct bpf_prog *prog, err = -ENOMEM; goto out_put_btp; } - bpf_link_init(&link->link, BPF_LINK_TYPE_RAW_TRACEPOINT, - &bpf_raw_tp_link_lops, prog); + bpf_link_init_sleepable(&link->link, BPF_LINK_TYPE_RAW_TRACEPOINT, + &bpf_raw_tp_link_lops, prog, + tracepoint_is_faultable(btp->tp)); link->btp = btp; link->cookie = cookie;
Now that kernel supports sleepable tracepoints, the fact that bpf_probe_unregister() is asynchronous, i.e., that it doesn't wait for any in-flight tracepoints to conclude before returning, we now need to delay BPF raw tp link's deallocation and bpf_prog_put() of its underlying BPF program (regardless of program's own sleepable semantics) until after full RCU Tasks Trace GP. With that GP over, we'll have a guarantee that no tracepoint can reach BPF link and thus its BPF program. We use newly added tracepoint_is_faultable() check to know when this RCU Tasks Trace GP is necessary and utilize BPF link's own sleepable flag passed through bpf_link_init_sleepable() initializer. Reported-by: Jordan Rife <jrife@google.com> Fixes: a363d27cdbc2 ("tracing: Allow system call tracepoints to handle page faults") Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- kernel/bpf/syscall.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)