Re: [PATCH v2 12/39] x86/ibt,ftrace: Search for __fentry__ location

[Masami Hiramatsu]

On Thu, 3 Mar 2022 16:59:03 +0100
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Thu, Mar 03, 2022 at 09:34:13AM -0500, Steven Rostedt wrote:
> > On Thu, 3 Mar 2022 14:04:52 +0100
> > Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > 
> > > > @@ -1596,7 +1596,7 @@ static int check_ftrace_location(struct kprobe *p)
> > > > {
> > > > 	unsigned long ftrace_addr;
> > > > 
> > > > -	ftrace_addr = ftrace_location((unsigned long)p->addr);
> > > > +	ftrace_addr = ftrace_location_range((unsigned long)p->addr, (unsigned long)p->addr);  
> > > 
> > > Yes, although perhaps a new helper. I'll go ponder during lunch.
> > 
> > Is there more places to add that to make it worth creating a helper?
> 
> This is what I ended up with, I've looked at all ftrace_location() sites
> there are, seems to work too, both the built-in boot time ftrace tests
> and the selftests work splat-less.
> 
> I should update the Changelog some though.
> 
> Naveen also mentioned register_ftrace_direct() could be further cleaned
> up, but I didn't want to do too much in once go.
> 
> ---
> 
> Subject: x86/ibt,ftrace: Search for __fentry__ location
> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Date: Wed Feb 23 10:01:38 CET 2022
> 
> Have ftrace_location() search the symbol for the __fentry__ location
> when it isn't at func+0 and use this for {,un}register_ftrace_direct().
> 
> This avoids a whole bunch of assumptions about __fentry__ being at
> func+0.
> 
> Suggested-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/kprobes/core.c |   11 +---------
>  kernel/bpf/trampoline.c        |   20 +++----------------
>  kernel/kprobes.c               |    8 +------
>  kernel/trace/ftrace.c          |   43 +++++++++++++++++++++++++++++++++--------
>  4 files changed, 43 insertions(+), 39 deletions(-)
> 
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -193,17 +193,10 @@ static unsigned long
>  __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
>  {
>  	struct kprobe *kp;
> -	unsigned long faddr;
> +	bool faddr;
>  
>  	kp = get_kprobe((void *)addr);
> -	faddr = ftrace_location(addr);
> -	/*
> -	 * Addresses inside the ftrace location are refused by
> -	 * arch_check_ftrace_location(). Something went terribly wrong
> -	 * if such an address is checked here.
> -	 */
> -	if (WARN_ON(faddr && faddr != addr))
> -		return 0UL;
> +	faddr = ftrace_location(addr) == addr;

OK, this looks good to me. 

>  	/*
>  	 * Use the current code if it is not modified by Kprobe
>  	 * and it cannot be modified by ftrace.
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -117,18 +117,6 @@ static void bpf_trampoline_module_put(st
>  	tr->mod = NULL;
>  }
>  
> -static int is_ftrace_location(void *ip)
> -{
> -	long addr;
> -
> -	addr = ftrace_location((long)ip);
> -	if (!addr)
> -		return 0;
> -	if (WARN_ON_ONCE(addr != (long)ip))
> -		return -EFAULT;
> -	return 1;
> -}
> -
>  static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
>  {
>  	void *ip = tr->func.addr;
> @@ -160,12 +148,12 @@ static int modify_fentry(struct bpf_tram
>  static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
>  {
>  	void *ip = tr->func.addr;
> +	unsigned long faddr;
>  	int ret;
>  
> -	ret = is_ftrace_location(ip);
> -	if (ret < 0)
> -		return ret;
> -	tr->func.ftrace_managed = ret;
> +	faddr = ftrace_location((unsigned long)ip);
> +	if (faddr)
> +		tr->func.ftrace_managed = true;
>  
>  	if (bpf_trampoline_module_get(tr))
>  		return -ENOENT;
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1562,14 +1562,10 @@ static inline int warn_kprobe_rereg(stru
>  
>  static int check_ftrace_location(struct kprobe *p)
>  {
> -	unsigned long ftrace_addr;
> +	unsigned long addr = (unsigned long)p->addr;
>  
> -	ftrace_addr = ftrace_location((unsigned long)p->addr);
> -	if (ftrace_addr) {
> +	if (ftrace_location(addr) == addr) {
>  #ifdef CONFIG_KPROBES_ON_FTRACE
> -		/* Given address is not on the instruction boundary */
> -		if ((unsigned long)p->addr != ftrace_addr)
> -			return -EILSEQ;

OK, so this means we only use the ftrace if the kprobe puts the
sym+ftrace-offset. Thus if there is ENDBR at the first instruction,
kprobe will use int3, right?
I agree with this, but later I have to add another patch to use ftrace
for the kprobes on symbol+0. But anyway, that is another issue.

So this looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Thank you!

>  		p->flags |= KPROBE_FLAG_FTRACE;
>  #else	/* !CONFIG_KPROBES_ON_FTRACE */
>  		return -EINVAL;
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1568,17 +1568,34 @@ unsigned long ftrace_location_range(unsi
>  }
>  
>  /**
> - * ftrace_location - return true if the ip giving is a traced location
> + * ftrace_location - return the ftrace location
>   * @ip: the instruction pointer to check
>   *
> - * Returns rec->ip if @ip given is a pointer to a ftrace location.
> - * That is, the instruction that is either a NOP or call to
> - * the function tracer. It checks the ftrace internal tables to
> - * determine if the address belongs or not.
> + * If @ip matches the ftrace location, return @ip.
> + * If @ip matches sym+0, return sym's ftrace location.
> + * Otherwise, return 0.
>   */
>  unsigned long ftrace_location(unsigned long ip)
>  {
> -	return ftrace_location_range(ip, ip);
> +	struct dyn_ftrace *rec;
> +	unsigned long offset;
> +	unsigned long size;
> +
> +	rec = lookup_rec(ip, ip);
> +	if (!rec) {
> +		if (!kallsyms_lookup_size_offset(ip, &size, &offset))
> +			goto out;
> +
> +		/* map sym+0 to __fentry__ */
> +		if (!offset)
> +			rec = lookup_rec(ip, ip + size - 1);
> +	}
> +
> +	if (rec)
> +		return rec->ip;
> +
> +out:
> +	return 0;
>  }
>  
>  /**
> @@ -4962,7 +4979,8 @@ ftrace_match_addr(struct ftrace_hash *ha
>  {
>  	struct ftrace_func_entry *entry;
>  
> -	if (!ftrace_location(ip))
> +	ip = ftrace_location(ip);
> +	if (!ip)
>  		return -EINVAL;
>  
>  	if (remove) {
> @@ -5110,11 +5128,16 @@ int register_ftrace_direct(unsigned long
>  	struct ftrace_func_entry *entry;
>  	struct ftrace_hash *free_hash = NULL;
>  	struct dyn_ftrace *rec;
> -	int ret = -EBUSY;
> +	int ret = -ENODEV;
>  
>  	mutex_lock(&direct_mutex);
>  
> +	ip = ftrace_location(ip);
> +	if (!ip)
> +		goto out_unlock;
> +
>  	/* See if there's a direct function at @ip already */
> +	ret = -EBUSY;
>  	if (ftrace_find_rec_direct(ip))
>  		goto out_unlock;
>  
> @@ -5222,6 +5245,10 @@ int unregister_ftrace_direct(unsigned lo
>  
>  	mutex_lock(&direct_mutex);
>  
> +	ip = ftrace_location(ip);
> +	if (!ip)
> +		goto out_unlock;
> +
>  	entry = find_direct_entry(&ip, NULL);
>  	if (!entry)
>  		goto out_unlock;


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>