diff mbox series

[3/3] kprobes: Allow probing on any address belonging to ftrace

Message ID 78480d05821d45e09fb234f61f9037e26d42f02d.1645096227.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive)
State Not Applicable
Delegated to: BPF
Headers show
Series powerpc/ftrace: Reserve instructions from function entry for ftrace | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next success VM_Test
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Naveen N. Rao Feb. 17, 2022, 11:36 a.m. UTC
On certain architectures, ftrace can reserve multiple instructions at
function entry. Rather than rejecting kprobe on addresses other than the
exact ftrace call instruction, use the address returned by ftrace to
probe at the correct address when CONFIG_KPROBES_ON_FTRACE is enabled.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/kprobes.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Naveen N. Rao Feb. 18, 2022, 6:45 a.m. UTC | #1
Naveen N. Rao wrote:
> On certain architectures, ftrace can reserve multiple instructions at
> function entry. Rather than rejecting kprobe on addresses other than the
> exact ftrace call instruction, use the address returned by ftrace to
> probe at the correct address when CONFIG_KPROBES_ON_FTRACE is enabled.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/kprobes.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 94cab8c9ce56cc..0a797ede3fdf37 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1497,6 +1497,10 @@ bool within_kprobe_blacklist(unsigned long addr)
>  static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
>  			const char *symbol_name, unsigned int offset)
>  {
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +	unsigned long ftrace_addr = 0;
> +#endif
> +
>  	if ((symbol_name && addr) || (!symbol_name && !addr))
>  		goto invalid;
>  
> @@ -1507,6 +1511,14 @@ static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
>  	}
>  
>  	addr = (kprobe_opcode_t *)(((char *)addr) + offset);
> +
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +	if (addr)
> +		ftrace_addr = ftrace_location((unsigned long)addr);
> +	if (ftrace_addr)
> +		return (kprobe_opcode_t *)ftrace_addr;
> +#endif

One of the side effects of this is that we'll now allow probes on 
non-instruction boundary within the full ftrace address range. It's not 
too much of an issue since we ensure that the probe location eventually 
lands on the actual ftrace instruction. But, I'm wondering if we should 
instead allow architectures to opt-in to this, by making this be 
architecture specific. Architectures can then do whatever validation is 
necessary.


- Naveen
Masami Hiramatsu (Google) Feb. 21, 2022, 12:15 a.m. UTC | #2
On Thu, 17 Feb 2022 17:06:25 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On certain architectures, ftrace can reserve multiple instructions at
> function entry. Rather than rejecting kprobe on addresses other than the
> exact ftrace call instruction, use the address returned by ftrace to
> probe at the correct address when CONFIG_KPROBES_ON_FTRACE is enabled.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/kprobes.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 94cab8c9ce56cc..0a797ede3fdf37 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1497,6 +1497,10 @@ bool within_kprobe_blacklist(unsigned long addr)
>  static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
>  			const char *symbol_name, unsigned int offset)
>  {
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +	unsigned long ftrace_addr = 0;
> +#endif
> +
>  	if ((symbol_name && addr) || (!symbol_name && !addr))
>  		goto invalid;
>  
> @@ -1507,6 +1511,14 @@ static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
>  	}
>  
>  	addr = (kprobe_opcode_t *)(((char *)addr) + offset);
> +
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +	if (addr)
> +		ftrace_addr = ftrace_location((unsigned long)addr);
> +	if (ftrace_addr)
> +		return (kprobe_opcode_t *)ftrace_addr;

As I said, this must be

if (ftrace_addr != addr)
	return -EILSEQ;

This will prevent users from being confused by the results of probing
that 'func' and 'func+4' are the same. (now only 'func' is allowed to
be probed.)

Thank you,

> +#endif
> +
>  	if (addr)
>  		return addr;
>  
> -- 
> 2.35.1
>
diff mbox series

Patch

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 94cab8c9ce56cc..0a797ede3fdf37 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1497,6 +1497,10 @@  bool within_kprobe_blacklist(unsigned long addr)
 static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
 			const char *symbol_name, unsigned int offset)
 {
+#ifdef CONFIG_KPROBES_ON_FTRACE
+	unsigned long ftrace_addr = 0;
+#endif
+
 	if ((symbol_name && addr) || (!symbol_name && !addr))
 		goto invalid;
 
@@ -1507,6 +1511,14 @@  static kprobe_opcode_t *_kprobe_addr(kprobe_opcode_t *addr,
 	}
 
 	addr = (kprobe_opcode_t *)(((char *)addr) + offset);
+
+#ifdef CONFIG_KPROBES_ON_FTRACE
+	if (addr)
+		ftrace_addr = ftrace_location((unsigned long)addr);
+	if (ftrace_addr)
+		return (kprobe_opcode_t *)ftrace_addr;
+#endif
+
 	if (addr)
 		return addr;