(2014/06/20 11:48), Steven Rostedt wrote:
> On Tue, 17 Jun 2014 11:04:49 +0000
> Masami Hiramatsu <masami.hiramatsu...@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...@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/
