diff mbox series

trace/trace_uprobe: Only invoke uprobe ebpf handler when event matches.

Message ID 41828b90.5798.18442c10017.Coremail.00107082@163.com
State New
Headers
Series trace/trace_uprobe: Only invoke uprobe ebpf handler when event matches. |

Commit Message

David Wang Nov. 4, 2022, 1:07 p.m. UTC 
  Only invoke uprobe ebpf handler when event matches.
    
uprobe ebpf handler would be called even the event dose not
match any registered perf event, following steps would be
used to generate a unregistered perf event.
1. register a uprobe event on a specified pid <pid-a>
2. <pid-a> invokes syscall `clone` (via pthread_create),
    new process <pid-b> generated. (Maybe it is a bug here, the
uprobe breakpoint is inherited from <pid-a>
3. <pid-b> invokes the function which is uprobed in step 1.
4. perf event generated...
    
Ebpf handler would be invoked even the event happened on <pid-b>,
but the default perf event handler make further check and ignore
the event because no registered perf event match on <pid-b>.

The patch means to fix the inconsistent behavior between ebpf and the default.
Before invoke uprobe ebpf handler, make sure current event match.
    
Signed-off-by: David Wang <00107082@163.com>
--
--

Comments

kernel test robot Nov. 4, 2022, 9:30 p.m. UTC | #1 
Hi David,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.1-rc3 next-20221104]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Wang/trace-trace_uprobe-Only-invoke-uprobe-ebpf-handler-when-event-matches/20221104-210925
patch link:    https://lore.kernel.org/r/41828b90.5798.18442c10017.Coremail.00107082%40163.com
patch subject: [PATCH] trace/trace_uprobe: Only invoke uprobe ebpf handler when event matches.
config: i386-defconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/6c778c475c0a6c8a11c1a74705a5856681cf6cf0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review David-Wang/trace-trace_uprobe-Only-invoke-uprobe-ebpf-handler-when-event-matches/20221104-210925
        git checkout 6c778c475c0a6c8a11c1a74705a5856681cf6cf0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 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>

All warnings (new ones prefixed by >>):

   kernel/trace/trace_uprobe.c: In function '__uprobe_perf_func':
>> kernel/trace/trace_uprobe.c:1349:14: warning: variable 'trace_event_match' set but not used [-Wunused-but-set-variable]
    1349 |         bool trace_event_match = true;
         |              ^~~~~~~~~~~~~~~~~


vim +/trace_event_match +1349 kernel/trace/trace_uprobe.c

  1338	
  1339	static void __uprobe_perf_func(struct trace_uprobe *tu,
  1340				       unsigned long func, struct pt_regs *regs,
  1341				       struct uprobe_cpu_buffer *ucb, int dsize)
  1342	{
  1343		struct trace_event_call *call = trace_probe_event_call(&tu->tp);
  1344		struct uprobe_trace_entry_head *entry;
  1345		struct hlist_head *head;
  1346		void *data;
  1347		int size, esize;
  1348		int rctx;
> 1349		bool trace_event_match = true;
  1350	
  1351	
  1352		preempt_disable();
  1353		head = this_cpu_ptr(call->perf_events);
  1354		if (hlist_empty(head)) {
  1355			trace_event_match = false;
  1356			goto out;
  1357		}
  1358	
  1359		esize = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
  1360		size = esize + tu->tp.size + dsize;
  1361		size = ALIGN(size + sizeof(u32), sizeof(u64)) - sizeof(u32);
  1362		if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE, "profile buffer not large enough"))
  1363			goto out;
  1364	
  1365		entry = perf_trace_buf_alloc(size, NULL, &rctx);
  1366		if (!entry)
  1367			goto out;
  1368	
  1369		if (is_ret_probe(tu)) {
  1370			entry->vaddr[0] = func;
  1371			entry->vaddr[1] = instruction_pointer(regs);
  1372			data = DATAOF_TRACE_ENTRY(entry, true);
  1373		} else {
  1374			entry->vaddr[0] = instruction_pointer(regs);
  1375			data = DATAOF_TRACE_ENTRY(entry, false);
  1376		}
  1377	
  1378		memcpy(data, ucb->buf, tu->tp.size + dsize);
  1379	
  1380		if (size - esize > tu->tp.size + dsize) {
  1381			int len = tu->tp.size + dsize;
  1382	
  1383			memset(data + len, 0, size - esize - len);
  1384		}
  1385		perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
  1386				      head, NULL);
  1387	 out:
  1388		preempt_enable();
  1389
David Wang Nov. 5, 2022, 6:38 a.m. UTC | #2 
Fix compile warning: unused variable trace_event_match
    
`trace_event_match` is used to indicate whether
current event matches any registered event. For
now, this variable is only used for ebpf handler
and CONFIG_BPF_EVENTS is needed, otherwise a compile
warning would be generated about unused variable.
    
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: David Wang <00107082@163.com>
--
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 6f13163c0c0f..0548c651699e 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1346,13 +1346,17 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
        void *data;
        int size, esize;
        int rctx;
+#ifdef CONFIG_BPF_EVENTS
        bool trace_event_match = true;
+#endif /* CONFIG_BPF_EVENTS */
 
 
        preempt_disable();
        head = this_cpu_ptr(call->perf_events);
        if (hlist_empty(head)) {
+#ifdef CONFIG_BPF_EVENTS
                trace_event_match = false;
+#endif /* CONFIG_BPF_EVENTS */
                goto out;
        }
 
--

















At 2022-11-05 05:30:37, "kernel test robot" <lkp@intel.com> wrote:
>Hi David,
>
>Thank you for the patch! Perhaps something to improve:
>
>[auto build test WARNING on linus/master]
>[also build test WARNING on v6.1-rc3 next-20221104]
>[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#_base_tree_information]
>
>url:    https://github.com/intel-lab-lkp/linux/commits/David-Wang/trace-trace_uprobe-Only-invoke-uprobe-ebpf-handler-when-event-matches/20221104-210925
>patch link:    https://lore.kernel.org/r/41828b90.5798.18442c10017.Coremail.00107082%40163.com
>patch subject: [PATCH] trace/trace_uprobe: Only invoke uprobe ebpf handler when event matches.
>config: i386-defconfig
>compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
>reproduce (this is a W=1 build):
>        # https://github.com/intel-lab-lkp/linux/commit/6c778c475c0a6c8a11c1a74705a5856681cf6cf0
>        git remote add linux-review https://github.com/intel-lab-lkp/linux
>        git fetch --no-tags linux-review David-Wang/trace-trace_uprobe-Only-invoke-uprobe-ebpf-handler-when-event-matches/20221104-210925
>        git checkout 6c778c475c0a6c8a11c1a74705a5856681cf6cf0
>        # save the config file
>        mkdir build_dir && cp config build_dir/.config
>        make W=1 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>
>
>All warnings (new ones prefixed by >>):
>
>   kernel/trace/trace_uprobe.c: In function '__uprobe_perf_func':
>>> kernel/trace/trace_uprobe.c:1349:14: warning: variable 'trace_event_match' set but not used [-Wunused-but-set-variable]
>    1349 |         bool trace_event_match = true;
>         |              ^~~~~~~~~~~~~~~~~
>
>
>vim +/trace_event_match +1349 kernel/trace/trace_uprobe.c
>
>  1338	
>  1339	static void __uprobe_perf_func(struct trace_uprobe *tu,
>  1340				       unsigned long func, struct pt_regs *regs,
>  1341				       struct uprobe_cpu_buffer *ucb, int dsize)
>  1342	{
>  1343		struct trace_event_call *call = trace_probe_event_call(&tu->tp);
>  1344		struct uprobe_trace_entry_head *entry;
>  1345		struct hlist_head *head;
>  1346		void *data;
>  1347		int size, esize;
>  1348		int rctx;
>> 1349		bool trace_event_match = true;
>  1350	
>  1351	
>  1352		preempt_disable();
>  1353		head = this_cpu_ptr(call->perf_events);
>  1354		if (hlist_empty(head)) {
>  1355			trace_event_match = false;
>  1356			goto out;
>  1357		}
>  1358	
>  1359		esize = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
>  1360		size = esize + tu->tp.size + dsize;
>  1361		size = ALIGN(size + sizeof(u32), sizeof(u64)) - sizeof(u32);
>  1362		if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE, "profile buffer not large enough"))
>  1363			goto out;
>  1364	
>  1365		entry = perf_trace_buf_alloc(size, NULL, &rctx);
>  1366		if (!entry)
>  1367			goto out;
>  1368	
>  1369		if (is_ret_probe(tu)) {
>  1370			entry->vaddr[0] = func;
>  1371			entry->vaddr[1] = instruction_pointer(regs);
>  1372			data = DATAOF_TRACE_ENTRY(entry, true);
>  1373		} else {
>  1374			entry->vaddr[0] = instruction_pointer(regs);
>  1375			data = DATAOF_TRACE_ENTRY(entry, false);
>  1376		}
>  1377	
>  1378		memcpy(data, ucb->buf, tu->tp.size + dsize);
>  1379	
>  1380		if (size - esize > tu->tp.size + dsize) {
>  1381			int len = tu->tp.size + dsize;
>  1382	
>  1383			memset(data + len, 0, size - esize - len);
>  1384		}
>  1385		perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
>  1386				      head, NULL);
>  1387	 out:
>  1388		preempt_enable();
>  1389	
>
>-- 
>0-DAY CI Kernel Test Service
>https://01.org/lkp
diff mbox series

Patch

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index fb58e86dd117..6f13163c0c0f 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1346,27 +1346,20 @@  static void __uprobe_perf_func(struct trace_uprobe *tu,
 	void *data;
 	int size, esize;
 	int rctx;
+	bool trace_event_match = true;
 
-#ifdef CONFIG_BPF_EVENTS
-	if (bpf_prog_array_valid(call)) {
-		u32 ret;
 
-		ret = bpf_prog_run_array_sleepable(call->prog_array, regs, bpf_prog_run);
-		if (!ret)
-			return;
+	preempt_disable();
+	head = this_cpu_ptr(call->perf_events);
+	if (hlist_empty(head)) {
+		trace_event_match = false;
+		goto out;
 	}
-#endif /* CONFIG_BPF_EVENTS */
 
 	esize = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
-
 	size = esize + tu->tp.size + dsize;
 	size = ALIGN(size + sizeof(u32), sizeof(u64)) - sizeof(u32);
 	if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE, "profile buffer not large enough"))
-		return;
-
-	preempt_disable();
-	head = this_cpu_ptr(call->perf_events);
-	if (hlist_empty(head))
 		goto out;
 
 	entry = perf_trace_buf_alloc(size, NULL, &rctx);
@@ -1389,11 +1382,21 @@  static void __uprobe_perf_func(struct trace_uprobe *tu,
 
 		memset(data + len, 0, size - esize - len);
 	}
-
 	perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
 			      head, NULL);
  out:
 	preempt_enable();
+
+#ifdef CONFIG_BPF_EVENTS
+	if (trace_event_match && bpf_prog_array_valid(call)) {
+		u32 ret;
+
+		ret = bpf_prog_run_array_sleepable(call->prog_array, regs, bpf_prog_run);
+		if (!ret)
+			return;
+	}
+#endif /* CONFIG_BPF_EVENTS */
+
 }
 
 /* uprobe profile handler */