Re: [PATCH -tip v2 2/3] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(2014/06/20 11:48), Steven Rostedt wrote:
> On Tue, 17 Jun 2014 11:04:49 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> Introduce FTRACE_OPS_FL_IPMODIFY to avoid conflict among
>> ftrace users who may modify regs->ip to change the execution
>> path. This also adds the flag to kprobe_ftrace_ops, since
>> ftrace-based kprobes already modifies regs->ip. Thus, if
>> another user modifies the regs->ip on the same function entry,
>> one of them will be broken. So both should add IPMODIFY flag
>> and make sure that ftrace_set_filter_ip() succeeds.
>>
>> Note that currently conflicts of IPMODIFY are detected on the
>> filter hash. It does NOT care about the notrace hash. This means
>> that if you set filter hash all functions and notrace(mask)
>> some of them, the IPMODIFY flag will be applied to all
>> functions.
> 
> Hmm, this worries me. I'm not sure I care about ignoring the notrace
> hash.

Since the notrace hash is not updated atomically (it is disabled ->
updated -> enabled), there could be a small window where only filter
hash is enabled.　I considered that.


[...]
>> @@ -317,13 +322,14 @@ extern int ftrace_nr_registered_ops(void);
>>   * from tracing that function.
>>   */
>>  enum {
>> +	FTRACE_FL_IPMODIFY	= (1UL << 28),
>>  	FTRACE_FL_ENABLED	= (1UL << 29),
>>  	FTRACE_FL_REGS		= (1UL << 30),
>>  	FTRACE_FL_REGS_EN	= (1UL << 31)
>>  };
>>  
>> -#define FTRACE_FL_MASK		(0x7UL << 29)
>> -#define FTRACE_REF_MAX		((1UL << 29) - 1)
>> +#define FTRACE_FL_MASK		(0xfUL << 28)
>> +#define FTRACE_REF_MAX		((1UL << 28) - 1)
> 
> Note, this is going to conflict with my queue for 3.17, as I'm starting
> to write individual trampolines.
> 
> You can take a look at my ftrace/next branch, but be warned, it will
> rebase.

OK, anyway, at least this flag can easily be changed. :)


>>  struct dyn_ftrace {
>>  	unsigned long		ip; /* address of mcount call-site */
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 3214289..e52d86f 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -915,7 +915,7 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
>>  #ifdef CONFIG_KPROBES_ON_FTRACE
>>  static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
>>  	.func = kprobe_ftrace_handler,
>> -	.flags = FTRACE_OPS_FL_SAVE_REGS,
>> +	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
> 
> We probably should comment somewhere that once you set the IPMODIFY
> flag (or do not set it), it should never change. An ops either has it
> or it doesn't, it can't change its mind. Otherwise it could play havoc
> with the update code below.

Agreed. Hmm, it seems that we currently have no document about ftrace_ops.
This is a kind of undocumented kmodule API. At least we need to add a comment
on the header.

[...]
>> @@ -1593,6 +1607,109 @@ static void ftrace_hash_rec_enable(struct ftrace_ops *ops,
>>  	__ftrace_hash_rec_update(ops, filter_hash, 1);
>>  }
>>  
>> +/*
>> + * Try to update IPMODIFY flag on each ftrace_rec. Return 0 if it is OK
>> + * or no-needed to update, -EBUSY if it detects a conflict of the flag
>> + * on a ftrace_rec.
>> + * Note that old_hash and new_hash has below meanings
>> + *  - If the hash is NULL, it hits all recs
>> + *  - If the hash is EMPTY_HASH, it hits nothing
>> + *  - Others hits the recs which match the hash entries.
> 
>  - Anything else hits the recs ...

Oh, thanks!

> 
>> + */
>> +static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
>> +					 struct ftrace_hash *old_hash,
>> +					 struct ftrace_hash *new_hash)
>> +{
>> +	struct ftrace_page *pg;
>> +	struct dyn_ftrace *rec, *end = NULL;
>> +	int in_old, in_new;
>> +
>> +	/* Only update if the ops has been registered */
>> +	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
>> +		return 0;
>> +
>> +	if (!(ops->flags & FTRACE_OPS_FL_SAVE_REGS) ||
>> +	    !(ops->flags & FTRACE_OPS_FL_IPMODIFY))
> 
> Only check the IPMODIFY flag. In the future, I may allow this without
> SAVE_REGS. That is, the function will get a regs pointer that has a
> limited number of regs set. Maybe just ip.

Ah, I see. I'll change that.

>> +		return 0;
>> +
>> +	/* Update rec->flags */
>> +	do_for_each_ftrace_rec(pg, rec) {
>> +		/* We need to update only differences of filter_hash */
>> +		in_old = !old_hash || ftrace_lookup_ip(old_hash, rec->ip);
>> +		in_new = !new_hash || ftrace_lookup_ip(new_hash, rec->ip);
>> +		if (in_old == in_new)
>> +			continue;
>> +
>> +		if (in_new) {
>> +			/* New entries must ensure no others are using it */
>> +			if (rec->flags & FTRACE_FL_IPMODIFY)
>> +				goto rollback;
>> +			rec->flags |= FTRACE_FL_IPMODIFY;
>> +		} else /* Removed entry */
>> +			rec->flags &= ~FTRACE_FL_IPMODIFY;
> 
> Like I mentioned before. This code depends on oldhash also having
> IPMODIFY set. I wonder if we can detect if it changed.

Yeah, I guess that SAVE_REGS flag has same issue. And, of course
kprobe->flags has same. I think we just need comments or a document
for the ftrace_ops API.

[...]
>> @@ -2078,6 +2195,15 @@ static int ftrace_startup(struct ftrace_ops *ops, int command)
>>  
>>  	ops->flags |= FTRACE_OPS_FL_ENABLED;
>>  
>> +	ret = ftrace_hash_ipmodify_enable(ops);
>> +	if (ret < 0) {
>> +		/* Rollback registration process */
>> +		__unregister_ftrace_function(ops);
>> +		ftrace_start_up--;
>> +		ops->flags &= ~FTRACE_OPS_FL_ENABLED;
>> +		return ret;
>> +	}
>> +
>>  	ftrace_hash_rec_enable(ops, 1);
>>  
>>  	ftrace_startup_enable(command);
>> @@ -2104,6 +2230,8 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
>>  	 */
>>  	WARN_ON_ONCE(ftrace_start_up < 0);
>>  
>> +	/* Disabling ipmodify never be failed */
> 
>           "Disabling ipmodify never fails"

OK, I'll fix that.

> 
>> +	ftrace_hash_ipmodify_disable(ops);
>>  	ftrace_hash_rec_disable(ops, 1);
>>  
>>  	if (!global_start_up)
>> @@ -2641,9 +2769,10 @@ static int t_show(struct seq_file *m, void *v)
>>  
>>  	seq_printf(m, "%ps", (void *)rec->ip);
>>  	if (iter->flags & FTRACE_ITER_ENABLED)
>> -		seq_printf(m, " (%ld)%s",
>> +		seq_printf(m, " (%ld)%s%s",
>>  			   rec->flags & ~FTRACE_FL_MASK,
>> -			   rec->flags & FTRACE_FL_REGS ? " R" : "");
>> +			   rec->flags & FTRACE_FL_REGS ? " R" : "",
>> +			   rec->flags & FTRACE_FL_IPMODIFY ? " I" : "");
> 
> Perhaps even add 'I' as we may allow setting it without regs.

Yes, here we do so. :)

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com