2014-06-10 10:50:13

by 平松雅巳 / HIRAMATU，MASAMI

Subject: [PATCH ftrace/core 0/2] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

Hi,

Here is a pair of patches which introduces IPMODIFY flag for
ftrace_ops to detect conflicts of ftrace users who can modify
regs->ip in their handler.
Currently, only kprobes can change the regs->ip in the handler,
but recently kpatch is also want to change it. Moreover, since
the ftrace itself exported to modules, it might be considerable
senario.

Here we talked on github.
https://github.com/dynup/kpatch/issues/47

To protect modified regs-ip from each other, this series
introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
the flag can be set on each function entry location. If there
is someone who already reserve regs->ip on target function
entry, ftrace_set_filter_ip or register_ftrace_function will
return -EBUSY. Users must handle that.

At this point, all kprobes will reserve regs->ip, since jprobe
requires it.

Thank you,

---

Masami Hiramatsu (2):
ftrace: Simplify ftrace_hash_disable/enable path in ftrace_hash_move
ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict


Documentation/trace/ftrace.txt | 5 +
include/linux/ftrace.h | 10 ++-
kernel/kprobes.c | 2 -
kernel/trace/ftrace.c | 152 ++++++++++++++++++++++++++++++++++------
4 files changed, 143 insertions(+), 26 deletions(-)

--

2014-06-10 10:50:20

by 平松雅巳 / HIRAMATU，MASAMI

Subject: [PATCH ftrace/core 1/2] ftrace: Simplify ftrace_hash_disable/enable path in ftrace_hash_move

Simplify ftrace_hash_disable/enable path in ftrace_hash_move
for hardening the process if the memory allocation failed.

Signed-off-by: Masami Hiramatsu <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
---
kernel/trace/ftrace.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index cc07b7f..e355b60 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1304,25 +1304,15 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
struct ftrace_hash *new_hash;
int size = src->count;
int bits = 0;
- int ret;
int i;

/*
- * Remove the current set, update the hash and add
- * them back.
- */
- ftrace_hash_rec_disable(ops, enable);
-
- /*
* If the new source is empty, just free dst and assign it
* the empty_hash.
*/
if (!src->count) {
- free_ftrace_hash_rcu(*dst);
- rcu_assign_pointer(*dst, EMPTY_HASH);
- /* still need to update the function records */
- ret = 0;
- goto out;
+ new_hash = EMPTY_HASH;
+ goto update;
}

/*
@@ -1335,10 +1325,9 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
if (bits > FTRACE_HASH_MAX_BITS)
bits = FTRACE_HASH_MAX_BITS;

- ret = -ENOMEM;
new_hash = alloc_ftrace_hash(bits);
if (!new_hash)
- goto out;
+ return -ENOMEM;

size = 1 << src->size_bits;
for (i = 0; i < size; i++) {
@@ -1349,20 +1338,20 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
}
}

+update:
+ /*
+ * Remove the current set, update the hash and add
+ * them back.
+ */
+ ftrace_hash_rec_disable(ops, enable);
+
old_hash = *dst;
rcu_assign_pointer(*dst, new_hash);
free_ftrace_hash_rcu(old_hash);

- ret = 0;
- out:
- /*
- * Enable regardless of ret:
- * On success, we enable the new hash.
- * On failure, we re-enable the original hash.
- */
ftrace_hash_rec_enable(ops, enable);

- return ret;
+ return 0;
}

/*

2014-06-10 10:50:25

by 平松雅巳 / HIRAMATU，MASAMI

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

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.

Signed-off-by: Masami Hiramatsu <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
---
Documentation/trace/ftrace.txt | 5 ++
include/linux/ftrace.h | 10 +++
kernel/kprobes.c | 2 -
kernel/trace/ftrace.c | 121 +++++++++++++++++++++++++++++++++++++++-
4 files changed, 133 insertions(+), 5 deletions(-)

diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt
index 2479b2a..0fcad7d 100644
--- a/Documentation/trace/ftrace.txt
+++ b/Documentation/trace/ftrace.txt
@@ -234,6 +234,11 @@ of ftrace. Here is a list of some of the key files:
will be displayed on the same line as the function that
is returning registers.

+ If the callback registered to be traced by a function with
+ the "ip modify" attribute (thus the regs->ip can be changed),
+ a 'I' will be displayed on the same line as the function that
+ can be overridden.
+
function_profile_enabled:

When set it will enable all functions with either the function
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 3e6dfb3..517112a 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -90,6 +90,9 @@ typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
* INITIALIZED - The ftrace_ops has already been initialized (first use time
* register_ftrace_function() is called, it will initialized the ops)
* DELETED - The ops are being deleted, do not let them be registered again.
+ * IPMODIFY - The ops can modify IP register. This must be set with SAVE_REGS
+ * and if the other ops has been set this on same function, filter
+ * update must be failed.
*/
enum {
FTRACE_OPS_FL_ENABLED = 1 << 0,
@@ -101,6 +104,7 @@ enum {
FTRACE_OPS_FL_STUB = 1 << 6,
FTRACE_OPS_FL_INITIALIZED = 1 << 7,
FTRACE_OPS_FL_DELETED = 1 << 8,
+ FTRACE_OPS_FL_IPMODIFY = 1 << 9,
};

/*
@@ -306,6 +310,7 @@ extern int ftrace_nr_registered_ops(void);
* the dyn_ftrace descriptor represents.
*
* The second part is a mask:
+ * IPMODIFY - the record wants to change IP address.
* ENABLED - the function is being traced
* REGS - the record wants the function to save regs
* REGS_EN - the function is set up to save regs.
@@ -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)

struct dyn_ftrace {
unsigned long ip; /* address of mcount call-site */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ceeadfc..3ff7dbd 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -926,7 +926,7 @@ static __kprobes 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,
};
static int kprobe_ftrace_enabled;

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index e355b60..113770a 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1293,6 +1293,9 @@ ftrace_hash_rec_disable(struct ftrace_ops *ops, int filter_hash);
static void
ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);

+static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
+ struct ftrace_hash *new_hash);
+
static int
ftrace_hash_move(struct ftrace_ops *ops, int enable,
struct ftrace_hash **dst, struct ftrace_hash *src)
@@ -1304,6 +1307,7 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
struct ftrace_hash *new_hash;
int size = src->count;
int bits = 0;
+ int ret;
int i;

/*
@@ -1339,6 +1343,13 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
}

update:
+ /* Before everything, make sure this can be applied */
+ ret = ftrace_hash_ipmodify_update(ops, new_hash);
+ if (ret < 0) {
+ free_ftrace_hash(new_hash);
+ return ret;
+ }
+
/*
* Remove the current set, update the hash and add
* them back.
@@ -1593,6 +1604,100 @@ static void ftrace_hash_rec_enable(struct ftrace_ops *ops,
__ftrace_hash_rec_update(ops, filter_hash, 1);
}

+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);
+ 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);
+}
+
+/* Disabling always succeeds */
+static void ftrace_hash_ipmodify_disable(struct ftrace_ops *ops)
+{
+ struct ftrace_hash *hash = ops->filter_hash;
+
+ if (ftrace_hash_empty(hash))
+ hash = NULL;
+
+ __ftrace_hash_update_ipmodify(ops, hash, EMPTY_HASH);
+}
+
+static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
+ struct ftrace_hash *new_hash)
+{
+ struct ftrace_hash *old_hash = ops->filter_hash;
+
+ if (ftrace_hash_empty(old_hash))
+ old_hash = NULL;
+
+ if (ftrace_hash_empty(new_hash))
+ new_hash = NULL;
+
+ return __ftrace_hash_update_ipmodify(ops, old_hash, new_hash);
+}
+
+
static void print_ip_ins(const char *fmt, unsigned char *p)
{
int i;
@@ -2078,6 +2183,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 */
+ ops->flags &= ~FTRACE_OPS_FL_ENABLED;
+ ftrace_start_up--;
+ __unregister_ftrace_function(ops);
+ return ret;
+ }
+
ftrace_hash_rec_enable(ops, 1);

ftrace_startup_enable(command);
@@ -2104,6 +2218,8 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
*/
WARN_ON_ONCE(ftrace_start_up < 0);

+ /* Disabling ipmodify never be failed */
+ ftrace_hash_ipmodify_disable(ops);
ftrace_hash_rec_disable(ops, 1);

if (!global_start_up)
@@ -2641,9 +2757,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" : "");
seq_printf(m, "\n");

return 0;

2014-06-10 13:54:04

by Namhyung Kim

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

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


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

Thanks,
Namhyung


> +
> +/* Disabling always succeeds */
> +static void ftrace_hash_ipmodify_disable(struct ftrace_ops *ops)
> +{
> + struct ftrace_hash *hash = ops->filter_hash;
> +
> + if (ftrace_hash_empty(hash))
> + hash = NULL;
> +
> + __ftrace_hash_update_ipmodify(ops, hash, EMPTY_HASH);
> +}
> +
> +static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
> + struct ftrace_hash *new_hash)
> +{
> + struct ftrace_hash *old_hash = ops->filter_hash;
> +
> + if (ftrace_hash_empty(old_hash))
> + old_hash = NULL;
> +
> + if (ftrace_hash_empty(new_hash))
> + new_hash = NULL;
> +
> + return __ftrace_hash_update_ipmodify(ops, old_hash, new_hash);
> +}
> +

2014-06-11 01:28:10

by 平松雅巳 / HIRAMATU，MASAMI

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

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

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

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

>
> Thanks,
> Namhyung
>
>
>> +
>> +/* Disabling always succeeds */
>> +static void ftrace_hash_ipmodify_disable(struct ftrace_ops *ops)
>> +{
>> + struct ftrace_hash *hash = ops->filter_hash;
>> +
>> + if (ftrace_hash_empty(hash))
>> + hash = NULL;
>> +
>> + __ftrace_hash_update_ipmodify(ops, hash, EMPTY_HASH);
>> +}
>> +
>> +static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
>> + struct ftrace_hash *new_hash)
>> +{
>> + struct ftrace_hash *old_hash = ops->filter_hash;
>> +
>> + if (ftrace_hash_empty(old_hash))
>> + old_hash = NULL;
>> +
>> + if (ftrace_hash_empty(new_hash))
>> + new_hash = NULL;
>> +
>> + return __ftrace_hash_update_ipmodify(ops, old_hash, new_hash);
>> +}
>> +
>
>
>


--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-06-11 07:41:07

by Namhyung Kim

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

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

2014-06-11 16:58:38

by Josh Poimboeuf

Subject: Re: [PATCH ftrace/core 0/2] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

On Tue, Jun 10, 2014 at 10:50:01AM +0000, Masami Hiramatsu wrote:
> Hi,
>
> Here is a pair of patches which introduces IPMODIFY flag for
> ftrace_ops to detect conflicts of ftrace users who can modify
> regs->ip in their handler.
> Currently, only kprobes can change the regs->ip in the handler,
> but recently kpatch is also want to change it. Moreover, since
> the ftrace itself exported to modules, it might be considerable
> senario.
>
> Here we talked on github.
> https://github.com/dynup/kpatch/issues/47
>
> To protect modified regs-ip from each other, this series
> introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
> the flag can be set on each function entry location. If there
> is someone who already reserve regs->ip on target function
> entry, ftrace_set_filter_ip or register_ftrace_function will
> return -EBUSY. Users must handle that.
>
> At this point, all kprobes will reserve regs->ip, since jprobe
> requires it.

Masami, thanks very much for this!

One issue with this approach is that it _always_ makes kprobes and
kpatch incompatible when probing/patching the same function, even when
kprobes doesn't need to touch regs->ip.

Is it possible to add a kprobes flag (KPROBE_FLAG_IPMODIFY), which is
only set by those kprobes users (just jprobes?) which need to modify IP?
Then kprobes could only set the corresponding ftrace flag when it's
really needed. And I think kprobes could even enforce the fact that
!KPROBE_FLAG_IPMODIFY users don't change regs->ip.


BTW, I've done some testing with this patch set by patching/probing the
same function with FTRACE_OPS_FL_IPMODIFY, and got some warnings. I saw
the following warning when attempting to kpatch a kprobed function:


WARNING: CPU: 2 PID: 18351 at kernel/trace/ftrace.c:419 __unregister_ftrace_function+0x1be/0x1d0()
Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) stap_8d70d6e041605bd1e144cba4801652_14636(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal coretemp kvm_intel snd_hda_intel iTCO_wdt iTCO_vendor_support snd_hda_controller kvm snd_hda_codec iwlwifi snd_hwdep uvcvideo snd_seq videobuf2_vmalloc videobuf2_memops snd_seq_device
videobuf2_core btusb v4l2_common snd_pcm videodev nfsd cfg80211 microcode e1000e bluetooth media thinkpad_acpi joydev sdhci_pci sdhci pcspkr serio_raw snd_timer i2c_i801 snd mmc_core auth_rpcgss mei_me mei lpc_ich mfd_core shpchp ptp pps_core wmi tpm_tis soundcore tpm rfkill nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video
CPU: 2 PID: 18351 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
0000000000000000 00000000b39bd289 ffff8803b78d7bc0 ffffffff816f31ed
0000000000000000 ffff8803b78d7bf8 ffffffff8108914d ffffffffa07f9040
00000000fffffff0 0000000000000000 0000000000000001 ffff8803e7ac4200
Call Trace:
[<ffffffff816f31ed>] dump_stack+0x45/0x56
[<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
[<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
[<ffffffff81135bae>] __unregister_ftrace_function+0x1be/0x1d0
[<ffffffff81137294>] ftrace_startup+0x1e4/0x220
[<ffffffff81137313>] register_ftrace_function+0x43/0x60
[<ffffffffa07f6c84>] kpatch_register+0x664/0x830 [kpatch]
[<ffffffffa0810000>] ? 0xffffffffa080ffff
[<ffffffffa0810000>] ? 0xffffffffa080ffff
[<ffffffffa0046194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
[<ffffffffa0046000>] ? 0xffffffffa0045fff
[<ffffffff81002144>] do_one_initcall+0xd4/0x210
[<ffffffff81059d43>] ? set_memory_nx+0x43/0x50
[<ffffffff81102e42>] load_module+0x1d92/0x25e0
[<ffffffff810feb60>] ? store_uevent+0x70/0x70
[<ffffffff811eba30>] ? kernel_read+0x50/0x80
[<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
[<ffffffff81703179>] system_call_fastpath+0x16/0x1b


That warning happened because __unregister_ftrace_function() doesn't
expect FTRACE_OPS_FL_ENABLED to be cleared in the ftrace_startup error
path. I tried removing the FTRACE_OPS_FL_ENABLED clearing line in
ftrace_startup, but I saw more warnings. This one happened when
attempting to kprobe a kpatched function:


WARNING: CPU: 3 PID: 4444 at kernel/kprobes.c:953 arm_kprobe+0xa7/0xe0()
Failed to init kprobe-ftrace (-16)
Modules linked in: stap_b2ea0de23f179d8ded86fcc19fcc533_4444(OE) kpatch_meminfo_string(OE) kpatch(OE) rfcomm fuse ccm ipt_MASQUERADE xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp kvm_intel kvm snd_hda_codec_hdmi iwlwifi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq uvcvideo btusb cfg80211 bluetooth videobuf2_vmalloc videobuf2_memops
videobuf2_core v4l2_common videodev snd_seq_device snd_pcm sdhci_pci media sdhci joydev nfsd i2c_i801 serio_raw pcspkr mmc_core microcode snd_timer e1000e lpc_ich thinkpad_acpi mfd_core shpchp snd wmi tpm_tis soundcore tpm ptp rfkill mei_me auth_rpcgss mei pps_core nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: kpatch_meminfo_string]
CPU: 3 PID: 4444 Comm: stapio Tainted: G U W OE 3.15.0-IPMODIFY+ #1
Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
0000000000000000 000000009cd22363 ffff880427bdfd80 ffffffff816f31ed
ffff880427bdfdc8 ffff880427bdfdb8 ffffffff8108914d ffffffffa08258e0
ffffffffa08258f0 0000000000000000 0000000000000000 0000000000000000
Call Trace:
[<ffffffff816f31ed>] dump_stack+0x45/0x56
[<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
[<ffffffff810891cc>] warn_slowpath_fmt+0x5c/0x80
[<ffffffff816ff9d7>] arm_kprobe+0xa7/0xe0
[<ffffffff817007f7>] register_kprobe+0x557/0x5d0
[<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
[<ffffffffa0820c95>] _stp_ctl_write_cmd+0x8d5/0x930 [stap_b2ea0de23f179d8ded86fcc19fcc533_4444]
[<ffffffff811e5daa>] vfs_write+0xba/0x1e0
[<ffffffff811e6965>] SyS_write+0x55/0xd0
[<ffffffff81703179>] system_call_fastpath+0x16/0x1b


And this one happened after unregistering a probe and then attempting to
register kpatch:


WARNING: CPU: 1 PID: 18041 at kernel/trace/ftrace.c:1584 __ftrace_hash_rec_update.part.35+0x20a/0x240()
Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge bnep stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm iTCO_wdt iTCO_vendor_support mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller x86_pkg_temp_thermal snd_hda_codec coretemp iwlwifi kvm_intel kvm snd_hwdep snd_seq cfg80211 snd_seq_device uvcvideo btusb videobuf2_vmalloc snd_pcm videobuf2_memops videobuf2_core v4l2_common
videodev microcode bluetooth media e1000e pcspkr snd_timer sdhci_pci sdhci thinkpad_acpi i2c_i801 joydev snd serio_raw mmc_core lpc_ich ptp mei_me mfd_core pps_core mei shpchp soundcore rfkill tpm_tis wmi tpm nfsd auth_rpcgss nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: stap_bc293456f85bd78c424ce27a4ab459_18005]
CPU: 1 PID: 18041 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
ffff8803b4717ad8 00000000af73dd79 ffff8803b4717ae8 ffffffff816f31ed
0000000000000000 ffff8803b4717b20 ffffffff8108914d 0000000000000000
0000000000000001 ffff8804280a0ea0 00000000000020ea ffff88042dc04aa0
Call Trace:
[<ffffffff816f31ed>] dump_stack+0x45/0x56
[<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
[<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
[<ffffffff811359ba>] __ftrace_hash_rec_update.part.35+0x20a/0x240
[<ffffffff81135e25>] ftrace_hash_move+0x1d5/0x200
[<ffffffff81137e16>] ftrace_set_hash+0x126/0x1d0
[<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
[<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
[<ffffffff81137f20>] ftrace_set_filter_ip+0x60/0x70
[<ffffffffa07efa50>] kpatch_register+0x430/0x830 [kpatch]
[<ffffffffa0809000>] ? 0xffffffffa0808fff
[<ffffffffa0809000>] ? 0xffffffffa0808fff
[<ffffffffa0047194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
[<ffffffffa0047000>] ? 0xffffffffa0046fff
[<ffffffff81002144>] do_one_initcall+0xd4/0x210
[<ffffffffa0047000>] ? 0xffffffffa0046fff
[<ffffffff81102e42>] load_module+0x1d92/0x25e0
[<ffffffff810feb60>] ? store_uevent+0x70/0x70
[<ffffffff811eba30>] ? kernel_read+0x50/0x80
[<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
[<ffffffff81703179>] system_call_fastpath+0x16/0x1b


--
Josh

2014-06-12 03:28:57

by Namhyung Kim

Subject: Re: [PATCH ftrace/core 0/2] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

Hi Josh,

On Wed, 11 Jun 2014 11:58:26 -0500, Josh Poimboeuf wrote:
> On Tue, Jun 10, 2014 at 10:50:01AM +0000, Masami Hiramatsu wrote:
>> Hi,
>>
>> Here is a pair of patches which introduces IPMODIFY flag for
>> ftrace_ops to detect conflicts of ftrace users who can modify
>> regs->ip in their handler.
>> Currently, only kprobes can change the regs->ip in the handler,
>> but recently kpatch is also want to change it. Moreover, since
>> the ftrace itself exported to modules, it might be considerable
>> senario.
>>
>> Here we talked on github.
>> https://github.com/dynup/kpatch/issues/47
>>
>> To protect modified regs-ip from each other, this series
>> introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
>> the flag can be set on each function entry location. If there
>> is someone who already reserve regs->ip on target function
>> entry, ftrace_set_filter_ip or register_ftrace_function will
>> return -EBUSY. Users must handle that.
>>
>> At this point, all kprobes will reserve regs->ip, since jprobe
>> requires it.
>
> Masami, thanks very much for this!
>
> One issue with this approach is that it _always_ makes kprobes and
> kpatch incompatible when probing/patching the same function, even when
> kprobes doesn't need to touch regs->ip.
>
> Is it possible to add a kprobes flag (KPROBE_FLAG_IPMODIFY), which is
> only set by those kprobes users (just jprobes?) which need to modify IP?
> Then kprobes could only set the corresponding ftrace flag when it's
> really needed. And I think kprobes could even enforce the fact that
> !KPROBE_FLAG_IPMODIFY users don't change regs->ip.
>
>
> BTW, I've done some testing with this patch set by patching/probing the
> same function with FTRACE_OPS_FL_IPMODIFY, and got some warnings. I saw
> the following warning when attempting to kpatch a kprobed function:
>
>
> WARNING: CPU: 2 PID: 18351 at kernel/trace/ftrace.c:419 __unregister_ftrace_function+0x1be/0x1d0()
> Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) stap_8d70d6e041605bd1e144cba4801652_14636(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal coretemp kvm_intel snd_hda_intel iTCO_wdt iTCO_vendor_support snd_hda_controller kvm snd_hda_codec iwlwifi snd_hwdep uvcvideo snd_seq videobuf2_vmalloc videobuf2_memops snd_seq_dev
> ice
> videobuf2_core btusb v4l2_common snd_pcm videodev nfsd cfg80211 microcode e1000e bluetooth media thinkpad_acpi joydev sdhci_pci sdhci pcspkr serio_raw snd_timer i2c_i801 snd mmc_core auth_rpcgss mei_me mei lpc_ich mfd_core shpchp ptp pps_core wmi tpm_tis soundcore tpm rfkill nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video
> CPU: 2 PID: 18351 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> 0000000000000000 00000000b39bd289 ffff8803b78d7bc0 ffffffff816f31ed
> 0000000000000000 ffff8803b78d7bf8 ffffffff8108914d ffffffffa07f9040
> 00000000fffffff0 0000000000000000 0000000000000001 ffff8803e7ac4200
> Call Trace:
> [<ffffffff816f31ed>] dump_stack+0x45/0x56
> [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> [<ffffffff81135bae>] __unregister_ftrace_function+0x1be/0x1d0
> [<ffffffff81137294>] ftrace_startup+0x1e4/0x220
> [<ffffffff81137313>] register_ftrace_function+0x43/0x60
> [<ffffffffa07f6c84>] kpatch_register+0x664/0x830 [kpatch]
> [<ffffffffa0810000>] ? 0xffffffffa080ffff
> [<ffffffffa0810000>] ? 0xffffffffa080ffff
> [<ffffffffa0046194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> [<ffffffffa0046000>] ? 0xffffffffa0045fff
> [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> [<ffffffff81059d43>] ? set_memory_nx+0x43/0x50
> [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
>
>
> That warning happened because __unregister_ftrace_function() doesn't
> expect FTRACE_OPS_FL_ENABLED to be cleared in the ftrace_startup error
> path. I tried removing the FTRACE_OPS_FL_ENABLED clearing line in
> ftrace_startup, but I saw more warnings.

Did you just remove the clearing line or actually clear the flag after
__unregister_ftrace_function() was called?


> This one happened when attempting to kprobe a kpatched function:
>
>
> WARNING: CPU: 3 PID: 4444 at kernel/kprobes.c:953 arm_kprobe+0xa7/0xe0()
> Failed to init kprobe-ftrace (-16)
> Modules linked in: stap_b2ea0de23f179d8ded86fcc19fcc533_4444(OE) kpatch_meminfo_string(OE) kpatch(OE) rfcomm fuse ccm ipt_MASQUERADE xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp kvm_intel kvm snd_hda_codec_hdmi iwlwifi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq uvcvideo btusb cfg80211 bluetooth videobuf2_vmalloc vide
> obuf2_memops
> videobuf2_core v4l2_common videodev snd_seq_device snd_pcm sdhci_pci media sdhci joydev nfsd i2c_i801 serio_raw pcspkr mmc_core microcode snd_timer e1000e lpc_ich thinkpad_acpi mfd_core shpchp snd wmi tpm_tis soundcore tpm ptp rfkill mei_me auth_rpcgss mei pps_core nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: kpatch_meminfo_string]
> CPU: 3 PID: 4444 Comm: stapio Tainted: G U W OE 3.15.0-IPMODIFY+ #1
> Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> 0000000000000000 000000009cd22363 ffff880427bdfd80 ffffffff816f31ed
> ffff880427bdfdc8 ffff880427bdfdb8 ffffffff8108914d ffffffffa08258e0
> ffffffffa08258f0 0000000000000000 0000000000000000 0000000000000000
> Call Trace:
> [<ffffffff816f31ed>] dump_stack+0x45/0x56
> [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> [<ffffffff810891cc>] warn_slowpath_fmt+0x5c/0x80
> [<ffffffff816ff9d7>] arm_kprobe+0xa7/0xe0
> [<ffffffff817007f7>] register_kprobe+0x557/0x5d0
> [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> [<ffffffffa0820c95>] _stp_ctl_write_cmd+0x8d5/0x930 [stap_b2ea0de23f179d8ded86fcc19fcc533_4444]
> [<ffffffff811e5daa>] vfs_write+0xba/0x1e0
> [<ffffffff811e6965>] SyS_write+0x55/0xd0
> [<ffffffff81703179>] system_call_fastpath+0x16/0x1b

This one looks fairly normal as kpatch set FTRACE_FL_IPMODIFY and kprobe
see it again..


>
>
> And this one happened after unregistering a probe and then attempting to
> register kpatch:
>
>
> WARNING: CPU: 1 PID: 18041 at kernel/trace/ftrace.c:1584 __ftrace_hash_rec_update.part.35+0x20a/0x240()
> Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge bnep stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm iTCO_wdt iTCO_vendor_support mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller x86_pkg_temp_thermal snd_hda_codec coretemp iwlwifi kvm_intel kvm snd_hwdep snd_seq cfg80211 snd_seq_device uvcvideo btusb videobuf2_vmalloc snd_pcm videobuf2_memops videobuf2_core v4l2
> _common
> videodev microcode bluetooth media e1000e pcspkr snd_timer sdhci_pci sdhci thinkpad_acpi i2c_i801 joydev snd serio_raw mmc_core lpc_ich ptp mei_me mfd_core pps_core mei shpchp soundcore rfkill tpm_tis wmi tpm nfsd auth_rpcgss nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: stap_bc293456f85bd78c424ce27a4ab459_18005]
> CPU: 1 PID: 18041 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> ffff8803b4717ad8 00000000af73dd79 ffff8803b4717ae8 ffffffff816f31ed
> 0000000000000000 ffff8803b4717b20 ffffffff8108914d 0000000000000000
> 0000000000000001 ffff8804280a0ea0 00000000000020ea ffff88042dc04aa0
> Call Trace:
> [<ffffffff816f31ed>] dump_stack+0x45/0x56
> [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> [<ffffffff811359ba>] __ftrace_hash_rec_update.part.35+0x20a/0x240
> [<ffffffff81135e25>] ftrace_hash_move+0x1d5/0x200
> [<ffffffff81137e16>] ftrace_set_hash+0x126/0x1d0
> [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> [<ffffffff81137f20>] ftrace_set_filter_ip+0x60/0x70
> [<ffffffffa07efa50>] kpatch_register+0x430/0x830 [kpatch]
> [<ffffffffa0809000>] ? 0xffffffffa0808fff
> [<ffffffffa0809000>] ? 0xffffffffa0808fff
> [<ffffffffa0047194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> [<ffffffffa0047000>] ? 0xffffffffa0046fff
> [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> [<ffffffffa0047000>] ? 0xffffffffa0046fff
> [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> [<ffffffff81703179>] system_call_fastpath+0x16/0x1b

But this is weird since kpatch_register() calls ftrace_set_filter_ip()
before registering kpatch_ftrace_ops. So the ops->flags should not have
FTRACE_OPS_FL_ENABLED and it makes __ftrace_hash_rec_update() return
immediately.

So I guessed a following scenario:

register kprobe (success)
register kpatch (-EBUSY, but not reset _FL_ENABLED)
unregister kprobe (success)
register kpatch (boom...)


Thanks,
Namhyung

2014-06-12 03:29:18

by 平松雅巳 / HIRAMATU，MASAMI

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

(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: [email protected]

2014-06-12 05:38:27

by Namhyung Kim

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

Hi Masami,

On Thu, 12 Jun 2014 12:29:09 +0900, Masami Hiramatsu wrote:
> (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
>>>>> + /* 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 :)

Ah, you're right!

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

Then I propose to use a different value/symbol instead of EMPTY_HASH in
order to prevent future confusion and add some comments there.


[SNIP]
>>>>> +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.

But AFAICS both of kprobes and kpatch call ftrace_set_filter_ip() before
calling register_ftrace_function(). That means there's no case when
ops->filter_hash can be empty, right?


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

Right.

Thanks,
Namhyung

2014-06-12 05:44:43

by 平松雅巳 / HIRAMATU，MASAMI

Subject: Re: Re: [PATCH ftrace/core 0/2] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

(2014/06/12 1:58), Josh Poimboeuf wrote:
> On Tue, Jun 10, 2014 at 10:50:01AM +0000, Masami Hiramatsu wrote:
>> Hi,
>>
>> Here is a pair of patches which introduces IPMODIFY flag for
>> ftrace_ops to detect conflicts of ftrace users who can modify
>> regs->ip in their handler.
>> Currently, only kprobes can change the regs->ip in the handler,
>> but recently kpatch is also want to change it. Moreover, since
>> the ftrace itself exported to modules, it might be considerable
>> senario.
>>
>> Here we talked on github.
>> https://github.com/dynup/kpatch/issues/47
>>
>> To protect modified regs-ip from each other, this series
>> introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
>> the flag can be set on each function entry location. If there
>> is someone who already reserve regs->ip on target function
>> entry, ftrace_set_filter_ip or register_ftrace_function will
>> return -EBUSY. Users must handle that.
>>
>> At this point, all kprobes will reserve regs->ip, since jprobe
>> requires it.
>
> Masami, thanks very much for this!
>
> One issue with this approach is that it _always_ makes kprobes and
> kpatch incompatible when probing/patching the same function, even when
> kprobes doesn't need to touch regs->ip.

Right.

> Is it possible to add a kprobes flag (KPROBE_FLAG_IPMODIFY), which is
> only set by those kprobes users (just jprobes?) which need to modify IP?
> Then kprobes could only set the corresponding ftrace flag when it's
> really needed. And I think kprobes could even enforce the fact that
> !KPROBE_FLAG_IPMODIFY users don't change regs->ip.

No, actually we don't need that additional flag, we can slightly change the
kprobes behavior(spec) that requires setting kprobe->break_handler a
function if it modifies regs->ip. (this doesn't break jprobe)
The problem is that we need a separate ftrace_ops for jprobe and other
probes which can change the regs->ip. But current kprobes don't expected
that such case...

> BTW, I've done some testing with this patch set by patching/probing the
> same function with FTRACE_OPS_FL_IPMODIFY, and got some warnings. I saw
> the following warning when attempting to kpatch a kprobed function:

Ah, thanks for testing! I think it needs more work on failure path.


>
> WARNING: CPU: 2 PID: 18351 at kernel/trace/ftrace.c:419 __unregister_ftrace_function+0x1be/0x1d0()
> Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) stap_8d70d6e041605bd1e144cba4801652_14636(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal coretemp kvm_intel snd_hda_intel iTCO_wdt iTCO_vendor_support snd_hda_controller kvm snd_hda_codec iwlwifi snd_hwdep uvcvideo snd_seq videobuf2_vmalloc videobuf2_memops snd_seq_device
> videobuf2_core btusb v4l2_common snd_pcm videodev nfsd cfg80211 microcode e1000e bluetooth media thinkpad_acpi joydev sdhci_pci sdhci pcspkr serio_raw snd_timer i2c_i801 snd mmc_core auth_rpcgss mei_me mei lpc_ich mfd_core shpchp ptp pps_core wmi tpm_tis soundcore tpm rfkill nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video
> CPU: 2 PID: 18351 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> 0000000000000000 00000000b39bd289 ffff8803b78d7bc0 ffffffff816f31ed
> 0000000000000000 ffff8803b78d7bf8 ffffffff8108914d ffffffffa07f9040
> 00000000fffffff0 0000000000000000 0000000000000001 ffff8803e7ac4200
> Call Trace:
> [<ffffffff816f31ed>] dump_stack+0x45/0x56
> [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> [<ffffffff81135bae>] __unregister_ftrace_function+0x1be/0x1d0
> [<ffffffff81137294>] ftrace_startup+0x1e4/0x220
> [<ffffffff81137313>] register_ftrace_function+0x43/0x60
> [<ffffffffa07f6c84>] kpatch_register+0x664/0x830 [kpatch]
> [<ffffffffa0810000>] ? 0xffffffffa080ffff
> [<ffffffffa0810000>] ? 0xffffffffa080ffff
> [<ffffffffa0046194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> [<ffffffffa0046000>] ? 0xffffffffa0045fff
> [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> [<ffffffff81059d43>] ? set_memory_nx+0x43/0x50
> [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
>
>
> That warning happened because __unregister_ftrace_function() doesn't
> expect FTRACE_OPS_FL_ENABLED to be cleared in the ftrace_startup error
> path.

Ah, right! I'll fix that.

> I tried removing the FTRACE_OPS_FL_ENABLED clearing line in
> ftrace_startup, but I saw more warnings. This one happened when
> attempting to kprobe a kpatched function:

Oops! yes, this should happen...

>
>
> WARNING: CPU: 3 PID: 4444 at kernel/kprobes.c:953 arm_kprobe+0xa7/0xe0()
> Failed to init kprobe-ftrace (-16)
> Modules linked in: stap_b2ea0de23f179d8ded86fcc19fcc533_4444(OE) kpatch_meminfo_string(OE) kpatch(OE) rfcomm fuse ccm ipt_MASQUERADE xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp kvm_intel kvm snd_hda_codec_hdmi iwlwifi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq uvcvideo btusb cfg80211 bluetooth videobuf2_vmalloc videobuf2_memops
> videobuf2_core v4l2_common videodev snd_seq_device snd_pcm sdhci_pci media sdhci joydev nfsd i2c_i801 serio_raw pcspkr mmc_core microcode snd_timer e1000e lpc_ich thinkpad_acpi mfd_core shpchp snd wmi tpm_tis soundcore tpm ptp rfkill mei_me auth_rpcgss mei pps_core nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: kpatch_meminfo_string]
> CPU: 3 PID: 4444 Comm: stapio Tainted: G U W OE 3.15.0-IPMODIFY+ #1
> Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> 0000000000000000 000000009cd22363 ffff880427bdfd80 ffffffff816f31ed
> ffff880427bdfdc8 ffff880427bdfdb8 ffffffff8108914d ffffffffa08258e0
> ffffffffa08258f0 0000000000000000 0000000000000000 0000000000000000
> Call Trace:
> [<ffffffff816f31ed>] dump_stack+0x45/0x56
> [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> [<ffffffff810891cc>] warn_slowpath_fmt+0x5c/0x80
> [<ffffffff816ff9d7>] arm_kprobe+0xa7/0xe0
> [<ffffffff817007f7>] register_kprobe+0x557/0x5d0
> [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> [<ffffffffa0820c95>] _stp_ctl_write_cmd+0x8d5/0x930 [stap_b2ea0de23f179d8ded86fcc19fcc533_4444]
> [<ffffffff811e5daa>] vfs_write+0xba/0x1e0
> [<ffffffff811e6965>] SyS_write+0x55/0xd0
> [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
>
>
> And this one happened after unregistering a probe and then attempting to
> register kpatch:

Did you see this on unpatching? it seems to happen on disabling a hash...

>
>
> WARNING: CPU: 1 PID: 18041 at kernel/trace/ftrace.c:1584 __ftrace_hash_rec_update.part.35+0x20a/0x240()
> Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge bnep stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm iTCO_wdt iTCO_vendor_support mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller x86_pkg_temp_thermal snd_hda_codec coretemp iwlwifi kvm_intel kvm snd_hwdep snd_seq cfg80211 snd_seq_device uvcvideo btusb videobuf2_vmalloc snd_pcm videobuf2_memops videobuf2_core v4l2_common
> videodev microcode bluetooth media e1000e pcspkr snd_timer sdhci_pci sdhci thinkpad_acpi i2c_i801 joydev snd serio_raw mmc_core lpc_ich ptp mei_me mfd_core pps_core mei shpchp soundcore rfkill tpm_tis wmi tpm nfsd auth_rpcgss nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: stap_bc293456f85bd78c424ce27a4ab459_18005]
> CPU: 1 PID: 18041 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> ffff8803b4717ad8 00000000af73dd79 ffff8803b4717ae8 ffffffff816f31ed
> 0000000000000000 ffff8803b4717b20 ffffffff8108914d 0000000000000000
> 0000000000000001 ffff8804280a0ea0 00000000000020ea ffff88042dc04aa0
> Call Trace:
> [<ffffffff816f31ed>] dump_stack+0x45/0x56
> [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> [<ffffffff811359ba>] __ftrace_hash_rec_update.part.35+0x20a/0x240
> [<ffffffff81135e25>] ftrace_hash_move+0x1d5/0x200
> [<ffffffff81137e16>] ftrace_set_hash+0x126/0x1d0
> [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> [<ffffffff81137f20>] ftrace_set_filter_ip+0x60/0x70
> [<ffffffffa07efa50>] kpatch_register+0x430/0x830 [kpatch]
> [<ffffffffa0809000>] ? 0xffffffffa0808fff
> [<ffffffffa0809000>] ? 0xffffffffa0808fff
> [<ffffffffa0047194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> [<ffffffffa0047000>] ? 0xffffffffa0046fff
> [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> [<ffffffffa0047000>] ? 0xffffffffa0046fff
> [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
>
>


--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-06-12 05:54:56

by Namhyung Kim

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

On Thu, 12 Jun 2014 12:29:09 +0900, Masami Hiramatsu wrote:
> 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.

Then I found an unrelated issue during review.

It seems that checking NULL other_hash for the 'all' case in
__ftrace_hash_rec_update() is not sufficient. It should check
EMPTY_HASH case too, but then it ends up removing the check at all since
it can be covered in ftrace_lookup_ip().

Thanks,
Namhyung


diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 13885590a184..8bd7aa69a479 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1545,7 +1545,7 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
* Only the filter_hash affects all records.
* Update if the record is not in the notrace hash.
*/
- if (!other_hash || !ftrace_lookup_ip(other_hash, rec->ip))
+ if (!ftrace_lookup_ip(other_hash, rec->ip))
match = 1;
} else {
in_hash = !!ftrace_lookup_ip(hash, rec->ip);

2014-06-12 06:06:27

by 平松雅巳 / HIRAMATU，MASAMI

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

(2014/06/12 14:38), Namhyung Kim wrote:
> Hi Masami,
>
> On Thu, 12 Jun 2014 12:29:09 +0900, Masami Hiramatsu wrote:
>> (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
>>>>>> + /* 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 :)
>
> Ah, you're right!
>
>>
>> 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...
>
> Then I propose to use a different value/symbol instead of EMPTY_HASH in
> order to prevent future confusion and add some comments there.

I doubt I need another symbol since the EMPTY_HASH means normally empty
and no hit(filter_hash case is a special one). I'd like to add a comment on it.


> [SNIP]
>>>>>> +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.
>
> But AFAICS both of kprobes and kpatch call ftrace_set_filter_ip() before
> calling register_ftrace_function(). That means there's no case when
> ops->filter_hash can be empty, right?

No, unless it is registered, FTRACE_OPS_FL_ENABLED flag is not set on the
ftrace_ops. In that case, recs are not updated. :)

Thank you,



--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-06-12 06:57:50

by 平松雅巳 / HIRAMATU，MASAMI

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

(2014/06/12 14:54), Namhyung Kim wrote:
> On Thu, 12 Jun 2014 12:29:09 +0900, Masami Hiramatsu wrote:
>> 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.
>
> Then I found an unrelated issue during review.
>
> It seems that checking NULL other_hash for the 'all' case in
> __ftrace_hash_rec_update() is not sufficient. It should check
> EMPTY_HASH case too, but then it ends up removing the check at all since
> it can be covered in ftrace_lookup_ip().

I guessed that was just for speeding up by skipping ftrace_lookup_ip().
(but I'm not sure how much it improves...)

Reviewed-by: Masami Hiramatsu <[email protected]>

However, I'd like to ask Steven his Ack.

Thank you,

>
> Thanks,
> Namhyung
>
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 13885590a184..8bd7aa69a479 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1545,7 +1545,7 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
> * Only the filter_hash affects all records.
> * Update if the record is not in the notrace hash.
> */
> - if (!other_hash || !ftrace_lookup_ip(other_hash, rec->ip))
> + if (!ftrace_lookup_ip(other_hash, rec->ip))
> match = 1;
> } else {
> in_hash = !!ftrace_lookup_ip(hash, rec->ip);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>


--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-06-12 12:43:48

by Josh Poimboeuf

Subject: Re: [PATCH ftrace/core 0/2] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

On Thu, Jun 12, 2014 at 02:44:35PM +0900, Masami Hiramatsu wrote:
> (2014/06/12 1:58), Josh Poimboeuf wrote:
> > On Tue, Jun 10, 2014 at 10:50:01AM +0000, Masami Hiramatsu wrote:
> >> Hi,
> >>
> >> Here is a pair of patches which introduces IPMODIFY flag for
> >> ftrace_ops to detect conflicts of ftrace users who can modify
> >> regs->ip in their handler.
> >> Currently, only kprobes can change the regs->ip in the handler,
> >> but recently kpatch is also want to change it. Moreover, since
> >> the ftrace itself exported to modules, it might be considerable
> >> senario.
> >>
> >> Here we talked on github.
> >> https://github.com/dynup/kpatch/issues/47
> >>
> >> To protect modified regs-ip from each other, this series
> >> introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
> >> the flag can be set on each function entry location. If there
> >> is someone who already reserve regs->ip on target function
> >> entry, ftrace_set_filter_ip or register_ftrace_function will
> >> return -EBUSY. Users must handle that.
> >>
> >> At this point, all kprobes will reserve regs->ip, since jprobe
> >> requires it.
> >
> > Masami, thanks very much for this!
> >
> > One issue with this approach is that it _always_ makes kprobes and
> > kpatch incompatible when probing/patching the same function, even when
> > kprobes doesn't need to touch regs->ip.
>
> Right.
>
> > Is it possible to add a kprobes flag (KPROBE_FLAG_IPMODIFY), which is
> > only set by those kprobes users (just jprobes?) which need to modify IP?
> > Then kprobes could only set the corresponding ftrace flag when it's
> > really needed. And I think kprobes could even enforce the fact that
> > !KPROBE_FLAG_IPMODIFY users don't change regs->ip.
>
> No, actually we don't need that additional flag, we can slightly change the
> kprobes behavior(spec) that requires setting kprobe->break_handler a
> function if it modifies regs->ip. (this doesn't break jprobe)
> The problem is that we need a separate ftrace_ops for jprobe and other
> probes which can change the regs->ip. But current kprobes don't expected
> that such case...

Sounds good to me.

>
> > BTW, I've done some testing with this patch set by patching/probing the
> > same function with FTRACE_OPS_FL_IPMODIFY, and got some warnings. I saw
> > the following warning when attempting to kpatch a kprobed function:
>
> Ah, thanks for testing! I think it needs more work on failure path.
>
>
> >
> > WARNING: CPU: 2 PID: 18351 at kernel/trace/ftrace.c:419 __unregister_ftrace_function+0x1be/0x1d0()
> > Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) stap_8d70d6e041605bd1e144cba4801652_14636(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal coretemp kvm_intel snd_hda_intel iTCO_wdt iTCO_vendor_support snd_hda_controller kvm snd_hda_codec iwlwifi snd_hwdep uvcvideo snd_seq videobuf2_vmalloc videobuf2_memops snd_seq_device
> > videobuf2_core btusb v4l2_common snd_pcm videodev nfsd cfg80211 microcode e1000e bluetooth media thinkpad_acpi joydev sdhci_pci sdhci pcspkr serio_raw snd_timer i2c_i801 snd mmc_core auth_rpcgss mei_me mei lpc_ich mfd_core shpchp ptp pps_core wmi tpm_tis soundcore tpm rfkill nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video
> > CPU: 2 PID: 18351 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> > Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> > 0000000000000000 00000000b39bd289 ffff8803b78d7bc0 ffffffff816f31ed
> > 0000000000000000 ffff8803b78d7bf8 ffffffff8108914d ffffffffa07f9040
> > 00000000fffffff0 0000000000000000 0000000000000001 ffff8803e7ac4200
> > Call Trace:
> > [<ffffffff816f31ed>] dump_stack+0x45/0x56
> > [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> > [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> > [<ffffffff81135bae>] __unregister_ftrace_function+0x1be/0x1d0
> > [<ffffffff81137294>] ftrace_startup+0x1e4/0x220
> > [<ffffffff81137313>] register_ftrace_function+0x43/0x60
> > [<ffffffffa07f6c84>] kpatch_register+0x664/0x830 [kpatch]
> > [<ffffffffa0810000>] ? 0xffffffffa080ffff
> > [<ffffffffa0810000>] ? 0xffffffffa080ffff
> > [<ffffffffa0046194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> > [<ffffffffa0046000>] ? 0xffffffffa0045fff
> > [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> > [<ffffffff81059d43>] ? set_memory_nx+0x43/0x50
> > [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> > [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> > [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> > [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> > [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
> >
> >
> > That warning happened because __unregister_ftrace_function() doesn't
> > expect FTRACE_OPS_FL_ENABLED to be cleared in the ftrace_startup error
> > path.
>
> Ah, right! I'll fix that.
>
> > I tried removing the FTRACE_OPS_FL_ENABLED clearing line in
> > ftrace_startup, but I saw more warnings. This one happened when
> > attempting to kprobe a kpatched function:
>
> Oops! yes, this should happen...

Instead of a warning here I'd expect to see register_kprobe return an
error and a staprun failure.

>
> >
> >
> > WARNING: CPU: 3 PID: 4444 at kernel/kprobes.c:953 arm_kprobe+0xa7/0xe0()
> > Failed to init kprobe-ftrace (-16)
> > Modules linked in: stap_b2ea0de23f179d8ded86fcc19fcc533_4444(OE) kpatch_meminfo_string(OE) kpatch(OE) rfcomm fuse ccm ipt_MASQUERADE xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp kvm_intel kvm snd_hda_codec_hdmi iwlwifi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq uvcvideo btusb cfg80211 bluetooth videobuf2_vmalloc videobuf2_memops
> > videobuf2_core v4l2_common videodev snd_seq_device snd_pcm sdhci_pci media sdhci joydev nfsd i2c_i801 serio_raw pcspkr mmc_core microcode snd_timer e1000e lpc_ich thinkpad_acpi mfd_core shpchp snd wmi tpm_tis soundcore tpm ptp rfkill mei_me auth_rpcgss mei pps_core nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: kpatch_meminfo_string]
> > CPU: 3 PID: 4444 Comm: stapio Tainted: G U W OE 3.15.0-IPMODIFY+ #1
> > Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> > 0000000000000000 000000009cd22363 ffff880427bdfd80 ffffffff816f31ed
> > ffff880427bdfdc8 ffff880427bdfdb8 ffffffff8108914d ffffffffa08258e0
> > ffffffffa08258f0 0000000000000000 0000000000000000 0000000000000000
> > Call Trace:
> > [<ffffffff816f31ed>] dump_stack+0x45/0x56
> > [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> > [<ffffffff810891cc>] warn_slowpath_fmt+0x5c/0x80
> > [<ffffffff816ff9d7>] arm_kprobe+0xa7/0xe0
> > [<ffffffff817007f7>] register_kprobe+0x557/0x5d0
> > [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> > [<ffffffffa0820c95>] _stp_ctl_write_cmd+0x8d5/0x930 [stap_b2ea0de23f179d8ded86fcc19fcc533_4444]
> > [<ffffffff811e5daa>] vfs_write+0xba/0x1e0
> > [<ffffffff811e6965>] SyS_write+0x55/0xd0
> > [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
> >
> >
> > And this one happened after unregistering a probe and then attempting to
> > register kpatch:
>
> Did you see this on unpatching? it seems to happen on disabling a hash...

No, I think it was in the patching path, after removing the kprobe.
kpatch was calling ftrace_set_filter_ip(), and had not called
register_ftrace_function() yet.

>
> >
> >
> > WARNING: CPU: 1 PID: 18041 at kernel/trace/ftrace.c:1584 __ftrace_hash_rec_update.part.35+0x20a/0x240()
> > Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge bnep stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm iTCO_wdt iTCO_vendor_support mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller x86_pkg_temp_thermal snd_hda_codec coretemp iwlwifi kvm_intel kvm snd_hwdep snd_seq cfg80211 snd_seq_device uvcvideo btusb videobuf2_vmalloc snd_pcm videobuf2_memops videobuf2_core v4l2_common
> > videodev microcode bluetooth media e1000e pcspkr snd_timer sdhci_pci sdhci thinkpad_acpi i2c_i801 joydev snd serio_raw mmc_core lpc_ich ptp mei_me mfd_core pps_core mei shpchp soundcore rfkill tpm_tis wmi tpm nfsd auth_rpcgss nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: stap_bc293456f85bd78c424ce27a4ab459_18005]
> > CPU: 1 PID: 18041 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> > Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> > ffff8803b4717ad8 00000000af73dd79 ffff8803b4717ae8 ffffffff816f31ed
> > 0000000000000000 ffff8803b4717b20 ffffffff8108914d 0000000000000000
> > 0000000000000001 ffff8804280a0ea0 00000000000020ea ffff88042dc04aa0
> > Call Trace:
> > [<ffffffff816f31ed>] dump_stack+0x45/0x56
> > [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> > [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> > [<ffffffff811359ba>] __ftrace_hash_rec_update.part.35+0x20a/0x240
> > [<ffffffff81135e25>] ftrace_hash_move+0x1d5/0x200
> > [<ffffffff81137e16>] ftrace_set_hash+0x126/0x1d0
> > [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> > [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> > [<ffffffff81137f20>] ftrace_set_filter_ip+0x60/0x70
> > [<ffffffffa07efa50>] kpatch_register+0x430/0x830 [kpatch]
> > [<ffffffffa0809000>] ? 0xffffffffa0808fff
> > [<ffffffffa0809000>] ? 0xffffffffa0808fff
> > [<ffffffffa0047194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> > [<ffffffffa0047000>] ? 0xffffffffa0046fff
> > [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> > [<ffffffffa0047000>] ? 0xffffffffa0046fff
> > [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> > [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> > [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> > [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> > [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
> >
> >
>
>
> --
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Research Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: [email protected]
>
>

--
Josh

2014-06-12 12:50:49

by Josh Poimboeuf

Subject: Re: [PATCH ftrace/core 0/2] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

On Thu, Jun 12, 2014 at 12:28:53PM +0900, Namhyung Kim wrote:
> Hi Josh,
>
> On Wed, 11 Jun 2014 11:58:26 -0500, Josh Poimboeuf wrote:
> > On Tue, Jun 10, 2014 at 10:50:01AM +0000, Masami Hiramatsu wrote:
> >> Hi,
> >>
> >> Here is a pair of patches which introduces IPMODIFY flag for
> >> ftrace_ops to detect conflicts of ftrace users who can modify
> >> regs->ip in their handler.
> >> Currently, only kprobes can change the regs->ip in the handler,
> >> but recently kpatch is also want to change it. Moreover, since
> >> the ftrace itself exported to modules, it might be considerable
> >> senario.
> >>
> >> Here we talked on github.
> >> https://github.com/dynup/kpatch/issues/47
> >>
> >> To protect modified regs-ip from each other, this series
> >> introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
> >> the flag can be set on each function entry location. If there
> >> is someone who already reserve regs->ip on target function
> >> entry, ftrace_set_filter_ip or register_ftrace_function will
> >> return -EBUSY. Users must handle that.
> >>
> >> At this point, all kprobes will reserve regs->ip, since jprobe
> >> requires it.
> >
> > Masami, thanks very much for this!
> >
> > One issue with this approach is that it _always_ makes kprobes and
> > kpatch incompatible when probing/patching the same function, even when
> > kprobes doesn't need to touch regs->ip.
> >
> > Is it possible to add a kprobes flag (KPROBE_FLAG_IPMODIFY), which is
> > only set by those kprobes users (just jprobes?) which need to modify IP?
> > Then kprobes could only set the corresponding ftrace flag when it's
> > really needed. And I think kprobes could even enforce the fact that
> > !KPROBE_FLAG_IPMODIFY users don't change regs->ip.
> >
> >
> > BTW, I've done some testing with this patch set by patching/probing the
> > same function with FTRACE_OPS_FL_IPMODIFY, and got some warnings. I saw
> > the following warning when attempting to kpatch a kprobed function:
> >
> >
> > WARNING: CPU: 2 PID: 18351 at kernel/trace/ftrace.c:419 __unregister_ftrace_function+0x1be/0x1d0()
> > Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) stap_8d70d6e041605bd1e144cba4801652_14636(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal coretemp kvm_intel snd_hda_intel iTCO_wdt iTCO_vendor_support snd_hda_controller kvm snd_hda_codec iwlwifi snd_hwdep uvcvideo snd_seq videobuf2_vmalloc videobuf2_memops snd_seq_dev
> > ice
> > videobuf2_core btusb v4l2_common snd_pcm videodev nfsd cfg80211 microcode e1000e bluetooth media thinkpad_acpi joydev sdhci_pci sdhci pcspkr serio_raw snd_timer i2c_i801 snd mmc_core auth_rpcgss mei_me mei lpc_ich mfd_core shpchp ptp pps_core wmi tpm_tis soundcore tpm rfkill nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video
> > CPU: 2 PID: 18351 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> > Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> > 0000000000000000 00000000b39bd289 ffff8803b78d7bc0 ffffffff816f31ed
> > 0000000000000000 ffff8803b78d7bf8 ffffffff8108914d ffffffffa07f9040
> > 00000000fffffff0 0000000000000000 0000000000000001 ffff8803e7ac4200
> > Call Trace:
> > [<ffffffff816f31ed>] dump_stack+0x45/0x56
> > [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> > [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> > [<ffffffff81135bae>] __unregister_ftrace_function+0x1be/0x1d0
> > [<ffffffff81137294>] ftrace_startup+0x1e4/0x220
> > [<ffffffff81137313>] register_ftrace_function+0x43/0x60
> > [<ffffffffa07f6c84>] kpatch_register+0x664/0x830 [kpatch]
> > [<ffffffffa0810000>] ? 0xffffffffa080ffff
> > [<ffffffffa0810000>] ? 0xffffffffa080ffff
> > [<ffffffffa0046194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> > [<ffffffffa0046000>] ? 0xffffffffa0045fff
> > [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> > [<ffffffff81059d43>] ? set_memory_nx+0x43/0x50
> > [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> > [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> > [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> > [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> > [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
> >
> >
> > That warning happened because __unregister_ftrace_function() doesn't
> > expect FTRACE_OPS_FL_ENABLED to be cleared in the ftrace_startup error
> > path. I tried removing the FTRACE_OPS_FL_ENABLED clearing line in
> > ftrace_startup, but I saw more warnings.
>
> Did you just remove the clearing line or actually clear the flag after
> __unregister_ftrace_function() was called?

Ah, I just removed the clearing line. Looks like I should have also
cleared it after calling __unregister_ftrace_function().

>
>
> > This one happened when attempting to kprobe a kpatched function:
> >
> >
> > WARNING: CPU: 3 PID: 4444 at kernel/kprobes.c:953 arm_kprobe+0xa7/0xe0()
> > Failed to init kprobe-ftrace (-16)
> > Modules linked in: stap_b2ea0de23f179d8ded86fcc19fcc533_4444(OE) kpatch_meminfo_string(OE) kpatch(OE) rfcomm fuse ccm ipt_MASQUERADE xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp kvm_intel kvm snd_hda_codec_hdmi iwlwifi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq uvcvideo btusb cfg80211 bluetooth videobuf2_vmalloc vide
> > obuf2_memops
> > videobuf2_core v4l2_common videodev snd_seq_device snd_pcm sdhci_pci media sdhci joydev nfsd i2c_i801 serio_raw pcspkr mmc_core microcode snd_timer e1000e lpc_ich thinkpad_acpi mfd_core shpchp snd wmi tpm_tis soundcore tpm ptp rfkill mei_me auth_rpcgss mei pps_core nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: kpatch_meminfo_string]
> > CPU: 3 PID: 4444 Comm: stapio Tainted: G U W OE 3.15.0-IPMODIFY+ #1
> > Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> > 0000000000000000 000000009cd22363 ffff880427bdfd80 ffffffff816f31ed
> > ffff880427bdfdc8 ffff880427bdfdb8 ffffffff8108914d ffffffffa08258e0
> > ffffffffa08258f0 0000000000000000 0000000000000000 0000000000000000
> > Call Trace:
> > [<ffffffff816f31ed>] dump_stack+0x45/0x56
> > [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> > [<ffffffff810891cc>] warn_slowpath_fmt+0x5c/0x80
> > [<ffffffff816ff9d7>] arm_kprobe+0xa7/0xe0
> > [<ffffffff817007f7>] register_kprobe+0x557/0x5d0
> > [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> > [<ffffffffa0820c95>] _stp_ctl_write_cmd+0x8d5/0x930 [stap_b2ea0de23f179d8ded86fcc19fcc533_4444]
> > [<ffffffff811e5daa>] vfs_write+0xba/0x1e0
> > [<ffffffff811e6965>] SyS_write+0x55/0xd0
> > [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
>
> This one looks fairly normal as kpatch set FTRACE_FL_IPMODIFY and kprobe
> see it again..

Instead of a warning I'd expect to see register_kprobe return an error
back to stap.

>
>
> >
> >
> > And this one happened after unregistering a probe and then attempting to
> > register kpatch:
> >
> >
> > WARNING: CPU: 1 PID: 18041 at kernel/trace/ftrace.c:1584 __ftrace_hash_rec_update.part.35+0x20a/0x240()
> > Modules linked in: kpatch_meminfo_string(OE+) kpatch(OE) rfcomm fuse ipt_MASQUERADE ccm xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge bnep stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm iTCO_wdt iTCO_vendor_support mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller x86_pkg_temp_thermal snd_hda_codec coretemp iwlwifi kvm_intel kvm snd_hwdep snd_seq cfg80211 snd_seq_device uvcvideo btusb videobuf2_vmalloc snd_pcm videobuf2_memops videobuf2_core v4l2
> > _common
> > videodev microcode bluetooth media e1000e pcspkr snd_timer sdhci_pci sdhci thinkpad_acpi i2c_i801 joydev snd serio_raw mmc_core lpc_ich ptp mei_me mfd_core pps_core mei shpchp soundcore rfkill tpm_tis wmi tpm nfsd auth_rpcgss nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: stap_bc293456f85bd78c424ce27a4ab459_18005]
> > CPU: 1 PID: 18041 Comm: insmod Tainted: G W OE 3.15.0-IPMODIFY+ #1
> > Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
> > ffff8803b4717ad8 00000000af73dd79 ffff8803b4717ae8 ffffffff816f31ed
> > 0000000000000000 ffff8803b4717b20 ffffffff8108914d 0000000000000000
> > 0000000000000001 ffff8804280a0ea0 00000000000020ea ffff88042dc04aa0
> > Call Trace:
> > [<ffffffff816f31ed>] dump_stack+0x45/0x56
> > [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
> > [<ffffffff8108927a>] warn_slowpath_null+0x1a/0x20
> > [<ffffffff811359ba>] __ftrace_hash_rec_update.part.35+0x20a/0x240
> > [<ffffffff81135e25>] ftrace_hash_move+0x1d5/0x200
> > [<ffffffff81137e16>] ftrace_set_hash+0x126/0x1d0
> > [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> > [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
> > [<ffffffff81137f20>] ftrace_set_filter_ip+0x60/0x70
> > [<ffffffffa07efa50>] kpatch_register+0x430/0x830 [kpatch]
> > [<ffffffffa0809000>] ? 0xffffffffa0808fff
> > [<ffffffffa0809000>] ? 0xffffffffa0808fff
> > [<ffffffffa0047194>] patch_init+0x194/0x1000 [kpatch_meminfo_string]
> > [<ffffffffa0047000>] ? 0xffffffffa0046fff
> > [<ffffffff81002144>] do_one_initcall+0xd4/0x210
> > [<ffffffffa0047000>] ? 0xffffffffa0046fff
> > [<ffffffff81102e42>] load_module+0x1d92/0x25e0
> > [<ffffffff810feb60>] ? store_uevent+0x70/0x70
> > [<ffffffff811eba30>] ? kernel_read+0x50/0x80
> > [<ffffffff81103846>] SyS_finit_module+0xa6/0xd0
> > [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
>
> But this is weird since kpatch_register() calls ftrace_set_filter_ip()
> before registering kpatch_ftrace_ops. So the ops->flags should not have
> FTRACE_OPS_FL_ENABLED and it makes __ftrace_hash_rec_update() return
> immediately.
>
> So I guessed a following scenario:
>
> register kprobe (success)
> register kpatch (-EBUSY, but not reset _FL_ENABLED)
> unregister kprobe (success)
> register kpatch (boom...)

That's entirely possible :-)

--
Josh

2014-06-13 10:09:54

by 平松雅巳 / HIRAMATU，MASAMI

Subject: Re: Re: [PATCH ftrace/core 0/2] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

(2014/06/12 21:43), Josh Poimboeuf wrote:
>>> I tried removing the FTRACE_OPS_FL_ENABLED clearing line in
>>> ftrace_startup, but I saw more warnings. This one happened when
>>> attempting to kprobe a kpatched function:
>>
>> Oops! yes, this should happen...
>
> Instead of a warning here I'd expect to see register_kprobe return an
> error and a staprun failure.

Actually, since kprobes has "disabled" state, not only register_kprobe
but also enable_kprobe also can be failed. :)


>>> WARNING: CPU: 3 PID: 4444 at kernel/kprobes.c:953 arm_kprobe+0xa7/0xe0()
>>> Failed to init kprobe-ftrace (-16)
>>> Modules linked in: stap_b2ea0de23f179d8ded86fcc19fcc533_4444(OE) kpatch_meminfo_string(OE) kpatch(OE) rfcomm fuse ccm ipt_MASQUERADE xt_CHECKSUM tun ip6t_rpfilter ip6t_REJECT xt_conntrack bnep ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw arc4 iwldvm mac80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp kvm_intel kvm snd_hda_codec_hdmi iwlwifi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq uvcvideo btusb cfg80211 bluetooth videobuf2_vmalloc videobuf2_memops
>>> videobuf2_core v4l2_common videodev snd_seq_device snd_pcm sdhci_pci media sdhci joydev nfsd i2c_i801 serio_raw pcspkr mmc_core microcode snd_timer e1000e lpc_ich thinkpad_acpi mfd_core shpchp snd wmi tpm_tis soundcore tpm ptp rfkill mei_me auth_rpcgss mei pps_core nfs_acl lockd sunrpc dm_crypt i915 i2c_algo_bit drm_kms_helper drm crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_core video [last unloaded: kpatch_meminfo_string]
>>> CPU: 3 PID: 4444 Comm: stapio Tainted: G U W OE 3.15.0-IPMODIFY+ #1
>>> Hardware name: LENOVO 2356BH8/2356BH8, BIOS G7ET63WW (2.05 ) 11/12/2012
>>> 0000000000000000 000000009cd22363 ffff880427bdfd80 ffffffff816f31ed
>>> ffff880427bdfdc8 ffff880427bdfdb8 ffffffff8108914d ffffffffa08258e0
>>> ffffffffa08258f0 0000000000000000 0000000000000000 0000000000000000
>>> Call Trace:
>>> [<ffffffff816f31ed>] dump_stack+0x45/0x56
>>> [<ffffffff8108914d>] warn_slowpath_common+0x7d/0xa0
>>> [<ffffffff810891cc>] warn_slowpath_fmt+0x5c/0x80
>>> [<ffffffff816ff9d7>] arm_kprobe+0xa7/0xe0
>>> [<ffffffff817007f7>] register_kprobe+0x557/0x5d0
>>> [<ffffffff81254da0>] ? meminfo_proc_open+0x30/0x30
>>> [<ffffffffa0820c95>] _stp_ctl_write_cmd+0x8d5/0x930 [stap_b2ea0de23f179d8ded86fcc19fcc533_4444]
>>> [<ffffffff811e5daa>] vfs_write+0xba/0x1e0
>>> [<ffffffff811e6965>] SyS_write+0x55/0xd0
>>> [<ffffffff81703179>] system_call_fastpath+0x16/0x1b
>>>
>>>
>>> And this one happened after unregistering a probe and then attempting to
>>> register kpatch:
>>
>> Did you see this on unpatching? it seems to happen on disabling a hash...
>
> No, I think it was in the patching path, after removing the kprobe.
> kpatch was calling ftrace_set_filter_ip(), and had not called
> register_ftrace_function() yet.

OK, anyway, I'll make a test case for checking it.
Thanks!


--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]