diff mbox series

[5/8] ftrace: Make DIRECT_CALLS work WITH_ARGS and !WITH_REGS

Message ID 20230201163420.1579014-6-revest@chromium.org
State New
Headers
Series Add ftrace direct call for arm64 |

Commit Message

Florent Revest Feb. 1, 2023, 4:34 p.m. UTC 
  Direct called trampolines can be called in two ways:
- either from the ftrace callsite. In this case, they do not access any
  struct ftrace_regs nor pt_regs
- Or, if a ftrace ops is also attached, from the end of a ftrace
  trampoline. In this case, the call_direct_funcs ops is in charge of
  setting the direct call trampoline's address in a struct ftrace_regs

Since "ftrace: pass fregs to arch_ftrace_set_direct_caller()", the later
case no longer requires a full pt_regs. It only needs a struct
ftrace_regs so DIRECT_CALLS can work with both WITH_ARGS or WITH_REGS.
With architectures like arm64 already abandoning WITH_REGS in favor of
WITH_ARGS, it's important to have DIRECT_CALLS work WITH_ARGS only.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 kernel/trace/Kconfig  | 2 +-
 kernel/trace/ftrace.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Mark Rutland Feb. 2, 2023, 3:54 p.m. UTC | #1 
On Wed, Feb 01, 2023 at 05:34:17PM +0100, Florent Revest wrote:
> Direct called trampolines can be called in two ways:
> - either from the ftrace callsite. In this case, they do not access any
>   struct ftrace_regs nor pt_regs
> - Or, if a ftrace ops is also attached, from the end of a ftrace
>   trampoline. In this case, the call_direct_funcs ops is in charge of
>   setting the direct call trampoline's address in a struct ftrace_regs
> 
> Since "ftrace: pass fregs to arch_ftrace_set_direct_caller()", the later
> case no longer requires a full pt_regs.

Minor nit, but could we please have the commit ID, e.g.

| Since commit:
| 
|   9705bc70960459ae ("ftrace: pass fregs to arch_ftrace_set_direct_caller()")
| 
| The latter case no longer requires a full pt_regs.

> It only needs a struct
> ftrace_regs so DIRECT_CALLS can work with both WITH_ARGS or WITH_REGS.
> With architectures like arm64 already abandoning WITH_REGS in favor of
> WITH_ARGS, it's important to have DIRECT_CALLS work WITH_ARGS only.
> 
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  kernel/trace/Kconfig  | 2 +-
>  kernel/trace/ftrace.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 5df427a2321d..4496a7c69810 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -257,7 +257,7 @@ config DYNAMIC_FTRACE_WITH_REGS
>  
>  config DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>  	def_bool y
> -	depends on DYNAMIC_FTRACE_WITH_REGS
> +	depends on DYNAMIC_FTRACE_WITH_REGS || DYNAMIC_FTRACE_WITH_ARGS
>  	depends on HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>  
>  config DYNAMIC_FTRACE_WITH_CALL_OPS
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b0426de11c45..73b6f6489ba1 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5282,7 +5282,7 @@ static LIST_HEAD(ftrace_direct_funcs);
>  
>  static int register_ftrace_function_nolock(struct ftrace_ops *ops);
>  
> -#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS)
> +#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT)

Unfortunately, I think this is broken for architectures where:

* DYNAMIC_FTRACE_WITH_DIRECT_CALLS=y
* DYNAMIC_FTRACE_WITH_REGS=y
* DYNAMIC_FTRACE_WITH_ARGS=n

... since those might pass a NULL ftrace_regs around, and so when using the
list ops arch_ftrace_set_direct_caller() might blow up accessing an element of
ftrace_regs.

It looks like 32-bit x86 is the only case with that combination, and its
ftrace_caller implementation passes a NULL regs, so I reckon that'll blow up.
However, it looks like there aren't any FTRACE_DIRECT samples wired up for
32-bit x86, so I'm not aware of a test case we can use.

We could make the FTRACE_OPS_FL_SAVE_REGS flag conditional, or we could
implement DYNAMIC_FTRACE_WITH_ARGS for 32-bit x86 and have
DYNAMIC_FTRACE_WITH_DIRECT_CALLS depend solely on that.

Thanks,
Mark.
Mark Rutland Feb. 2, 2023, 4:56 p.m. UTC | #2 
On Thu, Feb 02, 2023 at 03:54:33PM +0000, Mark Rutland wrote:
> On Wed, Feb 01, 2023 at 05:34:17PM +0100, Florent Revest wrote:
> > -#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS)
> > +#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT)
> 
> Unfortunately, I think this is broken for architectures where:
> 
> * DYNAMIC_FTRACE_WITH_DIRECT_CALLS=y
> * DYNAMIC_FTRACE_WITH_REGS=y
> * DYNAMIC_FTRACE_WITH_ARGS=n
> 
> ... since those might pass a NULL ftrace_regs around, and so when using the
> list ops arch_ftrace_set_direct_caller() might blow up accessing an element of
> ftrace_regs.
> 
> It looks like 32-bit x86 is the only case with that combination, and its
> ftrace_caller implementation passes a NULL regs, so I reckon that'll blow up.
> However, it looks like there aren't any FTRACE_DIRECT samples wired up for
> 32-bit x86, so I'm not aware of a test case we can use.

FWIW, the FTRACE_STARTUP_TEST tickles this:

[    1.896209] Testing tracer function_graph: 
[    2.900282] BUG: kernel NULL pointer dereference, address: 0000002c
[    2.901171] #PF: supervisor write access in kernel mode
[    2.901171] #PF: error_code(0x0002) - not-present page
[    2.901171] *pde = 00000000 
[    2.901171] Oops: 0002 [#1] PREEMPT SMP
[    2.901171] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3-00014-gcfd6340c71ce #1
[    2.901171] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
[    2.901171] EIP: call_direct_funcs+0xd/0x1c
[    2.901171] Code: 00 00 00 00 90 a9 00 00 00 01 0f 84 d7 fe ff ff 0d 00 00 80 00 89 46 04 e9 d2 fe ff ff 8b 41 64 85 c0 74 11 55 89 e5 8b 55 08 <89> 42 2c 5d c3 8d b6 00 00 00 00 c3 8d 76 00 89 c1 89 b
[    2.901171] EAX: cc3620e8 EBX: c1147e44 ECX: c1147e44 EDX: 00000000
[    2.901171] ESI: fffffeff EDI: cc354208 EBP: c1147dbc ESP: c1147dbc
[    2.901171] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
[    2.901171] CR0: 80050033 CR2: 0000002c CR3: 0d703000 CR4: 00350ed0
[    2.901171] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[    2.901171] DR6: fffe0ff0 DR7: 00000400
[    2.901171] Call Trace:
[    2.901171]  arch_ftrace_ops_list_func+0xf5/0x1bc
[    2.901171]  ? ftrace_enable_ftrace_graph_caller+0x3b/0x44
[    2.901171]  ? trace_selftest_startup_function_graph+0x1d9/0x298
[    2.901171]  ? syscall_unregfunc+0xa0/0xa0
[    2.901171]  ftrace_call+0x5/0x13
[    2.901171]  trace_selftest_dynamic_test_func+0x5/0xc
[    2.901171]  trace_selftest_startup_function_graph+0x1d9/0x298
[    2.901171]  ? trace_selftest_dynamic_test_func+0x5/0xc
[    2.901171]  ? trace_selftest_startup_function_graph+0x1d9/0x298
[    2.901171]  ? ftrace_check_record+0x340/0x340
[    2.901171]  ? ftrace_check_record+0x340/0x340
[    2.901171]  ? ftrace_stub_graph+0x4/0x4
[    2.901171]  ? trace_selftest_test_regs_func+0x18/0x18
[    2.901171]  run_tracer_selftest+0x7d/0x1bc
[    2.901171]  ? graph_depth_read+0x90/0x90
[    2.901171]  register_tracer+0xd3/0x284
[    2.901171]  ? register_trace_event+0xf6/0x180
[    2.901171]  ? init_graph_tracefs+0x38/0x38
[    2.901171]  init_graph_trace+0x56/0x78
[    2.901171]  do_one_initcall+0x53/0x204
[    2.901171]  ? parse_args+0x143/0x3ec
[    2.901171]  ? __kmem_cache_alloc_node+0x2d/0x224
[    2.901171]  kernel_init_freeable+0x198/0x2bc
[    2.901171]  ? rdinit_setup+0x30/0x30
[    2.901171]  ? rest_init+0xb0/0xb0
[    2.901171]  kernel_init+0x1a/0x1d0
[    2.901171]  ? schedule_tail_wrapper+0x9/0xc
[    2.901171]  ret_from_fork+0x1c/0x28
[    2.901171] Modules linked in:
[    2.901171] CR2: 000000000000002c
[    2.901171] ---[ end trace 0000000000000000 ]---
[    2.901171] EIP: call_direct_funcs+0xd/0x1c
[    2.901171] Code: 00 00 00 00 90 a9 00 00 00 01 0f 84 d7 fe ff ff 0d 00 00 80 00 89 46 04 e9 d2 fe ff ff 8b 41 64 85 c0 74 11 55 89 e5 8b 55 08 <89> 42 2c 5d c3 8d b6 00 00 00 00 c3 8d 76 00 89 c1 89 b
[    2.901171] EAX: cc3620e8 EBX: c1147e44 ECX: c1147e44 EDX: 00000000
[    2.901171] ESI: fffffeff EDI: cc354208 EBP: c1147dbc ESP: c1147dbc
[    2.901171] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
[    2.901171] CR0: 80050033 CR2: 0000002c CR3: 0d703000 CR4: 00350ed0
[    2.901171] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[    2.901171] DR6: fffe0ff0 DR7: 00000400
[    2.901171] note: swapper/0[1] exited with preempt_count 1
[    2.901175] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
[    2.902171] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---

The below diff solved that for me.

Thanks,
Mark.

---->8----
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 84f717f8959e..3d2156e335d7 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -241,6 +241,12 @@ enum {
        FTRACE_OPS_FL_DIRECT                    = BIT(17),
 };
 
+#ifndef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+#define FTRACE_OPS_FL_SAVE_ARGS                        FTRACE_OPS_FL_SAVE_REGS
+#else
+#define FTRACE_OPS_FL_SAVE_ARGS                        0
+#endif
+
 /*
  * FTRACE_OPS_CMD_* commands allow the ftrace core logic to request changes
  * to a ftrace_ops. Note, the requests may fail.
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 73b6f6489ba1..8e739303b6a2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5282,7 +5282,7 @@ static LIST_HEAD(ftrace_direct_funcs);
 
 static int register_ftrace_function_nolock(struct ftrace_ops *ops);
 
-#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT)
+#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_ARGS)
 
 static int check_direct_multi(struct ftrace_ops *ops)
 {
Florent Revest Feb. 2, 2023, 6:18 p.m. UTC | #3 
On Thu, Feb 2, 2023 at 4:54 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Feb 01, 2023 at 05:34:17PM +0100, Florent Revest wrote:
> > Direct called trampolines can be called in two ways:
> > - either from the ftrace callsite. In this case, they do not access any
> >   struct ftrace_regs nor pt_regs
> > - Or, if a ftrace ops is also attached, from the end of a ftrace
> >   trampoline. In this case, the call_direct_funcs ops is in charge of
> >   setting the direct call trampoline's address in a struct ftrace_regs
> >
> > Since "ftrace: pass fregs to arch_ftrace_set_direct_caller()", the later
> > case no longer requires a full pt_regs.
>
> Minor nit, but could we please have the commit ID, e.g.
>
> | Since commit:
> |
> |   9705bc70960459ae ("ftrace: pass fregs to arch_ftrace_set_direct_caller()")
> |
> | The latter case no longer requires a full pt_regs.

Sure thing, will do!
Florent Revest Feb. 2, 2023, 6:19 p.m. UTC | #4 
On Thu, Feb 2, 2023 at 5:57 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Feb 02, 2023 at 03:54:33PM +0000, Mark Rutland wrote:
> > On Wed, Feb 01, 2023 at 05:34:17PM +0100, Florent Revest wrote:
> > > -#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS)
> > > +#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT)
> >
> > Unfortunately, I think this is broken for architectures where:
> >
> > * DYNAMIC_FTRACE_WITH_DIRECT_CALLS=y
> > * DYNAMIC_FTRACE_WITH_REGS=y
> > * DYNAMIC_FTRACE_WITH_ARGS=n
> >
> > ... since those might pass a NULL ftrace_regs around, and so when using the
> > list ops arch_ftrace_set_direct_caller() might blow up accessing an element of
> > ftrace_regs.
> >
> > It looks like 32-bit x86 is the only case with that combination, and its
> > ftrace_caller implementation passes a NULL regs, so I reckon that'll blow up.
> > However, it looks like there aren't any FTRACE_DIRECT samples wired up for
> > 32-bit x86, so I'm not aware of a test case we can use.
>
> FWIW, the FTRACE_STARTUP_TEST tickles this:

Good catch and thanks for reproducing the bug too!

> [    1.896209] Testing tracer function_graph:
> [    2.900282] BUG: kernel NULL pointer dereference, address: 0000002c
> [    2.901171] #PF: supervisor write access in kernel mode
> [    2.901171] #PF: error_code(0x0002) - not-present page
> [    2.901171] *pde = 00000000
> [    2.901171] Oops: 0002 [#1] PREEMPT SMP
> [    2.901171] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3-00014-gcfd6340c71ce #1
> [    2.901171] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
> [    2.901171] EIP: call_direct_funcs+0xd/0x1c
> [    2.901171] Code: 00 00 00 00 90 a9 00 00 00 01 0f 84 d7 fe ff ff 0d 00 00 80 00 89 46 04 e9 d2 fe ff ff 8b 41 64 85 c0 74 11 55 89 e5 8b 55 08 <89> 42 2c 5d c3 8d b6 00 00 00 00 c3 8d 76 00 89 c1 89 b
> [    2.901171] EAX: cc3620e8 EBX: c1147e44 ECX: c1147e44 EDX: 00000000
> [    2.901171] ESI: fffffeff EDI: cc354208 EBP: c1147dbc ESP: c1147dbc
> [    2.901171] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
> [    2.901171] CR0: 80050033 CR2: 0000002c CR3: 0d703000 CR4: 00350ed0
> [    2.901171] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [    2.901171] DR6: fffe0ff0 DR7: 00000400
> [    2.901171] Call Trace:
> [    2.901171]  arch_ftrace_ops_list_func+0xf5/0x1bc
> [    2.901171]  ? ftrace_enable_ftrace_graph_caller+0x3b/0x44
> [    2.901171]  ? trace_selftest_startup_function_graph+0x1d9/0x298
> [    2.901171]  ? syscall_unregfunc+0xa0/0xa0
> [    2.901171]  ftrace_call+0x5/0x13
> [    2.901171]  trace_selftest_dynamic_test_func+0x5/0xc
> [    2.901171]  trace_selftest_startup_function_graph+0x1d9/0x298
> [    2.901171]  ? trace_selftest_dynamic_test_func+0x5/0xc
> [    2.901171]  ? trace_selftest_startup_function_graph+0x1d9/0x298
> [    2.901171]  ? ftrace_check_record+0x340/0x340
> [    2.901171]  ? ftrace_check_record+0x340/0x340
> [    2.901171]  ? ftrace_stub_graph+0x4/0x4
> [    2.901171]  ? trace_selftest_test_regs_func+0x18/0x18
> [    2.901171]  run_tracer_selftest+0x7d/0x1bc
> [    2.901171]  ? graph_depth_read+0x90/0x90
> [    2.901171]  register_tracer+0xd3/0x284
> [    2.901171]  ? register_trace_event+0xf6/0x180
> [    2.901171]  ? init_graph_tracefs+0x38/0x38
> [    2.901171]  init_graph_trace+0x56/0x78
> [    2.901171]  do_one_initcall+0x53/0x204
> [    2.901171]  ? parse_args+0x143/0x3ec
> [    2.901171]  ? __kmem_cache_alloc_node+0x2d/0x224
> [    2.901171]  kernel_init_freeable+0x198/0x2bc
> [    2.901171]  ? rdinit_setup+0x30/0x30
> [    2.901171]  ? rest_init+0xb0/0xb0
> [    2.901171]  kernel_init+0x1a/0x1d0
> [    2.901171]  ? schedule_tail_wrapper+0x9/0xc
> [    2.901171]  ret_from_fork+0x1c/0x28
> [    2.901171] Modules linked in:
> [    2.901171] CR2: 000000000000002c
> [    2.901171] ---[ end trace 0000000000000000 ]---
> [    2.901171] EIP: call_direct_funcs+0xd/0x1c
> [    2.901171] Code: 00 00 00 00 90 a9 00 00 00 01 0f 84 d7 fe ff ff 0d 00 00 80 00 89 46 04 e9 d2 fe ff ff 8b 41 64 85 c0 74 11 55 89 e5 8b 55 08 <89> 42 2c 5d c3 8d b6 00 00 00 00 c3 8d 76 00 89 c1 89 b
> [    2.901171] EAX: cc3620e8 EBX: c1147e44 ECX: c1147e44 EDX: 00000000
> [    2.901171] ESI: fffffeff EDI: cc354208 EBP: c1147dbc ESP: c1147dbc
> [    2.901171] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
> [    2.901171] CR0: 80050033 CR2: 0000002c CR3: 0d703000 CR4: 00350ed0
> [    2.901171] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [    2.901171] DR6: fffe0ff0 DR7: 00000400
> [    2.901171] note: swapper/0[1] exited with preempt_count 1
> [    2.901175] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
> [    2.902171] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---
>
> The below diff solved that for me.
>
> Thanks,
> Mark.
>
> ---->8----
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 84f717f8959e..3d2156e335d7 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -241,6 +241,12 @@ enum {
>         FTRACE_OPS_FL_DIRECT                    = BIT(17),
>  };
>
> +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> +#define FTRACE_OPS_FL_SAVE_ARGS                        FTRACE_OPS_FL_SAVE_REGS
> +#else
> +#define FTRACE_OPS_FL_SAVE_ARGS                        0

Mh, could we (theoretically) be in a situation where an arch supports
WITH_ARGS but it also has two ftrace_caller trampolines: one that
saves the args and the other that saves nothing ? (For example if x86
migrates their WITH_REGS to WITH_ARGS only)
Wouldn't it make sense then to define FTRACE_OPS_FL_SAVE_ARGS as an
extra bit to tell ftrace that we need the args, similarly to
FTRACE_OPS_FL_SAVE_REGS ?

If that can't happen or if we want to leave this up for later, the
patch lgtm and I can squash it into my patch 5 in v2. ;)

> +#endif
> +
>  /*
>   * FTRACE_OPS_CMD_* commands allow the ftrace core logic to request changes
>   * to a ftrace_ops. Note, the requests may fail.
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 73b6f6489ba1..8e739303b6a2 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5282,7 +5282,7 @@ static LIST_HEAD(ftrace_direct_funcs);
>
>  static int register_ftrace_function_nolock(struct ftrace_ops *ops);
>
> -#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT)
> +#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_ARGS)
>
>  static int check_direct_multi(struct ftrace_ops *ops)
>  {
>
Mark Rutland Feb. 3, 2023, 10:03 a.m. UTC | #5 
On Thu, Feb 02, 2023 at 07:19:58PM +0100, Florent Revest wrote:
> On Thu, Feb 2, 2023 at 5:57 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 84f717f8959e..3d2156e335d7 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -241,6 +241,12 @@ enum {
> >         FTRACE_OPS_FL_DIRECT                    = BIT(17),
> >  };
> >
> > +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > +#define FTRACE_OPS_FL_SAVE_ARGS                        FTRACE_OPS_FL_SAVE_REGS
> > +#else
> > +#define FTRACE_OPS_FL_SAVE_ARGS                        0
> 
> Mh, could we (theoretically) be in a situation where an arch supports
> WITH_ARGS but it also has two ftrace_caller trampolines: one that
> saves the args and the other that saves nothing ? (For example if x86
> migrates their WITH_REGS to WITH_ARGS only)

I don't think so -- the point of WITH_ARGS is that we always have to
save/restore the args, and when WITH_ARGS is selected they're unconditionally
available (though not necessarily a full pt_regs), which is what other code
assumes when WITH_ARGS is selected.

> Wouldn't it make sense then to define FTRACE_OPS_FL_SAVE_ARGS as an
> extra bit to tell ftrace that we need the args, similarly to
> FTRACE_OPS_FL_SAVE_REGS ?
> 
> If that can't happen or if we want to leave this up for later, the
> patch lgtm and I can squash it into my patch 5 in v2. ;)

I think that can't happen, and for now the above should be fine.

Thanks,
Mark.
Florent Revest Feb. 3, 2023, 11:01 a.m. UTC | #6 
On Fri, Feb 3, 2023 at 11:03 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Feb 02, 2023 at 07:19:58PM +0100, Florent Revest wrote:
> > On Thu, Feb 2, 2023 at 5:57 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index 84f717f8959e..3d2156e335d7 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -241,6 +241,12 @@ enum {
> > >         FTRACE_OPS_FL_DIRECT                    = BIT(17),
> > >  };
> > >
> > > +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > > +#define FTRACE_OPS_FL_SAVE_ARGS                        FTRACE_OPS_FL_SAVE_REGS
> > > +#else
> > > +#define FTRACE_OPS_FL_SAVE_ARGS                        0
> >
> > Mh, could we (theoretically) be in a situation where an arch supports
> > WITH_ARGS but it also has two ftrace_caller trampolines: one that
> > saves the args and the other that saves nothing ? (For example if x86
> > migrates their WITH_REGS to WITH_ARGS only)
>
> I don't think so -- the point of WITH_ARGS is that we always have to
> save/restore the args, and when WITH_ARGS is selected they're unconditionally
> available (though not necessarily a full pt_regs), which is what other code
> assumes when WITH_ARGS is selected.

Perfect then!

> > Wouldn't it make sense then to define FTRACE_OPS_FL_SAVE_ARGS as an
> > extra bit to tell ftrace that we need the args, similarly to
> > FTRACE_OPS_FL_SAVE_REGS ?
> >
> > If that can't happen or if we want to leave this up for later, the
> > patch lgtm and I can squash it into my patch 5 in v2. ;)
>
> I think that can't happen, and for now the above should be fine.

Yep
diff mbox series

Patch

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 5df427a2321d..4496a7c69810 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -257,7 +257,7 @@  config DYNAMIC_FTRACE_WITH_REGS
 
 config DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	def_bool y
-	depends on DYNAMIC_FTRACE_WITH_REGS
+	depends on DYNAMIC_FTRACE_WITH_REGS || DYNAMIC_FTRACE_WITH_ARGS
 	depends on HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 
 config DYNAMIC_FTRACE_WITH_CALL_OPS
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b0426de11c45..73b6f6489ba1 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5282,7 +5282,7 @@  static LIST_HEAD(ftrace_direct_funcs);
 
 static int register_ftrace_function_nolock(struct ftrace_ops *ops);
 
-#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS)
+#define MULTI_FLAGS (FTRACE_OPS_FL_DIRECT)
 
 static int check_direct_multi(struct ftrace_ops *ops)
 {