Re: [PATCH ftrace/core 2/2] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict

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

(2014/06/11 16:41), Namhyung Kim wrote:
> Hi Masami,
> 
> On Wed, 11 Jun 2014 10:28:01 +0900, Masami Hiramatsu wrote:
>> (2014/06/10 22:53), Namhyung Kim wrote:
>>> Hi Masami,
>>>
>>> 2014-06-10 (화), 10:50 +0000, Masami Hiramatsu:
>>>> 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.
>>>>
>>>
>>> [SNIP]
>>>> +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))
>>>> +		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);
>>>
>>> Why not use ftrace_hash_empty() here instead of checking NULL? 
>>
>> Ah, a trick is here. Since an empty filter_hash must hit all, we can not
>> enable/disable filter_hash if we use ftrace_hash_empty() here.
>>
>> To enabling the new_hash, old_hash must be EMPTY_HASH which means in_old
>> always be false. To disabling, new_hash is EMPTY_HASH too.
>> Please see ftrace_hash_ipmodify_enable/disable/update().
> 
> I'm confused. 8-p  I guess what you want to do is checking records in
> either of the filter_hash, right?  If so, what about this?
> 
> 	in_old = !ftrace_hash_empty(old_hash) && ftrace_lookup_ip(old_hash, rec->ip);
> 	in_new = !ftrace_hash_empty(new_hash) && ftrace_lookup_ip(new_hash, rec->ip);

NO, ftrace_lookup_ip() returns NULL if the hash is empty, so adding
!ftrace_hash_empty() is meaningless :)

Actually, here I intended to have 3 meanings for the new/old_hash arguments,
- If it is NULL, it hits all
- If it is EMPTY_HASH, it hits nothing
- If it has some entries, it hits those entries.

And in ftrace.c(__ftrace_hash_rec_update), AFAICS, ops->filter_hash has only
2 meanings,
- If it is EMPTY_HASH or NULL, it hits all
- If it has some entries, it hits those entries.

So I had to do above change...

>>> Also
>>> return value of ftrace_lookup_ip is not boolean..  maybe you need to
>>> add !! or convert type of the in_{old,new} to bool.
>>
>> Yeah, I see. And there is '||' (logical OR) which evaluates the result
>> as boolean. :)
> 
> Argh... you're right! :)
> 
>>
>>>
>>>
>>>> +		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;
>>>> +	} while_for_each_ftrace_rec();
>>>> +
>>>> +	return 0;
>>>> +
>>>> +rollback:
>>>> +	end = rec;
>>>> +
>>>> +	/* Roll back what we did above */
>>>> +	do_for_each_ftrace_rec(pg, rec) {
>>>> +		if (rec == end)
>>>> +			goto err_out;
>>>> +
>>>> +		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)
>>>> +			rec->flags &= ~FTRACE_FL_IPMODIFY;
>>>> +		else
>>>> +			rec->flags |= FTRACE_FL_IPMODIFY;
>>>> +	} while_for_each_ftrace_rec();
>>>> +
>>>> +err_out:
>>>> +	return -EBUSY;
>>>> +}
>>>> +
>>>> +static int ftrace_hash_ipmodify_enable(struct ftrace_ops *ops)
>>>> +{
>>>> +	struct ftrace_hash *hash = ops->filter_hash;
>>>> +
>>>> +	if (ftrace_hash_empty(hash))
>>>> +		hash = NULL;
>>>> +
>>>> +	return __ftrace_hash_update_ipmodify(ops, EMPTY_HASH, hash);
>>>> +}
>>>
>>> Please see above comment.  You can pass an empty hash as is, or pass
>>> NULL as second arg.  The same goes to below...
>>
>> As I said above, that is by design :). EMPTY_HASH means it hits nothing,
>> NULL means it hits all.
> 
> But doesn't it make unrelated records also get the flag updated?  I'm
> curious when new_hash can be empty on _enable() case..

NO, _enable() is called right before ftrace_hash_rec_enable(ops,1) which
always enables filter_hash (since the 2nd arg is 1). If the filter_hash
is empty, ftrace_hash_rec_enable() enables ftrace_ops on all ftrace_recs.

Ah, but I found I made a redundant mistake (different one) in ftrace_hash_move(),
ftrace_hash_ipmodify_update() should be done only if "enable" is set (that
means ftrace_hash_move() updates filter_hash, not notrace_hash).
I'll update this patch.

Thank you, :)

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