Message ID | 20230324123142.7463-1-mathieu.desnoyers@efficios.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp596791vqo; Fri, 24 Mar 2023 05:48:07 -0700 (PDT) X-Google-Smtp-Source: AKy350aFnbto0tW9T/yid+v+6zPK08HgqZ1owqR4mYoBZjU1tZDZgQrYNb/DH7vO12A8xXJpduiy X-Received: by 2002:a17:907:3e1a:b0:932:853c:c958 with SMTP id hp26-20020a1709073e1a00b00932853cc958mr3058843ejc.25.1679662087208; Fri, 24 Mar 2023 05:48:07 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679662087; cv=none; d=google.com; s=arc-20160816; b=JFbqw9E9roHu0w29b9hdlWdnaX13NcmPgZ+Rr1PAImqMXFP7nqTqTnxLcdasghAqkB 68d4ryBTaxGixWCilJV3J0426q03Eced5ghmEkI74h9YJ7rpvnNgNjoqlZltCbe64Nr/ FNxGILJ7MyVsNO6j0pZ3W5nIVuPjQRtSnglOrtBYCvAs73tn1EPtXC25NHdQVs6BU1lF LR3TDBIrEtn7hKLje4KADI7WMuSfP2r6G0zUR+qxKAEgoOhitAU1L3yfKapbmB4DdlkL GwYquD1WV78P1G4kX0PqgTlrC8o5b5tTNrXh0K5huENLgowqQw6cPqNJZrVyIemi3FcP /u+g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=kRQvDcM4DelFYgd3I97ki/p15wFCkbHKrypC3lZKtSg=; b=IK08Ar+WuUBSkTQV4R1r1HxsxmIrc/7x0OzGMhOHw/g9tAA6L3hTpD6OH87PkVySID hEpral3ooK3fRAYKhksrr+6dn6PnDQ1325mcyoevJGpOj/WPTp73zyZNHtw1xLV9H0p7 we07+gTwTG58hl5hgcZGzbX7Kq6WFu3PPz9CLhc/q9jVCGiA+XdeGqdogruJ7AFYndfF Jw4hEGU2knaHbFK/LBP0cAOp3mK7Sc+cmcGIk5xWhmAbheraxu7m4NQItslsgVc6e/XO eCakWghbsI6h0QAYQg57zxUlA7QxUSHTlp4dpPfsCOBosgrR0BkZ976tf06Msd0eJTcO KrlQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@efficios.com header.s=smtpout1 header.b="rY+ccjp/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b19-20020a1709063f9300b0091fca6007bbsi19349090ejj.488.2023.03.24.05.47.43; Fri, 24 Mar 2023 05:48:07 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@efficios.com header.s=smtpout1 header.b="rY+ccjp/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231135AbjCXMcS (ORCPT <rfc822;ezelljr.billy@gmail.com> + 99 others); Fri, 24 Mar 2023 08:32:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59942 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231387AbjCXMcN (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 24 Mar 2023 08:32:13 -0400 Received: from smtpout.efficios.com (unknown [IPv6:2607:5300:203:b2ee::31e5]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 70F5C9010 for <linux-kernel@vger.kernel.org>; Fri, 24 Mar 2023 05:31:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=efficios.com; s=smtpout1; t=1679661108; bh=bbY7fKmOGZlbBs0GEtdCsGyFjjGMwhT2jcpGl3ev/TY=; h=From:To:Cc:Subject:Date:From; b=rY+ccjp/85u8ItujvEpb1pfi4OpOnJm6TJatCUXIqV9reOpDwrjI+M2QosQkL98Bj LS3RfELlMgAEHHjnZPg/wqUEIrMkC0PwGGazFoC6npCdxYOPMBirWKKT6Eyuj7xti/ IJcBFW1O1VXTS8r4vkaD2KgP0D92qUF3VjDSv0sO7MOJysaD/Fru+NXXJmvCU7xQth tuN1CQldqlKp/XL6hogNJEr7XVnVB/MDCVLV+wIsW6o0cYVMAvQBweTZ9YCKkorT9C GlsXxlnSQRxv58+bsdBBJubiZhyzZTyeVXbFjHeAk1PWicZPu8j+kUBoZ3hut3StYB 4HTfGotsAricw== Received: from localhost.localdomain (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) by smtpout.efficios.com (Postfix) with ESMTPSA id 4PjhR80crdzt63; Fri, 24 Mar 2023 08:31:48 -0400 (EDT) From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> To: Steven Rostedt <rostedt@goodmis.org> Cc: linux-kernel@vger.kernel.org, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Mark Rutland <mark.rutland@arm.com>, Mark Rutland <mark.rutlnand@arm.com>, Masami Hiramatsu <mhiramat@kernel.org>, "Jose E . Marchesi" <jose.marchesi@oracle.com>, Nick Desaulniers <ndesaulniers@google.com>, Sami Tolvanen <samitolvanen@google.com>, Peter Zijlstra <peterz@infradead.org> Subject: [PATCH v3] tracepoint: Fix CFI failures with tp_sub_func Date: Fri, 24 Mar 2023 08:31:42 -0400 Message-Id: <20230324123142.7463-1-mathieu.desnoyers@efficios.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DKIM_VALID_EF,RDNS_NONE,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Level: * X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761253352944622554?= X-GMAIL-MSGID: =?utf-8?q?1761253352944622554?= |
Series |
[v3] tracepoint: Fix CFI failures with tp_sub_func
|
|
Commit Message
Mathieu Desnoyers
March 24, 2023, 12:31 p.m. UTC
When CLANG_CFI is in use, using tracing will occasionally result in CFI failures, e.g. | CFI failure at syscall_trace_enter+0x66c/0x7d0 (target: tp_stub_func+0x0/0x2c; expected type: 0x4877830c) | Internal error: Oops - CFI: 00000000f200823a [#1] PREEMPT SMP | CPU: 2 PID: 118 Comm: klogd Not tainted 6.3.0-rc3-00002-gc242ea6f2f98 #2 | Hardware name: linux,dummy-virt (DT) | pstate: 30400005 (nzCV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) | pc : syscall_trace_enter+0x66c/0x7d0 | lr : syscall_trace_enter+0x5e8/0x7d0 | sp : ffff800015ce7d80 | x29: ffff800015ce7d80 x28: ffff000017538000 x27: 0000000000000003 | x26: ffff8000084c9454 x25: ffff00001182bd10 x24: dfff800000000000 | x23: 1fffe00002ea7001 x22: ffff00001182bd10 x21: ffff000017538008 | x20: ffff000017538000 x19: ffff800015ce7eb0 x18: 0000000000000000 | x17: 000000004877830c x16: 00000000a540670c x15: 0000000000000000 | x14: 0000000000000000 x13: 0000000000000000 x12: ff80800008039d8c | x11: ff80800008039dd0 x10: 0000000000000000 x9 : 0000000000000000 | x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000 | x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 | x2 : 00000000000000ce x1 : ffff800015ce7eb0 x0 : ffff800013d55000 | Call trace: | syscall_trace_enter+0x66c/0x7d0 | el0_svc_common+0x1dc/0x268 | do_el0_svc+0x70/0x1a4 | el0_svc+0x58/0x14c | el0t_64_sync_handler+0x84/0xf0 | el0t_64_sync+0x190/0x194 | Code: 72906191 72a90ef1 6b11021f 54000040 (d4304740) | ---[ end trace 0000000000000000 ]--- This happens because the function prototype of tp_sub_func doesn't match the prototype of the tracepoint function. As each tracepoint may have a distinct prototype, it's not possible to share a common stub function. Avoid this by comparing the tracepoint function pointer to the value 1 before calling each function. Prior to this change, it_func_ptr->func was loaded twice per loop iteration: it_func = READ_ONCE((it_func_ptr)->func); [...] } while ((++it_func_ptr)->func); With this change, the loaded it_func is used for comparisons with 0 (end of loop), 1 (skip) and as a function pointer, thus saving a load per loop. The comparison with <= TRACEPOINT_FUNC_SKIP is done on purpose to branch over both comparisons with 0 and 1 with a single branch for each loop and an extra conditional branch on the last loop to compare with 0. A simplified example of this compiled on godbolt.org: #include <stdint.h> typedef void (*f_t)(void); volatile f_t *arr; void fct(void) { volatile f_t *iter = &arr[0]; for (;;) { f_t l_iter = *iter; if (((uintptr_t)l_iter <= 0x1)) { if ((uintptr_t)l_iter == 0x1) continue; else break; } l_iter(); iter++; } } Generates: x86-64 gcc 12.2 -O2: fct: push rbx mov rbx, QWORD PTR arr[rip] .L29: mov rax, QWORD PTR [rbx] cmp rax, 1 jbe .L38 .L30: call rax mov rax, QWORD PTR [rbx+8] add rbx, 8 cmp rax, 1 ja .L30 .L38: je .L29 pop rbx ret clang 16.0.0 -O2: fct: # @fct push rbx mov rbx, qword ptr [rip + arr] .LBB0_1: # =>This Inner Loop Header: Depth=1 mov rax, qword ptr [rbx] cmp rax, 1 ja .LBB0_4 je .LBB0_1 jmp .LBB0_3 .LBB0_4: # in Loop: Header=BB0_1 Depth=1 call rax add rbx, 8 jmp .LBB0_1 .LBB0_3: pop rbx ret arr: .quad 0 On x86, the result of the "cmp" instruction is used to conditionally branch based on both inequality (jbe, ja) and equality (je), thus keeping the code size small and limiting the number of instructions to execute on this tracepoint instrumentation fast path. Reported-by: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Tested-by: Mark Rutland <mark.rutlnand@arm.com> Acked-by: Mark Rutland <mark.rutland@arm.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Jose E. Marchesi <jose.marchesi@oracle.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Sami Tolvanen <samitolvanen@google.com> Cc: Peter Zijlstra <peterz@infradead.org> --- Changes since v2: - Fix typo in commit message. - Add acked-by, tested-by. --- include/linux/tracepoint.h | 14 ++++++++++++-- kernel/tracepoint.c | 20 +++++++------------- 2 files changed, 19 insertions(+), 15 deletions(-)
Comments
On 2023-03-24 08:31, Mathieu Desnoyers wrote: > When CLANG_CFI is in use, using tracing will occasionally result in > CFI failures, e.g. > > | CFI failure at syscall_trace_enter+0x66c/0x7d0 (target: tp_stub_func+0x0/0x2c; expected type: 0x4877830c) > | Internal error: Oops - CFI: 00000000f200823a [#1] PREEMPT SMP > | CPU: 2 PID: 118 Comm: klogd Not tainted 6.3.0-rc3-00002-gc242ea6f2f98 #2 > | Hardware name: linux,dummy-virt (DT) > | pstate: 30400005 (nzCV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > | pc : syscall_trace_enter+0x66c/0x7d0 > | lr : syscall_trace_enter+0x5e8/0x7d0 > | sp : ffff800015ce7d80 > | x29: ffff800015ce7d80 x28: ffff000017538000 x27: 0000000000000003 > | x26: ffff8000084c9454 x25: ffff00001182bd10 x24: dfff800000000000 > | x23: 1fffe00002ea7001 x22: ffff00001182bd10 x21: ffff000017538008 > | x20: ffff000017538000 x19: ffff800015ce7eb0 x18: 0000000000000000 > | x17: 000000004877830c x16: 00000000a540670c x15: 0000000000000000 > | x14: 0000000000000000 x13: 0000000000000000 x12: ff80800008039d8c > | x11: ff80800008039dd0 x10: 0000000000000000 x9 : 0000000000000000 > | x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000 > | x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 > | x2 : 00000000000000ce x1 : ffff800015ce7eb0 x0 : ffff800013d55000 > | Call trace: > | syscall_trace_enter+0x66c/0x7d0 > | el0_svc_common+0x1dc/0x268 > | do_el0_svc+0x70/0x1a4 > | el0_svc+0x58/0x14c > | el0t_64_sync_handler+0x84/0xf0 > | el0t_64_sync+0x190/0x194 > | Code: 72906191 72a90ef1 6b11021f 54000040 (d4304740) > | ---[ end trace 0000000000000000 ]--- > > This happens because the function prototype of tp_sub_func doesn't match > the prototype of the tracepoint function. As each tracepoint may have a > distinct prototype, it's not possible to share a common stub function. > > Avoid this by comparing the tracepoint function pointer to the value 1 > before calling each function. > > Prior to this change, it_func_ptr->func was loaded twice per loop > iteration: > > it_func = READ_ONCE((it_func_ptr)->func); > [...] > } while ((++it_func_ptr)->func); > > With this change, the loaded it_func is used for comparisons with 0 (end > of loop), 1 (skip) and as a function pointer, thus saving a load per > loop. > > The comparison with <= TRACEPOINT_FUNC_SKIP is done on purpose to branch > over both comparisons with 0 and 1 with a single branch for each loop > and an extra conditional branch on the last loop to compare with 0. > > A simplified example of this compiled on godbolt.org: > > #include <stdint.h> > > typedef void (*f_t)(void); > volatile f_t *arr; > > void fct(void) > { > volatile f_t *iter = &arr[0]; > > for (;;) { > f_t l_iter = *iter; > if (((uintptr_t)l_iter <= 0x1)) { > if ((uintptr_t)l_iter == 0x1) > continue; > else > break; > } > l_iter(); > iter++; > } > } > > Generates: > > x86-64 gcc 12.2 -O2: > > fct: > push rbx > mov rbx, QWORD PTR arr[rip] > .L29: > mov rax, QWORD PTR [rbx] > cmp rax, 1 > jbe .L38 > .L30: > call rax > mov rax, QWORD PTR [rbx+8] > add rbx, 8 > cmp rax, 1 > ja .L30 > .L38: > je .L29 > pop rbx > ret > > clang 16.0.0 -O2: > > fct: # @fct > push rbx > mov rbx, qword ptr [rip + arr] > .LBB0_1: # =>This Inner Loop Header: Depth=1 > mov rax, qword ptr [rbx] > cmp rax, 1 > ja .LBB0_4 > je .LBB0_1 > jmp .LBB0_3 > .LBB0_4: # in Loop: Header=BB0_1 Depth=1 > call rax > add rbx, 8 > jmp .LBB0_1 > .LBB0_3: > pop rbx > ret > arr: > .quad 0 > > On x86, the result of the "cmp" instruction is used to conditionally > branch based on both inequality (jbe, ja) and equality (je), thus > keeping the code size small and limiting the number of instructions to > execute on this tracepoint instrumentation fast path. > > Reported-by: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Tested-by: Mark Rutland <mark.rutlnand@arm.com> It appears there was a typo in this Tested-by address from Mark. Steven, should I resend or can you correct this as you pick it up ? Thanks, Mathieu > Acked-by: Mark Rutland <mark.rutland@arm.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Jose E. Marchesi <jose.marchesi@oracle.com> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Sami Tolvanen <samitolvanen@google.com> > Cc: Peter Zijlstra <peterz@infradead.org> > --- > Changes since v2: > - Fix typo in commit message. > - Add acked-by, tested-by. > --- > include/linux/tracepoint.h | 14 ++++++++++++-- > kernel/tracepoint.c | 20 +++++++------------- > 2 files changed, 19 insertions(+), 15 deletions(-) > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > index 4b33b95eb8be..0aeac249d412 100644 > --- a/include/linux/tracepoint.h > +++ b/include/linux/tracepoint.h > @@ -33,6 +33,9 @@ struct trace_eval_map { > > #define TRACEPOINT_DEFAULT_PRIO 10 > > +/* Reserved value for tracepoint callback. */ > +#define TRACEPOINT_FUNC_SKIP ((void *) 0x1) > + > extern struct srcu_struct tracepoint_srcu; > > extern int > @@ -314,11 +317,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > it_func_ptr = \ > rcu_dereference_raw((&__tracepoint_##_name)->funcs); \ > if (it_func_ptr) { \ > - do { \ > + for (;;) { \ > it_func = READ_ONCE((it_func_ptr)->func); \ > + if ((uintptr_t) it_func <= (uintptr_t) TRACEPOINT_FUNC_SKIP) { \ > + if (it_func == TRACEPOINT_FUNC_SKIP) \ > + continue; \ > + else \ > + break; \ > + } \ > __data = (it_func_ptr)->data; \ > ((void(*)(void *, proto))(it_func))(__data, args); \ > - } while ((++it_func_ptr)->func); \ > + it_func_ptr++; \ > + } \ > } \ > return 0; \ > } \ > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index f23144af5743..2fa108ddbbc2 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -98,12 +98,6 @@ struct tp_probes { > struct tracepoint_func probes[]; > }; > > -/* Called in removal of a func but failed to allocate a new tp_funcs */ > -static void tp_stub_func(void) > -{ > - return; > -} > - > static inline void *allocate_probes(int count) > { > struct tp_probes *p = kmalloc(struct_size(p, probes, count), > @@ -193,8 +187,8 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > if (old) { > /* (N -> N+1), (N != 0, 1) probes */ > for (iter_probes = 0; old[iter_probes].func; iter_probes++) { > - if (old[iter_probes].func == tp_stub_func) > - continue; /* Skip stub functions. */ > + if (old[iter_probes].func == TRACEPOINT_FUNC_SKIP) > + continue; > if (old[iter_probes].func == tp_func->func && > old[iter_probes].data == tp_func->data) > return ERR_PTR(-EEXIST); > @@ -208,7 +202,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > if (old) { > nr_probes = 0; > for (iter_probes = 0; old[iter_probes].func; iter_probes++) { > - if (old[iter_probes].func == tp_stub_func) > + if (old[iter_probes].func == TRACEPOINT_FUNC_SKIP) > continue; > /* Insert before probes of lower priority */ > if (pos < 0 && old[iter_probes].prio < prio) > @@ -246,7 +240,7 @@ static void *func_remove(struct tracepoint_func **funcs, > for (nr_probes = 0; old[nr_probes].func; nr_probes++) { > if ((old[nr_probes].func == tp_func->func && > old[nr_probes].data == tp_func->data) || > - old[nr_probes].func == tp_stub_func) > + old[nr_probes].func == TRACEPOINT_FUNC_SKIP) > nr_del++; > } > } > @@ -269,7 +263,7 @@ static void *func_remove(struct tracepoint_func **funcs, > for (i = 0; old[i].func; i++) { > if ((old[i].func != tp_func->func || > old[i].data != tp_func->data) && > - old[i].func != tp_stub_func) > + old[i].func != TRACEPOINT_FUNC_SKIP) > new[j++] = old[i]; > } > new[nr_probes - nr_del].func = NULL; > @@ -277,12 +271,12 @@ static void *func_remove(struct tracepoint_func **funcs, > } else { > /* > * Failed to allocate, replace the old function > - * with calls to tp_stub_func. > + * with TRACEPOINT_FUNC_SKIP. > */ > for (i = 0; old[i].func; i++) { > if (old[i].func == tp_func->func && > old[i].data == tp_func->data) > - WRITE_ONCE(old[i].func, tp_stub_func); > + WRITE_ONCE(old[i].func, TRACEPOINT_FUNC_SKIP); > } > *funcs = old; > }
Hi Steve, On Fri, Mar 24, 2023 at 08:33:35AM -0400, Mathieu Desnoyers wrote: > On 2023-03-24 08:31, Mathieu Desnoyers wrote: > > When CLANG_CFI is in use, using tracing will occasionally result in > > CFI failures, e.g. > > > > | CFI failure at syscall_trace_enter+0x66c/0x7d0 (target: tp_stub_func+0x0/0x2c; expected type: 0x4877830c) > > | Internal error: Oops - CFI: 00000000f200823a [#1] PREEMPT SMP > > | CPU: 2 PID: 118 Comm: klogd Not tainted 6.3.0-rc3-00002-gc242ea6f2f98 #2 > > | Hardware name: linux,dummy-virt (DT) > > | pstate: 30400005 (nzCV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > | pc : syscall_trace_enter+0x66c/0x7d0 > > | lr : syscall_trace_enter+0x5e8/0x7d0 > > | sp : ffff800015ce7d80 > > | x29: ffff800015ce7d80 x28: ffff000017538000 x27: 0000000000000003 > > | x26: ffff8000084c9454 x25: ffff00001182bd10 x24: dfff800000000000 > > | x23: 1fffe00002ea7001 x22: ffff00001182bd10 x21: ffff000017538008 > > | x20: ffff000017538000 x19: ffff800015ce7eb0 x18: 0000000000000000 > > | x17: 000000004877830c x16: 00000000a540670c x15: 0000000000000000 > > | x14: 0000000000000000 x13: 0000000000000000 x12: ff80800008039d8c > > | x11: ff80800008039dd0 x10: 0000000000000000 x9 : 0000000000000000 > > | x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000 > > | x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 > > | x2 : 00000000000000ce x1 : ffff800015ce7eb0 x0 : ffff800013d55000 > > | Call trace: > > | syscall_trace_enter+0x66c/0x7d0 > > | el0_svc_common+0x1dc/0x268 > > | do_el0_svc+0x70/0x1a4 > > | el0_svc+0x58/0x14c > > | el0t_64_sync_handler+0x84/0xf0 > > | el0t_64_sync+0x190/0x194 > > | Code: 72906191 72a90ef1 6b11021f 54000040 (d4304740) > > | ---[ end trace 0000000000000000 ]--- > > > > This happens because the function prototype of tp_sub_func doesn't match > > the prototype of the tracepoint function. As each tracepoint may have a > > distinct prototype, it's not possible to share a common stub function. > > > > Avoid this by comparing the tracepoint function pointer to the value 1 > > before calling each function. [...] > > Reported-by: Mark Rutland <mark.rutland@arm.com> > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Tested-by: Mark Rutland <mark.rutlnand@arm.com> > > It appears there was a typo in this Tested-by address from Mark. Steven, > should I resend or can you correct this as you pick it up ? Sorry to ping, but are you planning to pick this up, and/or did you want this resent with the Testing-by typo fixed? I couldn't spot it in the tracing tree, and I'm not sure if it's still on your TODO list for review or has just been missed. Thanks, Mark. > > Thanks, > > Mathieu > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: Jose E. Marchesi <jose.marchesi@oracle.com> > > Cc: Nick Desaulniers <ndesaulniers@google.com> > > Cc: Sami Tolvanen <samitolvanen@google.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > --- > > Changes since v2: > > - Fix typo in commit message. > > - Add acked-by, tested-by. > > --- > > include/linux/tracepoint.h | 14 ++++++++++++-- > > kernel/tracepoint.c | 20 +++++++------------- > > 2 files changed, 19 insertions(+), 15 deletions(-) > > > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > > index 4b33b95eb8be..0aeac249d412 100644 > > --- a/include/linux/tracepoint.h > > +++ b/include/linux/tracepoint.h > > @@ -33,6 +33,9 @@ struct trace_eval_map { > > #define TRACEPOINT_DEFAULT_PRIO 10 > > +/* Reserved value for tracepoint callback. */ > > +#define TRACEPOINT_FUNC_SKIP ((void *) 0x1) > > + > > extern struct srcu_struct tracepoint_srcu; > > extern int > > @@ -314,11 +317,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > > it_func_ptr = \ > > rcu_dereference_raw((&__tracepoint_##_name)->funcs); \ > > if (it_func_ptr) { \ > > - do { \ > > + for (;;) { \ > > it_func = READ_ONCE((it_func_ptr)->func); \ > > + if ((uintptr_t) it_func <= (uintptr_t) TRACEPOINT_FUNC_SKIP) { \ > > + if (it_func == TRACEPOINT_FUNC_SKIP) \ > > + continue; \ > > + else \ > > + break; \ > > + } \ > > __data = (it_func_ptr)->data; \ > > ((void(*)(void *, proto))(it_func))(__data, args); \ > > - } while ((++it_func_ptr)->func); \ > > + it_func_ptr++; \ > > + } \ > > } \ > > return 0; \ > > } \ > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > > index f23144af5743..2fa108ddbbc2 100644 > > --- a/kernel/tracepoint.c > > +++ b/kernel/tracepoint.c > > @@ -98,12 +98,6 @@ struct tp_probes { > > struct tracepoint_func probes[]; > > }; > > -/* Called in removal of a func but failed to allocate a new tp_funcs */ > > -static void tp_stub_func(void) > > -{ > > - return; > > -} > > - > > static inline void *allocate_probes(int count) > > { > > struct tp_probes *p = kmalloc(struct_size(p, probes, count), > > @@ -193,8 +187,8 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > > if (old) { > > /* (N -> N+1), (N != 0, 1) probes */ > > for (iter_probes = 0; old[iter_probes].func; iter_probes++) { > > - if (old[iter_probes].func == tp_stub_func) > > - continue; /* Skip stub functions. */ > > + if (old[iter_probes].func == TRACEPOINT_FUNC_SKIP) > > + continue; > > if (old[iter_probes].func == tp_func->func && > > old[iter_probes].data == tp_func->data) > > return ERR_PTR(-EEXIST); > > @@ -208,7 +202,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, > > if (old) { > > nr_probes = 0; > > for (iter_probes = 0; old[iter_probes].func; iter_probes++) { > > - if (old[iter_probes].func == tp_stub_func) > > + if (old[iter_probes].func == TRACEPOINT_FUNC_SKIP) > > continue; > > /* Insert before probes of lower priority */ > > if (pos < 0 && old[iter_probes].prio < prio) > > @@ -246,7 +240,7 @@ static void *func_remove(struct tracepoint_func **funcs, > > for (nr_probes = 0; old[nr_probes].func; nr_probes++) { > > if ((old[nr_probes].func == tp_func->func && > > old[nr_probes].data == tp_func->data) || > > - old[nr_probes].func == tp_stub_func) > > + old[nr_probes].func == TRACEPOINT_FUNC_SKIP) > > nr_del++; > > } > > } > > @@ -269,7 +263,7 @@ static void *func_remove(struct tracepoint_func **funcs, > > for (i = 0; old[i].func; i++) { > > if ((old[i].func != tp_func->func || > > old[i].data != tp_func->data) && > > - old[i].func != tp_stub_func) > > + old[i].func != TRACEPOINT_FUNC_SKIP) > > new[j++] = old[i]; > > } > > new[nr_probes - nr_del].func = NULL; > > @@ -277,12 +271,12 @@ static void *func_remove(struct tracepoint_func **funcs, > > } else { > > /* > > * Failed to allocate, replace the old function > > - * with calls to tp_stub_func. > > + * with TRACEPOINT_FUNC_SKIP. > > */ > > for (i = 0; old[i].func; i++) { > > if (old[i].func == tp_func->func && > > old[i].data == tp_func->data) > > - WRITE_ONCE(old[i].func, tp_stub_func); > > + WRITE_ONCE(old[i].func, TRACEPOINT_FUNC_SKIP); > > } > > *funcs = old; > > } > > -- > Mathieu Desnoyers > EfficiOS Inc. > https://www.efficios.com >
On Tue, 4 Apr 2023 11:59:26 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > Sorry to ping, but are you planning to pick this up, and/or did you want this > resent with the Testing-by typo fixed? > > I couldn't spot it in the tracing tree, and I'm not sure if it's still on your > TODO list for review or has just been missed. Actually, we may be going back to your original patches, as having a stub function for every trace event will allow us to attach fprobes to them. And we actually have a need to do that (connecting to tracepoints to get more info than the trace event gives). Masami is currently working on this. -- Steve
On Tue, 4 Apr 2023 16:22:41 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Tue, 4 Apr 2023 11:59:26 +0100 > Mark Rutland <mark.rutland@arm.com> wrote: > > > Sorry to ping, but are you planning to pick this up, and/or did you want this > > resent with the Testing-by typo fixed? > > > > I couldn't spot it in the tracing tree, and I'm not sure if it's still on your > > TODO list for review or has just been missed. > > Actually, we may be going back to your original patches, as having a stub > function for every trace event will allow us to attach fprobes to them. And > we actually have a need to do that (connecting to tracepoints to get more > info than the trace event gives). > > Masami is currently working on this. I think we need another stub function similar to the original patch, because the __tracestub_* functions are usually not called back. Even if I enabled the static key to enable the stub callback, if user sets another callback handler to the tracepoint (e.g. enable trace event on it), the __tracestub_* function is not called anymore. Thus I think we just pick this version and add another patch to introduce __probestub_* functions in macro, and fprobe on it. This new stub function is something like the original one so that it doesn't break CFI. And it will be registered to tracepoint when the new fprobe based dynamic event is enabled. Thank you,
On Sat, 8 Apr 2023 23:03:44 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > I think we need another stub function similar to the original patch, because > the __tracestub_* functions are usually not called back. Even if I enabled > the static key to enable the stub callback, if user sets another callback > handler to the tracepoint (e.g. enable trace event on it), the __tracestub_* > function is not called anymore. > > Thus I think we just pick this version and add another patch to introduce > __probestub_* functions in macro, and fprobe on it. This new stub function is > something like the original one so that it doesn't break CFI. And it will be > registered to tracepoint when the new fprobe based dynamic event is enabled. > If we are going to add *another* stub, then no, I don't want to use it. But I don't see why we can't use one stub for both? What's the issue you are having? -- Steve
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h index 4b33b95eb8be..0aeac249d412 100644 --- a/include/linux/tracepoint.h +++ b/include/linux/tracepoint.h @@ -33,6 +33,9 @@ struct trace_eval_map { #define TRACEPOINT_DEFAULT_PRIO 10 +/* Reserved value for tracepoint callback. */ +#define TRACEPOINT_FUNC_SKIP ((void *) 0x1) + extern struct srcu_struct tracepoint_srcu; extern int @@ -314,11 +317,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) it_func_ptr = \ rcu_dereference_raw((&__tracepoint_##_name)->funcs); \ if (it_func_ptr) { \ - do { \ + for (;;) { \ it_func = READ_ONCE((it_func_ptr)->func); \ + if ((uintptr_t) it_func <= (uintptr_t) TRACEPOINT_FUNC_SKIP) { \ + if (it_func == TRACEPOINT_FUNC_SKIP) \ + continue; \ + else \ + break; \ + } \ __data = (it_func_ptr)->data; \ ((void(*)(void *, proto))(it_func))(__data, args); \ - } while ((++it_func_ptr)->func); \ + it_func_ptr++; \ + } \ } \ return 0; \ } \ diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index f23144af5743..2fa108ddbbc2 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -98,12 +98,6 @@ struct tp_probes { struct tracepoint_func probes[]; }; -/* Called in removal of a func but failed to allocate a new tp_funcs */ -static void tp_stub_func(void) -{ - return; -} - static inline void *allocate_probes(int count) { struct tp_probes *p = kmalloc(struct_size(p, probes, count), @@ -193,8 +187,8 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, if (old) { /* (N -> N+1), (N != 0, 1) probes */ for (iter_probes = 0; old[iter_probes].func; iter_probes++) { - if (old[iter_probes].func == tp_stub_func) - continue; /* Skip stub functions. */ + if (old[iter_probes].func == TRACEPOINT_FUNC_SKIP) + continue; if (old[iter_probes].func == tp_func->func && old[iter_probes].data == tp_func->data) return ERR_PTR(-EEXIST); @@ -208,7 +202,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func, if (old) { nr_probes = 0; for (iter_probes = 0; old[iter_probes].func; iter_probes++) { - if (old[iter_probes].func == tp_stub_func) + if (old[iter_probes].func == TRACEPOINT_FUNC_SKIP) continue; /* Insert before probes of lower priority */ if (pos < 0 && old[iter_probes].prio < prio) @@ -246,7 +240,7 @@ static void *func_remove(struct tracepoint_func **funcs, for (nr_probes = 0; old[nr_probes].func; nr_probes++) { if ((old[nr_probes].func == tp_func->func && old[nr_probes].data == tp_func->data) || - old[nr_probes].func == tp_stub_func) + old[nr_probes].func == TRACEPOINT_FUNC_SKIP) nr_del++; } } @@ -269,7 +263,7 @@ static void *func_remove(struct tracepoint_func **funcs, for (i = 0; old[i].func; i++) { if ((old[i].func != tp_func->func || old[i].data != tp_func->data) && - old[i].func != tp_stub_func) + old[i].func != TRACEPOINT_FUNC_SKIP) new[j++] = old[i]; } new[nr_probes - nr_del].func = NULL; @@ -277,12 +271,12 @@ static void *func_remove(struct tracepoint_func **funcs, } else { /* * Failed to allocate, replace the old function - * with calls to tp_stub_func. + * with TRACEPOINT_FUNC_SKIP. */ for (i = 0; old[i].func; i++) { if (old[i].func == tp_func->func && old[i].data == tp_func->data) - WRITE_ONCE(old[i].func, tp_stub_func); + WRITE_ONCE(old[i].func, TRACEPOINT_FUNC_SKIP); } *funcs = old; }