Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Wed, 25 Jul 2018 11:01:22 -0500
Tom Zanussi wrote:
> > First we have this:
> >
> > ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> > /*
> > * The above returns on success the # of functions enabled,
> > * but if it didn't find any functions it returns zero.
> > * Consider no functions a failure too.
> > */
> >
> > Which looks to be total BS.
>
> Yes, it is BS in the case of event triggers. This was taken from the
> ftrace function trigger code, where it does make sense. I think I left
> it in thinking the code would at some point later converge.
OK, that makes a little more sense.
>
> >
> > As we have this:
> >
> > /**
> > * register_trigger - Generic event_command @reg implementation
> > * @glob: The raw string used to register the trigger
> > * @ops: The trigger ops associated with the trigger
> > * @data: Trigger-specific data to associate with the trigger
> > * @file: The trace_event_file associated with the event
> > *
> > * Common implementation for event trigger registration.
> > *
> > * Usually used directly as the @reg method in event command
> > * implementations.
> > *
> > * Return: 0 on success, errno otherwise
>
> And this is how it should work.
>
> > */
> > static int register_trigger(char *glob, struct event_trigger_ops *ops,
> > struct event_trigger_data *data,
> > struct trace_event_file *file)
> > {
> > struct event_trigger_data *test;
> > int ret = 0;
> >
> > list_for_each_entry_rcu(test, >triggers, list) {
> > if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type)
> > {
> > ret = -EEXIST;
> > goto out;
> > }
> > }
> >
> > if (data->ops->init) {
> > ret = data->ops->init(data->ops, data);
> > if (ret < 0)
> > goto out;
> > }
> >
> > list_add_rcu(>list, >triggers);
> > ret++;
> >
> > update_cond_flag(file);
> > if (trace_event_trigger_enable_disable(file, 1) < 0) {
> > list_del_rcu(>list);
> > update_cond_flag(file);
> > ret--;
> > }
> > out:
> > return ret;
> > }
> >
> > Where the comment is total wrong. It doesn't return 0 on success, it
> > returns 1. And if trace_event_trigger_enable_disable() fails it returns
> > zero.
> >
> > And that can fail with the call->class->reg() return value, which could
> > fail for various strange reasons. I don't know why we would want to
> > return 0 when it fails?
> >
> > I don't see where ->reg() would return anything but 1 on success. Maybe
> > I'm missing something. I'll look some more, but I'm thinking of changing
> > ->reg() to return zero on all success, and negative on all errors and
> > just check those results.
> >
>
> Right, in the case of event triggers, we only register one at a time,
> whereas with the trace function triggers, with globbing multiple
> functions can register triggers at the same time, so it makes sense
> there to have reg() return a count and the more convoluted error handling.
OK, reg in function probes will be handled differently.
>
> So I agree, simplifying things here by using the standard error handling
> would be an improvement.
I'll start working on something for 4.19 to simplify it.
Thanks for confirming!
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Wed, 25 Jul 2018 11:01:22 -0500
Tom Zanussi wrote:
> > First we have this:
> >
> > ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> > /*
> > * The above returns on success the # of functions enabled,
> > * but if it didn't find any functions it returns zero.
> > * Consider no functions a failure too.
> > */
> >
> > Which looks to be total BS.
>
> Yes, it is BS in the case of event triggers. This was taken from the
> ftrace function trigger code, where it does make sense. I think I left
> it in thinking the code would at some point later converge.
OK, that makes a little more sense.
>
> >
> > As we have this:
> >
> > /**
> > * register_trigger - Generic event_command @reg implementation
> > * @glob: The raw string used to register the trigger
> > * @ops: The trigger ops associated with the trigger
> > * @data: Trigger-specific data to associate with the trigger
> > * @file: The trace_event_file associated with the event
> > *
> > * Common implementation for event trigger registration.
> > *
> > * Usually used directly as the @reg method in event command
> > * implementations.
> > *
> > * Return: 0 on success, errno otherwise
>
> And this is how it should work.
>
> > */
> > static int register_trigger(char *glob, struct event_trigger_ops *ops,
> > struct event_trigger_data *data,
> > struct trace_event_file *file)
> > {
> > struct event_trigger_data *test;
> > int ret = 0;
> >
> > list_for_each_entry_rcu(test, >triggers, list) {
> > if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type)
> > {
> > ret = -EEXIST;
> > goto out;
> > }
> > }
> >
> > if (data->ops->init) {
> > ret = data->ops->init(data->ops, data);
> > if (ret < 0)
> > goto out;
> > }
> >
> > list_add_rcu(>list, >triggers);
> > ret++;
> >
> > update_cond_flag(file);
> > if (trace_event_trigger_enable_disable(file, 1) < 0) {
> > list_del_rcu(>list);
> > update_cond_flag(file);
> > ret--;
> > }
> > out:
> > return ret;
> > }
> >
> > Where the comment is total wrong. It doesn't return 0 on success, it
> > returns 1. And if trace_event_trigger_enable_disable() fails it returns
> > zero.
> >
> > And that can fail with the call->class->reg() return value, which could
> > fail for various strange reasons. I don't know why we would want to
> > return 0 when it fails?
> >
> > I don't see where ->reg() would return anything but 1 on success. Maybe
> > I'm missing something. I'll look some more, but I'm thinking of changing
> > ->reg() to return zero on all success, and negative on all errors and
> > just check those results.
> >
>
> Right, in the case of event triggers, we only register one at a time,
> whereas with the trace function triggers, with globbing multiple
> functions can register triggers at the same time, so it makes sense
> there to have reg() return a count and the more convoluted error handling.
OK, reg in function probes will be handled differently.
>
> So I agree, simplifying things here by using the standard error handling
> would be an improvement.
I'll start working on something for 4.19 to simplify it.
Thanks for confirming!
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
Hi Steve,
On 7/24/2018 4:30 PM, Steven Rostedt wrote:
On Tue, 24 Jul 2018 16:49:59 -0400
Steven Rostedt wrote:
Hmm it seems we should review the register_trigger() implementation.
It should return the return value of trace_event_trigger_enable_disable(),
shouldn't it?
Yeah, that's not done well. I'll fix it up.
Thanks for pointing it out.
Tom,
register_trigger() is messed up. I should have caught this when it was
first submitted, but I'm totally confused. The comments don't match the
code.
First we have this:
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
Which looks to be total BS.
Yes, it is BS in the case of event triggers. This was taken from the
ftrace function trigger code, where it does make sense. I think I left
it in thinking the code would at some point later converge.
As we have this:
/**
* register_trigger - Generic event_command @reg implementation
* @glob: The raw string used to register the trigger
* @ops: The trigger ops associated with the trigger
* @data: Trigger-specific data to associate with the trigger
* @file: The trace_event_file associated with the event
*
* Common implementation for event trigger registration.
*
* Usually used directly as the @reg method in event command
* implementations.
*
* Return: 0 on success, errno otherwise
And this is how it should work.
*/
static int register_trigger(char *glob, struct event_trigger_ops *ops,
struct event_trigger_data *data,
struct trace_event_file *file)
{
struct event_trigger_data *test;
int ret = 0;
list_for_each_entry_rcu(test, >triggers, list) {
if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type)
{
ret = -EEXIST;
goto out;
}
}
if (data->ops->init) {
ret = data->ops->init(data->ops, data);
if (ret < 0)
goto out;
}
list_add_rcu(>list, >triggers);
ret++;
update_cond_flag(file);
if (trace_event_trigger_enable_disable(file, 1) < 0) {
list_del_rcu(>list);
update_cond_flag(file);
ret--;
}
out:
return ret;
}
Where the comment is total wrong. It doesn't return 0 on success, it
returns 1. And if trace_event_trigger_enable_disable() fails it returns
zero.
And that can fail with the call->class->reg() return value, which could
fail for various strange reasons. I don't know why we would want to
return 0 when it fails?
I don't see where ->reg() would return anything but 1 on success. Maybe
I'm missing something. I'll look some more, but I'm thinking of changing
->reg() to return zero on all success, and negative on all errors and
just check those results.
Right, in the case of event triggers, we only register one at a time,
whereas with the trace function triggers, with globbing multiple
functions can register triggers at the same time, so it makes sense
there to have reg() return a count and the more convoluted error handling.
So I agree, simplifying things here by using the standard error handling
would be an improvement.
Tom
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
Hi Steve,
On 7/24/2018 4:30 PM, Steven Rostedt wrote:
On Tue, 24 Jul 2018 16:49:59 -0400
Steven Rostedt wrote:
Hmm it seems we should review the register_trigger() implementation.
It should return the return value of trace_event_trigger_enable_disable(),
shouldn't it?
Yeah, that's not done well. I'll fix it up.
Thanks for pointing it out.
Tom,
register_trigger() is messed up. I should have caught this when it was
first submitted, but I'm totally confused. The comments don't match the
code.
First we have this:
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
Which looks to be total BS.
Yes, it is BS in the case of event triggers. This was taken from the
ftrace function trigger code, where it does make sense. I think I left
it in thinking the code would at some point later converge.
As we have this:
/**
* register_trigger - Generic event_command @reg implementation
* @glob: The raw string used to register the trigger
* @ops: The trigger ops associated with the trigger
* @data: Trigger-specific data to associate with the trigger
* @file: The trace_event_file associated with the event
*
* Common implementation for event trigger registration.
*
* Usually used directly as the @reg method in event command
* implementations.
*
* Return: 0 on success, errno otherwise
And this is how it should work.
*/
static int register_trigger(char *glob, struct event_trigger_ops *ops,
struct event_trigger_data *data,
struct trace_event_file *file)
{
struct event_trigger_data *test;
int ret = 0;
list_for_each_entry_rcu(test, >triggers, list) {
if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type)
{
ret = -EEXIST;
goto out;
}
}
if (data->ops->init) {
ret = data->ops->init(data->ops, data);
if (ret < 0)
goto out;
}
list_add_rcu(>list, >triggers);
ret++;
update_cond_flag(file);
if (trace_event_trigger_enable_disable(file, 1) < 0) {
list_del_rcu(>list);
update_cond_flag(file);
ret--;
}
out:
return ret;
}
Where the comment is total wrong. It doesn't return 0 on success, it
returns 1. And if trace_event_trigger_enable_disable() fails it returns
zero.
And that can fail with the call->class->reg() return value, which could
fail for various strange reasons. I don't know why we would want to
return 0 when it fails?
I don't see where ->reg() would return anything but 1 on success. Maybe
I'm missing something. I'll look some more, but I'm thinking of changing
->reg() to return zero on all success, and negative on all errors and
just check those results.
Right, in the case of event triggers, we only register one at a time,
whereas with the trace function triggers, with globbing multiple
functions can register triggers at the same time, so it makes sense
there to have reg() return a count and the more convoluted error handling.
So I agree, simplifying things here by using the standard error handling
would be an improvement.
Tom
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 22:41:49 -0400
Steven Rostedt wrote:
> On Wed, 25 Jul 2018 10:16:53 +0900
> Masami Hiramatsu wrote:
>
> > Hm, as far as I can see, when register_trigger() returns >= 0, it already
> > calls ->init the trigger_data. This means its refcount++, and in that
> > case, below patch will miss to free the trigger_data.
> > How about below for tentative fix?
> >
> > if (!ret) {
> > ret = -ENOENT;
> > /* We have to forcibly free the trigger_data */
> > goto out_free;
> > } else if (ret > 0)
> > ret = 0;
>
> Or better yet, match it properly:
OK, this looks good to me :)
Reviewed-by: Masami Hiramatsu
Thanks!
>
> out_reg:
> /* Up the trigger_data count to make sure reg doesn't free it on
> failure */
> event_trigger_init(trigger_ops, trigger_data);
> ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> /*
>* The above returns on success the # of functions enabled,
>* but if it didn't find any functions it returns zero.
>* Consider no functions a failure too.
>*/
> if (!ret) {
> cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
> ret = -ENOENT;
> } else if (ret > 0)
> ret = 0;
>
> /* Down the counter of trigger_data or free it if not used anymore */
> event_trigger_free(trigger_ops, trigger_data);
> out:
> return ret;
>
> -- Steve
--
Masami Hiramatsu
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 22:41:49 -0400
Steven Rostedt wrote:
> On Wed, 25 Jul 2018 10:16:53 +0900
> Masami Hiramatsu wrote:
>
> > Hm, as far as I can see, when register_trigger() returns >= 0, it already
> > calls ->init the trigger_data. This means its refcount++, and in that
> > case, below patch will miss to free the trigger_data.
> > How about below for tentative fix?
> >
> > if (!ret) {
> > ret = -ENOENT;
> > /* We have to forcibly free the trigger_data */
> > goto out_free;
> > } else if (ret > 0)
> > ret = 0;
>
> Or better yet, match it properly:
OK, this looks good to me :)
Reviewed-by: Masami Hiramatsu
Thanks!
>
> out_reg:
> /* Up the trigger_data count to make sure reg doesn't free it on
> failure */
> event_trigger_init(trigger_ops, trigger_data);
> ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> /*
>* The above returns on success the # of functions enabled,
>* but if it didn't find any functions it returns zero.
>* Consider no functions a failure too.
>*/
> if (!ret) {
> cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
> ret = -ENOENT;
> } else if (ret > 0)
> ret = 0;
>
> /* Down the counter of trigger_data or free it if not used anymore */
> event_trigger_free(trigger_ops, trigger_data);
> out:
> return ret;
>
> -- Steve
--
Masami Hiramatsu
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Wed, 25 Jul 2018 10:16:53 +0900
Masami Hiramatsu wrote:
> Hm, as far as I can see, when register_trigger() returns >= 0, it already
> calls ->init the trigger_data. This means its refcount++, and in that
> case, below patch will miss to free the trigger_data.
> How about below for tentative fix?
>
> if (!ret) {
> ret = -ENOENT;
> /* We have to forcibly free the trigger_data */
> goto out_free;
> } else if (ret > 0)
> ret = 0;
Or better yet, match it properly:
out_reg:
/* Up the trigger_data count to make sure reg doesn't free it on
failure */
event_trigger_init(trigger_ops, trigger_data);
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
if (!ret) {
cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
ret = -ENOENT;
} else if (ret > 0)
ret = 0;
/* Down the counter of trigger_data or free it if not used anymore */
event_trigger_free(trigger_ops, trigger_data);
out:
return ret;
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Wed, 25 Jul 2018 10:16:53 +0900
Masami Hiramatsu wrote:
> Hm, as far as I can see, when register_trigger() returns >= 0, it already
> calls ->init the trigger_data. This means its refcount++, and in that
> case, below patch will miss to free the trigger_data.
> How about below for tentative fix?
>
> if (!ret) {
> ret = -ENOENT;
> /* We have to forcibly free the trigger_data */
> goto out_free;
> } else if (ret > 0)
> ret = 0;
Or better yet, match it properly:
out_reg:
/* Up the trigger_data count to make sure reg doesn't free it on
failure */
event_trigger_init(trigger_ops, trigger_data);
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
if (!ret) {
cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
ret = -ENOENT;
} else if (ret > 0)
ret = 0;
/* Down the counter of trigger_data or free it if not used anymore */
event_trigger_free(trigger_ops, trigger_data);
out:
return ret;
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 19:13:31 -0400
Steven Rostedt wrote:
> On Tue, 24 Jul 2018 17:30:08 -0400
> Steven Rostedt wrote:
>
> > I don't see where ->reg() would return anything but 1 on success. Maybe
> > I'm missing something. I'll look some more, but I'm thinking of changing
> > ->reg() to return zero on all success, and negative on all errors and
> > just check those results.
>
> Yeah, I'm going to make these changes. That is, all struct
> event_command .reg functions will return negative on error, and
> zero or positive on everything else. All the checks are going to be
> only for the negative value.
>
> But for the bug fix itself, I believe this should do it. Masami, what
> do you think?
Hm, as far as I can see, when register_trigger() returns >= 0, it already
calls ->init the trigger_data. This means its refcount++, and in that
case, below patch will miss to free the trigger_data.
How about below for tentative fix?
if (!ret) {
ret = -ENOENT;
/* We have to forcibly free the trigger_data */
goto out_free;
} else if (ret > 0)
ret = 0;
Thank you,
>
> -- Steve
>
> diff --git a/kernel/trace/trace_events_trigger.c
> b/kernel/trace/trace_events_trigger.c
> index d18249683682..8771a27ca60f 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -679,6 +679,8 @@ event_trigger_callback(struct event_command *cmd_ops,
> goto out_free;
>
> out_reg:
> + /* Up the trigger_data count to make sure reg doesn't free it on
> failure */
> + event_trigger_init(trigger_ops, trigger_data);
> ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> /*
>* The above returns on success the # of functions enabled,
> @@ -687,10 +689,11 @@ event_trigger_callback(struct event_command *cmd_ops,
>*/
> if (!ret) {
> ret = -ENOENT;
> - goto out_free;
> - } else if (ret < 0)
> - goto out_free;
> - ret = 0;
> + } else if (ret > 0)
> + ret = 0;
> +
> + /* Down the counter of trigger_data or free it if not used anymore */
> + event_trigger_free(trigger_ops, trigger_data);
> out:
> return ret;
>
--
Masami Hiramatsu
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 19:13:31 -0400
Steven Rostedt wrote:
> On Tue, 24 Jul 2018 17:30:08 -0400
> Steven Rostedt wrote:
>
> > I don't see where ->reg() would return anything but 1 on success. Maybe
> > I'm missing something. I'll look some more, but I'm thinking of changing
> > ->reg() to return zero on all success, and negative on all errors and
> > just check those results.
>
> Yeah, I'm going to make these changes. That is, all struct
> event_command .reg functions will return negative on error, and
> zero or positive on everything else. All the checks are going to be
> only for the negative value.
>
> But for the bug fix itself, I believe this should do it. Masami, what
> do you think?
Hm, as far as I can see, when register_trigger() returns >= 0, it already
calls ->init the trigger_data. This means its refcount++, and in that
case, below patch will miss to free the trigger_data.
How about below for tentative fix?
if (!ret) {
ret = -ENOENT;
/* We have to forcibly free the trigger_data */
goto out_free;
} else if (ret > 0)
ret = 0;
Thank you,
>
> -- Steve
>
> diff --git a/kernel/trace/trace_events_trigger.c
> b/kernel/trace/trace_events_trigger.c
> index d18249683682..8771a27ca60f 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -679,6 +679,8 @@ event_trigger_callback(struct event_command *cmd_ops,
> goto out_free;
>
> out_reg:
> + /* Up the trigger_data count to make sure reg doesn't free it on
> failure */
> + event_trigger_init(trigger_ops, trigger_data);
> ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> /*
>* The above returns on success the # of functions enabled,
> @@ -687,10 +689,11 @@ event_trigger_callback(struct event_command *cmd_ops,
>*/
> if (!ret) {
> ret = -ENOENT;
> - goto out_free;
> - } else if (ret < 0)
> - goto out_free;
> - ret = 0;
> + } else if (ret > 0)
> + ret = 0;
> +
> + /* Down the counter of trigger_data or free it if not used anymore */
> + event_trigger_free(trigger_ops, trigger_data);
> out:
> return ret;
>
--
Masami Hiramatsu
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 16:49:59 -0400
Steven Rostedt wrote:
> On Wed, 25 Jul 2018 00:09:09 +0900
> Masami Hiramatsu wrote:
>
> > Hmm, your patch seems to leak a memory since event_trigger_init() will
> > be called twice on same trigger_data (Note that event_trigger_init()
> > does not init ref counter, but increment it.) So we should decrement
> > it when we find it is succeeded. Moreover, if register_trigger()
>
> Good catch, and easily fixed.
>
> > fails before calling data->ops->init() (see -EEXIST case), the ref
> > counter will be 0 (-1 +1). But if it fails after data->ops->init(),
> > the ref counter will be 1 (-1 +1 +1). It still be unstable.
> > (Ah, that means we may have another trouble...)
>
> I'm not sure there's a problem here. I now have:
>
> out_reg:
> /* Up the trigger_data count to make sure reg doesn't free it on
> failuer */
> event_trigger_init(trigger_ops, trigger_data);
> ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> /*
>* The above returns on success the # of functions enabled,
>* but if it didn't find any functions it returns zero.
>* Consider no functions a failure too.
>*/
> if (!ret) {
> ret = -ENOENT;
> } else if (ret > 0)
> ret = 0;
Can we mixed up ret == 0 and ret > 0? It seems cmd_ops->reg() == 0
is a failure case.
>
> /* Down the counter of trigger_data or free it if not used anymore */
> event_trigger_free(trigger_ops, trigger_data);
> out:
> return ret;
>
> Thus we increment trigger_data before calling reg, and free it
> afterward. But if reg() did an init too, then the event_trigger_free()
> just decs the ref counter.
To avoid confusion, I would like to suggest to rename those pair to
event_trigger_data_get/put(). :)
>
> As for register_trigger()
>
>
> >
> > >
> > > P.S. This brings up another minor bug. The failure should return ENOMEM
> > > not ENOENT.
> >
> > Hmm it seems we should review the register_trigger() implementation.
> > It should return the return value of trace_event_trigger_enable_disable(),
> > shouldn't it?
> >
>
> Yeah, that's not done well. I'll fix it up.
Thanks!
>
> Thanks for pointing it out.
>
> -- Steve
--
Masami Hiramatsu
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 16:49:59 -0400
Steven Rostedt wrote:
> On Wed, 25 Jul 2018 00:09:09 +0900
> Masami Hiramatsu wrote:
>
> > Hmm, your patch seems to leak a memory since event_trigger_init() will
> > be called twice on same trigger_data (Note that event_trigger_init()
> > does not init ref counter, but increment it.) So we should decrement
> > it when we find it is succeeded. Moreover, if register_trigger()
>
> Good catch, and easily fixed.
>
> > fails before calling data->ops->init() (see -EEXIST case), the ref
> > counter will be 0 (-1 +1). But if it fails after data->ops->init(),
> > the ref counter will be 1 (-1 +1 +1). It still be unstable.
> > (Ah, that means we may have another trouble...)
>
> I'm not sure there's a problem here. I now have:
>
> out_reg:
> /* Up the trigger_data count to make sure reg doesn't free it on
> failuer */
> event_trigger_init(trigger_ops, trigger_data);
> ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> /*
>* The above returns on success the # of functions enabled,
>* but if it didn't find any functions it returns zero.
>* Consider no functions a failure too.
>*/
> if (!ret) {
> ret = -ENOENT;
> } else if (ret > 0)
> ret = 0;
Can we mixed up ret == 0 and ret > 0? It seems cmd_ops->reg() == 0
is a failure case.
>
> /* Down the counter of trigger_data or free it if not used anymore */
> event_trigger_free(trigger_ops, trigger_data);
> out:
> return ret;
>
> Thus we increment trigger_data before calling reg, and free it
> afterward. But if reg() did an init too, then the event_trigger_free()
> just decs the ref counter.
To avoid confusion, I would like to suggest to rename those pair to
event_trigger_data_get/put(). :)
>
> As for register_trigger()
>
>
> >
> > >
> > > P.S. This brings up another minor bug. The failure should return ENOMEM
> > > not ENOENT.
> >
> > Hmm it seems we should review the register_trigger() implementation.
> > It should return the return value of trace_event_trigger_enable_disable(),
> > shouldn't it?
> >
>
> Yeah, that's not done well. I'll fix it up.
Thanks!
>
> Thanks for pointing it out.
>
> -- Steve
--
Masami Hiramatsu
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 17:30:08 -0400
Steven Rostedt wrote:
> I don't see where ->reg() would return anything but 1 on success. Maybe
> I'm missing something. I'll look some more, but I'm thinking of changing
> ->reg() to return zero on all success, and negative on all errors and
> just check those results.
Yeah, I'm going to make these changes. That is, all struct
event_command .reg functions will return negative on error, and
zero or positive on everything else. All the checks are going to be
only for the negative value.
But for the bug fix itself, I believe this should do it. Masami, what
do you think?
-- Steve
diff --git a/kernel/trace/trace_events_trigger.c
b/kernel/trace/trace_events_trigger.c
index d18249683682..8771a27ca60f 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -679,6 +679,8 @@ event_trigger_callback(struct event_command *cmd_ops,
goto out_free;
out_reg:
+ /* Up the trigger_data count to make sure reg doesn't free it on
failure */
+ event_trigger_init(trigger_ops, trigger_data);
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
@@ -687,10 +689,11 @@ event_trigger_callback(struct event_command *cmd_ops,
*/
if (!ret) {
ret = -ENOENT;
- goto out_free;
- } else if (ret < 0)
- goto out_free;
- ret = 0;
+ } else if (ret > 0)
+ ret = 0;
+
+ /* Down the counter of trigger_data or free it if not used anymore */
+ event_trigger_free(trigger_ops, trigger_data);
out:
return ret;
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 17:30:08 -0400
Steven Rostedt wrote:
> I don't see where ->reg() would return anything but 1 on success. Maybe
> I'm missing something. I'll look some more, but I'm thinking of changing
> ->reg() to return zero on all success, and negative on all errors and
> just check those results.
Yeah, I'm going to make these changes. That is, all struct
event_command .reg functions will return negative on error, and
zero or positive on everything else. All the checks are going to be
only for the negative value.
But for the bug fix itself, I believe this should do it. Masami, what
do you think?
-- Steve
diff --git a/kernel/trace/trace_events_trigger.c
b/kernel/trace/trace_events_trigger.c
index d18249683682..8771a27ca60f 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -679,6 +679,8 @@ event_trigger_callback(struct event_command *cmd_ops,
goto out_free;
out_reg:
+ /* Up the trigger_data count to make sure reg doesn't free it on
failure */
+ event_trigger_init(trigger_ops, trigger_data);
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
@@ -687,10 +689,11 @@ event_trigger_callback(struct event_command *cmd_ops,
*/
if (!ret) {
ret = -ENOENT;
- goto out_free;
- } else if (ret < 0)
- goto out_free;
- ret = 0;
+ } else if (ret > 0)
+ ret = 0;
+
+ /* Down the counter of trigger_data or free it if not used anymore */
+ event_trigger_free(trigger_ops, trigger_data);
out:
return ret;
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 16:49:59 -0400
Steven Rostedt wrote:
> > Hmm it seems we should review the register_trigger() implementation.
> > It should return the return value of trace_event_trigger_enable_disable(),
> > shouldn't it?
> >
>
> Yeah, that's not done well. I'll fix it up.
>
> Thanks for pointing it out.
Tom,
register_trigger() is messed up. I should have caught this when it was
first submitted, but I'm totally confused. The comments don't match the
code.
First we have this:
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
Which looks to be total BS.
As we have this:
/**
* register_trigger - Generic event_command @reg implementation
* @glob: The raw string used to register the trigger
* @ops: The trigger ops associated with the trigger
* @data: Trigger-specific data to associate with the trigger
* @file: The trace_event_file associated with the event
*
* Common implementation for event trigger registration.
*
* Usually used directly as the @reg method in event command
* implementations.
*
* Return: 0 on success, errno otherwise
*/
static int register_trigger(char *glob, struct event_trigger_ops *ops,
struct event_trigger_data *data,
struct trace_event_file *file)
{
struct event_trigger_data *test;
int ret = 0;
list_for_each_entry_rcu(test, >triggers, list) {
if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type)
{
ret = -EEXIST;
goto out;
}
}
if (data->ops->init) {
ret = data->ops->init(data->ops, data);
if (ret < 0)
goto out;
}
list_add_rcu(>list, >triggers);
ret++;
update_cond_flag(file);
if (trace_event_trigger_enable_disable(file, 1) < 0) {
list_del_rcu(>list);
update_cond_flag(file);
ret--;
}
out:
return ret;
}
Where the comment is total wrong. It doesn't return 0 on success, it
returns 1. And if trace_event_trigger_enable_disable() fails it returns
zero.
And that can fail with the call->class->reg() return value, which could
fail for various strange reasons. I don't know why we would want to
return 0 when it fails?
I don't see where ->reg() would return anything but 1 on success. Maybe
I'm missing something. I'll look some more, but I'm thinking of changing
->reg() to return zero on all success, and negative on all errors and
just check those results.
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Tue, 24 Jul 2018 16:49:59 -0400
Steven Rostedt wrote:
> > Hmm it seems we should review the register_trigger() implementation.
> > It should return the return value of trace_event_trigger_enable_disable(),
> > shouldn't it?
> >
>
> Yeah, that's not done well. I'll fix it up.
>
> Thanks for pointing it out.
Tom,
register_trigger() is messed up. I should have caught this when it was
first submitted, but I'm totally confused. The comments don't match the
code.
First we have this:
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
Which looks to be total BS.
As we have this:
/**
* register_trigger - Generic event_command @reg implementation
* @glob: The raw string used to register the trigger
* @ops: The trigger ops associated with the trigger
* @data: Trigger-specific data to associate with the trigger
* @file: The trace_event_file associated with the event
*
* Common implementation for event trigger registration.
*
* Usually used directly as the @reg method in event command
* implementations.
*
* Return: 0 on success, errno otherwise
*/
static int register_trigger(char *glob, struct event_trigger_ops *ops,
struct event_trigger_data *data,
struct trace_event_file *file)
{
struct event_trigger_data *test;
int ret = 0;
list_for_each_entry_rcu(test, >triggers, list) {
if (test->cmd_ops->trigger_type == data->cmd_ops->trigger_type)
{
ret = -EEXIST;
goto out;
}
}
if (data->ops->init) {
ret = data->ops->init(data->ops, data);
if (ret < 0)
goto out;
}
list_add_rcu(>list, >triggers);
ret++;
update_cond_flag(file);
if (trace_event_trigger_enable_disable(file, 1) < 0) {
list_del_rcu(>list);
update_cond_flag(file);
ret--;
}
out:
return ret;
}
Where the comment is total wrong. It doesn't return 0 on success, it
returns 1. And if trace_event_trigger_enable_disable() fails it returns
zero.
And that can fail with the call->class->reg() return value, which could
fail for various strange reasons. I don't know why we would want to
return 0 when it fails?
I don't see where ->reg() would return anything but 1 on success. Maybe
I'm missing something. I'll look some more, but I'm thinking of changing
->reg() to return zero on all success, and negative on all errors and
just check those results.
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Wed, 25 Jul 2018 00:09:09 +0900
Masami Hiramatsu wrote:
> Hmm, your patch seems to leak a memory since event_trigger_init() will
> be called twice on same trigger_data (Note that event_trigger_init()
> does not init ref counter, but increment it.) So we should decrement
> it when we find it is succeeded. Moreover, if register_trigger()
Good catch, and easily fixed.
> fails before calling data->ops->init() (see -EEXIST case), the ref
> counter will be 0 (-1 +1). But if it fails after data->ops->init(),
> the ref counter will be 1 (-1 +1 +1). It still be unstable.
> (Ah, that means we may have another trouble...)
I'm not sure there's a problem here. I now have:
out_reg:
/* Up the trigger_data count to make sure reg doesn't free it on
failuer */
event_trigger_init(trigger_ops, trigger_data);
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
if (!ret) {
ret = -ENOENT;
} else if (ret > 0)
ret = 0;
/* Down the counter of trigger_data or free it if not used anymore */
event_trigger_free(trigger_ops, trigger_data);
out:
return ret;
Thus we increment trigger_data before calling reg, and free it
afterward. But if reg() did an init too, then the event_trigger_free()
just decs the ref counter.
As for register_trigger()
>
> >
> > P.S. This brings up another minor bug. The failure should return ENOMEM
> > not ENOENT.
>
> Hmm it seems we should review the register_trigger() implementation.
> It should return the return value of trace_event_trigger_enable_disable(),
> shouldn't it?
>
Yeah, that's not done well. I'll fix it up.
Thanks for pointing it out.
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Wed, 25 Jul 2018 00:09:09 +0900
Masami Hiramatsu wrote:
> Hmm, your patch seems to leak a memory since event_trigger_init() will
> be called twice on same trigger_data (Note that event_trigger_init()
> does not init ref counter, but increment it.) So we should decrement
> it when we find it is succeeded. Moreover, if register_trigger()
Good catch, and easily fixed.
> fails before calling data->ops->init() (see -EEXIST case), the ref
> counter will be 0 (-1 +1). But if it fails after data->ops->init(),
> the ref counter will be 1 (-1 +1 +1). It still be unstable.
> (Ah, that means we may have another trouble...)
I'm not sure there's a problem here. I now have:
out_reg:
/* Up the trigger_data count to make sure reg doesn't free it on
failuer */
event_trigger_init(trigger_ops, trigger_data);
ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
/*
* The above returns on success the # of functions enabled,
* but if it didn't find any functions it returns zero.
* Consider no functions a failure too.
*/
if (!ret) {
ret = -ENOENT;
} else if (ret > 0)
ret = 0;
/* Down the counter of trigger_data or free it if not used anymore */
event_trigger_free(trigger_ops, trigger_data);
out:
return ret;
Thus we increment trigger_data before calling reg, and free it
afterward. But if reg() did an init too, then the event_trigger_free()
just decs the ref counter.
As for register_trigger()
>
> >
> > P.S. This brings up another minor bug. The failure should return ENOMEM
> > not ENOENT.
>
> Hmm it seems we should review the register_trigger() implementation.
> It should return the return value of trace_event_trigger_enable_disable(),
> shouldn't it?
>
Yeah, that's not done well. I'll fix it up.
Thanks for pointing it out.
-- Steve
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Mon, 23 Jul 2018 22:10:06 -0400
Steven Rostedt wrote:
> On Sat, 14 Jul 2018 01:27:47 +0900
> Masami Hiramatsu wrote:
>
> > Fix a double free bug of event_trigger_data caused by
> > calling unregister_trigger() from register_snapshot_trigger().
> > This kicks a kernel BUG if double free checker is enabled
> > as below;
> >
> > kernel BUG at /home/mhiramat/ksrc/linux/mm/slub.c:296!
> > invalid opcode: [#1] SMP PTI
> > CPU: 2 PID: 4312 Comm: ftracetest Not tainted 4.18.0-rc1+ #44
> > Hardware name: ASUS All Series/B85M-G, BIOS 2108 08/11/2014
> > RIP: 0010:set_freepointer.part.37+0x0/0x10
> > Code: 41 b8 01 00 00 00 29 c8 4d 8d 0c 0c b9 10 00 00 00 50 e8 e3 28 23 00
> > 8b 53 08 5e 5f 89 d1 81 e1 00 04 00 00 e9 e9 fe ff ff 90 <0f> 0b 0f 1f 40
> > 00 66 2e 0f 1f 84 00 00 00 00 00 48 c7 c6 90 7f 0d
> > RSP: 0018:a799caa3bd90 EFLAGS: 00010246
> > RAX: 9b825f8c8e80 RBX: 9b825f8c8e80 RCX: 9b825f8c8e80
> > RDX: 00021562 RSI: 9b830e9e70e0 RDI: 0202
> > RBP: 0246 R08: 0001 R09:
> > R10: R11: R12: 9b830e0072c0
> > R13: eb8e0d7e3200 R14: 961db7af R15: fffe
> > FS: 7f135ba9f700() GS:9b830e80()
> > knlGS:
> > CS: 0010 DS: ES: CR0: 80050033
> > CR2: 563736b5f3a2 CR3: 000295916005 CR4: 001606e0
> > Call Trace:
> > kfree+0x35d/0x380
> > event_trigger_callback+0x13f/0x1c0
> > event_trigger_write+0xf2/0x1a0
> > ? lock_acquire+0x9f/0x200
> > __vfs_write+0x26/0x170
> > ? rcu_read_lock_sched_held+0x6b/0x80
> > ? rcu_sync_lockdep_assert+0x2e/0x60
> > ? __sb_start_write+0x13e/0x1a0
> > ? vfs_write+0x18a/0x1b0
> > vfs_write+0xc1/0x1b0
> > ksys_write+0x45/0xa0
> > do_syscall_64+0x60/0x200
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > unregister_trigger() will free given event_trigger_data
> > at last. But that event_trigger_data will be freed again
> > in event_trigger_callback() if register_snapshot_trigger()
> > is failed, and causes a double free bug.
> >
> > Registering the data should be the final operation in the
> > register function on normal path, because the trigger
> > must be ready for taking action right after it is
> > registered.
>
> Nice catch. I can reproduce this. But this patch is fixing the symptom
> and not the disease.
>
> >
> > Fixes: commit 93e31ffbf417 ("tracing: Add 'snapshot' event trigger command")
> > Signed-off-by: Masami Hiramatsu
> > Cc: Steven Rostedt
> > Cc: Ingo Molnar
> > Cc: Tom Zanussi
> > Cc: sta...@vger.kernel.org
> > ---
> > kernel/trace/trace.c|5 +
> > kernel/trace/trace.h|2 ++
> > kernel/trace/trace_events_trigger.c | 10 ++
> > 3 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index f054bd6a1c66..2556d8c097d2 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -980,6 +980,11 @@ static void free_snapshot(struct trace_array *tr)
> > tr->allocated_snapshot = false;
> > }
> >
> > +void tracing_free_snapshot_instance(struct trace_array *tr)
> > +{
> > + free_snapshot(tr);
> > +}
> > +
> > /**
> > * tracing_alloc_snapshot - allocate snapshot buffer.
> > *
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index f8f86231ad90..03468bb8a79a 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -1823,12 +1823,14 @@ static inline void trace_event_eval_update(struct
> > trace_eval_map **map, int len)
> > #ifdef CONFIG_TRACER_SNAPSHOT
> > void tracing_snapshot_instance(struct trace_array *tr);
> > int tracing_alloc_snapshot_instance(struct trace_array *tr);
> > +void tracing_free_snapshot_instance(struct trace_array *tr);
> > #else
> > static inline void tracing_snapshot_instance(struct trace_array *tr) { }
> > static inline int tracing_alloc_snapshot_instance(struct trace_array *tr)
> > {
> > return 0;
> > }
> > +static inline void tracing_free_snapshot_instance(struct trace_array *tr)
> > { }
> > #endif
> >
> > extern struct trace_iterator *tracepoint_print_iter;
> > diff --git a/kernel/trace/trace_events_trigger.c
> > b/kernel/trace/trace_events_trigger.c
> > index d18249683682..40e2f4406b2c 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -1079,11 +1079,13 @@ register_snapshot_trigger(char *glob, struct
> > event_trigger_ops *ops,
> > struct event_trigger_data *data,
> > struct trace_event_file *file)
> > {
> > - int ret = register_trigger(glob, ops, data, file);
> > + int free_if_fail = !file->tr->allocated_snapshot;
> > + int ret = 0;
> >
> > - if (ret > 0 && tracing_alloc_snapshot_instance(file->tr) != 0) {
> > - unregister_trigger(glob, ops, data, file);
> > - ret
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Mon, 23 Jul 2018 22:10:06 -0400
Steven Rostedt wrote:
> On Sat, 14 Jul 2018 01:27:47 +0900
> Masami Hiramatsu wrote:
>
> > Fix a double free bug of event_trigger_data caused by
> > calling unregister_trigger() from register_snapshot_trigger().
> > This kicks a kernel BUG if double free checker is enabled
> > as below;
> >
> > kernel BUG at /home/mhiramat/ksrc/linux/mm/slub.c:296!
> > invalid opcode: [#1] SMP PTI
> > CPU: 2 PID: 4312 Comm: ftracetest Not tainted 4.18.0-rc1+ #44
> > Hardware name: ASUS All Series/B85M-G, BIOS 2108 08/11/2014
> > RIP: 0010:set_freepointer.part.37+0x0/0x10
> > Code: 41 b8 01 00 00 00 29 c8 4d 8d 0c 0c b9 10 00 00 00 50 e8 e3 28 23 00
> > 8b 53 08 5e 5f 89 d1 81 e1 00 04 00 00 e9 e9 fe ff ff 90 <0f> 0b 0f 1f 40
> > 00 66 2e 0f 1f 84 00 00 00 00 00 48 c7 c6 90 7f 0d
> > RSP: 0018:a799caa3bd90 EFLAGS: 00010246
> > RAX: 9b825f8c8e80 RBX: 9b825f8c8e80 RCX: 9b825f8c8e80
> > RDX: 00021562 RSI: 9b830e9e70e0 RDI: 0202
> > RBP: 0246 R08: 0001 R09:
> > R10: R11: R12: 9b830e0072c0
> > R13: eb8e0d7e3200 R14: 961db7af R15: fffe
> > FS: 7f135ba9f700() GS:9b830e80()
> > knlGS:
> > CS: 0010 DS: ES: CR0: 80050033
> > CR2: 563736b5f3a2 CR3: 000295916005 CR4: 001606e0
> > Call Trace:
> > kfree+0x35d/0x380
> > event_trigger_callback+0x13f/0x1c0
> > event_trigger_write+0xf2/0x1a0
> > ? lock_acquire+0x9f/0x200
> > __vfs_write+0x26/0x170
> > ? rcu_read_lock_sched_held+0x6b/0x80
> > ? rcu_sync_lockdep_assert+0x2e/0x60
> > ? __sb_start_write+0x13e/0x1a0
> > ? vfs_write+0x18a/0x1b0
> > vfs_write+0xc1/0x1b0
> > ksys_write+0x45/0xa0
> > do_syscall_64+0x60/0x200
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > unregister_trigger() will free given event_trigger_data
> > at last. But that event_trigger_data will be freed again
> > in event_trigger_callback() if register_snapshot_trigger()
> > is failed, and causes a double free bug.
> >
> > Registering the data should be the final operation in the
> > register function on normal path, because the trigger
> > must be ready for taking action right after it is
> > registered.
>
> Nice catch. I can reproduce this. But this patch is fixing the symptom
> and not the disease.
>
> >
> > Fixes: commit 93e31ffbf417 ("tracing: Add 'snapshot' event trigger command")
> > Signed-off-by: Masami Hiramatsu
> > Cc: Steven Rostedt
> > Cc: Ingo Molnar
> > Cc: Tom Zanussi
> > Cc: sta...@vger.kernel.org
> > ---
> > kernel/trace/trace.c|5 +
> > kernel/trace/trace.h|2 ++
> > kernel/trace/trace_events_trigger.c | 10 ++
> > 3 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index f054bd6a1c66..2556d8c097d2 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -980,6 +980,11 @@ static void free_snapshot(struct trace_array *tr)
> > tr->allocated_snapshot = false;
> > }
> >
> > +void tracing_free_snapshot_instance(struct trace_array *tr)
> > +{
> > + free_snapshot(tr);
> > +}
> > +
> > /**
> > * tracing_alloc_snapshot - allocate snapshot buffer.
> > *
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index f8f86231ad90..03468bb8a79a 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -1823,12 +1823,14 @@ static inline void trace_event_eval_update(struct
> > trace_eval_map **map, int len)
> > #ifdef CONFIG_TRACER_SNAPSHOT
> > void tracing_snapshot_instance(struct trace_array *tr);
> > int tracing_alloc_snapshot_instance(struct trace_array *tr);
> > +void tracing_free_snapshot_instance(struct trace_array *tr);
> > #else
> > static inline void tracing_snapshot_instance(struct trace_array *tr) { }
> > static inline int tracing_alloc_snapshot_instance(struct trace_array *tr)
> > {
> > return 0;
> > }
> > +static inline void tracing_free_snapshot_instance(struct trace_array *tr)
> > { }
> > #endif
> >
> > extern struct trace_iterator *tracepoint_print_iter;
> > diff --git a/kernel/trace/trace_events_trigger.c
> > b/kernel/trace/trace_events_trigger.c
> > index d18249683682..40e2f4406b2c 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -1079,11 +1079,13 @@ register_snapshot_trigger(char *glob, struct
> > event_trigger_ops *ops,
> > struct event_trigger_data *data,
> > struct trace_event_file *file)
> > {
> > - int ret = register_trigger(glob, ops, data, file);
> > + int free_if_fail = !file->tr->allocated_snapshot;
> > + int ret = 0;
> >
> > - if (ret > 0 && tracing_alloc_snapshot_instance(file->tr) != 0) {
> > - unregister_trigger(glob, ops, data, file);
> > - ret
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Sat, 14 Jul 2018 01:27:47 +0900
Masami Hiramatsu wrote:
> Fix a double free bug of event_trigger_data caused by
> calling unregister_trigger() from register_snapshot_trigger().
> This kicks a kernel BUG if double free checker is enabled
> as below;
>
> kernel BUG at /home/mhiramat/ksrc/linux/mm/slub.c:296!
> invalid opcode: [#1] SMP PTI
> CPU: 2 PID: 4312 Comm: ftracetest Not tainted 4.18.0-rc1+ #44
> Hardware name: ASUS All Series/B85M-G, BIOS 2108 08/11/2014
> RIP: 0010:set_freepointer.part.37+0x0/0x10
> Code: 41 b8 01 00 00 00 29 c8 4d 8d 0c 0c b9 10 00 00 00 50 e8 e3 28 23 00
> 8b 53 08 5e 5f 89 d1 81 e1 00 04 00 00 e9 e9 fe ff ff 90 <0f> 0b 0f 1f 40 00
> 66 2e 0f 1f 84 00 00 00 00 00 48 c7 c6 90 7f 0d
> RSP: 0018:a799caa3bd90 EFLAGS: 00010246
> RAX: 9b825f8c8e80 RBX: 9b825f8c8e80 RCX: 9b825f8c8e80
> RDX: 00021562 RSI: 9b830e9e70e0 RDI: 0202
> RBP: 0246 R08: 0001 R09:
> R10: R11: R12: 9b830e0072c0
> R13: eb8e0d7e3200 R14: 961db7af R15: fffe
> FS: 7f135ba9f700() GS:9b830e80() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 563736b5f3a2 CR3: 000295916005 CR4: 001606e0
> Call Trace:
> kfree+0x35d/0x380
> event_trigger_callback+0x13f/0x1c0
> event_trigger_write+0xf2/0x1a0
> ? lock_acquire+0x9f/0x200
> __vfs_write+0x26/0x170
> ? rcu_read_lock_sched_held+0x6b/0x80
> ? rcu_sync_lockdep_assert+0x2e/0x60
> ? __sb_start_write+0x13e/0x1a0
> ? vfs_write+0x18a/0x1b0
> vfs_write+0xc1/0x1b0
> ksys_write+0x45/0xa0
> do_syscall_64+0x60/0x200
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> unregister_trigger() will free given event_trigger_data
> at last. But that event_trigger_data will be freed again
> in event_trigger_callback() if register_snapshot_trigger()
> is failed, and causes a double free bug.
>
> Registering the data should be the final operation in the
> register function on normal path, because the trigger
> must be ready for taking action right after it is
> registered.
Nice catch. I can reproduce this. But this patch is fixing the symptom
and not the disease.
>
> Fixes: commit 93e31ffbf417 ("tracing: Add 'snapshot' event trigger command")
> Signed-off-by: Masami Hiramatsu
> Cc: Steven Rostedt
> Cc: Ingo Molnar
> Cc: Tom Zanussi
> Cc: sta...@vger.kernel.org
> ---
> kernel/trace/trace.c|5 +
> kernel/trace/trace.h|2 ++
> kernel/trace/trace_events_trigger.c | 10 ++
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index f054bd6a1c66..2556d8c097d2 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -980,6 +980,11 @@ static void free_snapshot(struct trace_array *tr)
> tr->allocated_snapshot = false;
> }
>
> +void tracing_free_snapshot_instance(struct trace_array *tr)
> +{
> + free_snapshot(tr);
> +}
> +
> /**
> * tracing_alloc_snapshot - allocate snapshot buffer.
> *
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index f8f86231ad90..03468bb8a79a 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1823,12 +1823,14 @@ static inline void trace_event_eval_update(struct
> trace_eval_map **map, int len)
> #ifdef CONFIG_TRACER_SNAPSHOT
> void tracing_snapshot_instance(struct trace_array *tr);
> int tracing_alloc_snapshot_instance(struct trace_array *tr);
> +void tracing_free_snapshot_instance(struct trace_array *tr);
> #else
> static inline void tracing_snapshot_instance(struct trace_array *tr) { }
> static inline int tracing_alloc_snapshot_instance(struct trace_array *tr)
> {
> return 0;
> }
> +static inline void tracing_free_snapshot_instance(struct trace_array *tr) { }
> #endif
>
> extern struct trace_iterator *tracepoint_print_iter;
> diff --git a/kernel/trace/trace_events_trigger.c
> b/kernel/trace/trace_events_trigger.c
> index d18249683682..40e2f4406b2c 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -1079,11 +1079,13 @@ register_snapshot_trigger(char *glob, struct
> event_trigger_ops *ops,
> struct event_trigger_data *data,
> struct trace_event_file *file)
> {
> - int ret = register_trigger(glob, ops, data, file);
> + int free_if_fail = !file->tr->allocated_snapshot;
> + int ret = 0;
>
> - if (ret > 0 && tracing_alloc_snapshot_instance(file->tr) != 0) {
> - unregister_trigger(glob, ops, data, file);
> - ret = 0;
> + if (!tracing_alloc_snapshot_instance(file->tr)) {
> + ret = register_trigger(glob, ops, data, file);
> + if (ret == 0 && free_if_fail)
> + tracing_free_snapshot_instance(file->tr);
> }
>
> return ret;
Really,
Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data
On Sat, 14 Jul 2018 01:27:47 +0900
Masami Hiramatsu wrote:
> Fix a double free bug of event_trigger_data caused by
> calling unregister_trigger() from register_snapshot_trigger().
> This kicks a kernel BUG if double free checker is enabled
> as below;
>
> kernel BUG at /home/mhiramat/ksrc/linux/mm/slub.c:296!
> invalid opcode: [#1] SMP PTI
> CPU: 2 PID: 4312 Comm: ftracetest Not tainted 4.18.0-rc1+ #44
> Hardware name: ASUS All Series/B85M-G, BIOS 2108 08/11/2014
> RIP: 0010:set_freepointer.part.37+0x0/0x10
> Code: 41 b8 01 00 00 00 29 c8 4d 8d 0c 0c b9 10 00 00 00 50 e8 e3 28 23 00
> 8b 53 08 5e 5f 89 d1 81 e1 00 04 00 00 e9 e9 fe ff ff 90 <0f> 0b 0f 1f 40 00
> 66 2e 0f 1f 84 00 00 00 00 00 48 c7 c6 90 7f 0d
> RSP: 0018:a799caa3bd90 EFLAGS: 00010246
> RAX: 9b825f8c8e80 RBX: 9b825f8c8e80 RCX: 9b825f8c8e80
> RDX: 00021562 RSI: 9b830e9e70e0 RDI: 0202
> RBP: 0246 R08: 0001 R09:
> R10: R11: R12: 9b830e0072c0
> R13: eb8e0d7e3200 R14: 961db7af R15: fffe
> FS: 7f135ba9f700() GS:9b830e80() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 563736b5f3a2 CR3: 000295916005 CR4: 001606e0
> Call Trace:
> kfree+0x35d/0x380
> event_trigger_callback+0x13f/0x1c0
> event_trigger_write+0xf2/0x1a0
> ? lock_acquire+0x9f/0x200
> __vfs_write+0x26/0x170
> ? rcu_read_lock_sched_held+0x6b/0x80
> ? rcu_sync_lockdep_assert+0x2e/0x60
> ? __sb_start_write+0x13e/0x1a0
> ? vfs_write+0x18a/0x1b0
> vfs_write+0xc1/0x1b0
> ksys_write+0x45/0xa0
> do_syscall_64+0x60/0x200
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> unregister_trigger() will free given event_trigger_data
> at last. But that event_trigger_data will be freed again
> in event_trigger_callback() if register_snapshot_trigger()
> is failed, and causes a double free bug.
>
> Registering the data should be the final operation in the
> register function on normal path, because the trigger
> must be ready for taking action right after it is
> registered.
Nice catch. I can reproduce this. But this patch is fixing the symptom
and not the disease.
>
> Fixes: commit 93e31ffbf417 ("tracing: Add 'snapshot' event trigger command")
> Signed-off-by: Masami Hiramatsu
> Cc: Steven Rostedt
> Cc: Ingo Molnar
> Cc: Tom Zanussi
> Cc: sta...@vger.kernel.org
> ---
> kernel/trace/trace.c|5 +
> kernel/trace/trace.h|2 ++
> kernel/trace/trace_events_trigger.c | 10 ++
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index f054bd6a1c66..2556d8c097d2 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -980,6 +980,11 @@ static void free_snapshot(struct trace_array *tr)
> tr->allocated_snapshot = false;
> }
>
> +void tracing_free_snapshot_instance(struct trace_array *tr)
> +{
> + free_snapshot(tr);
> +}
> +
> /**
> * tracing_alloc_snapshot - allocate snapshot buffer.
> *
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index f8f86231ad90..03468bb8a79a 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1823,12 +1823,14 @@ static inline void trace_event_eval_update(struct
> trace_eval_map **map, int len)
> #ifdef CONFIG_TRACER_SNAPSHOT
> void tracing_snapshot_instance(struct trace_array *tr);
> int tracing_alloc_snapshot_instance(struct trace_array *tr);
> +void tracing_free_snapshot_instance(struct trace_array *tr);
> #else
> static inline void tracing_snapshot_instance(struct trace_array *tr) { }
> static inline int tracing_alloc_snapshot_instance(struct trace_array *tr)
> {
> return 0;
> }
> +static inline void tracing_free_snapshot_instance(struct trace_array *tr) { }
> #endif
>
> extern struct trace_iterator *tracepoint_print_iter;
> diff --git a/kernel/trace/trace_events_trigger.c
> b/kernel/trace/trace_events_trigger.c
> index d18249683682..40e2f4406b2c 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -1079,11 +1079,13 @@ register_snapshot_trigger(char *glob, struct
> event_trigger_ops *ops,
> struct event_trigger_data *data,
> struct trace_event_file *file)
> {
> - int ret = register_trigger(glob, ops, data, file);
> + int free_if_fail = !file->tr->allocated_snapshot;
> + int ret = 0;
>
> - if (ret > 0 && tracing_alloc_snapshot_instance(file->tr) != 0) {
> - unregister_trigger(glob, ops, data, file);
> - ret = 0;
> + if (!tracing_alloc_snapshot_instance(file->tr)) {
> + ret = register_trigger(glob, ops, data, file);
> + if (ret == 0 && free_if_fail)
> + tracing_free_snapshot_instance(file->tr);
> }
>
> return ret;
Really,
