lkml.org 
[lkml]   [2014]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH ftrace/core 2/2] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict
Date
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);


>
>> 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..

Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2014-06-11 10:21    [W:0.827 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site