(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...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
