From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com ([134.134.136.100]:7091 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729159AbeGYRNr (ORCPT ); Wed, 25 Jul 2018 13:13:47 -0400 Subject: Re: [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data To: Steven Rostedt Cc: Masami Hiramatsu , Ingo Molnar , Shuah Khan , Hiraku Toyooka , linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <153149923649.11274.14970833360963898112.stgit@devbox> <153149926702.11274.12489440326560729788.stgit@devbox> <20180723221006.60cc7aa9@vmware.local.home> <20180725000909.6c8b2f3881ee75c4f6bd466b@kernel.org> <20180724164959.3cbc1422@gandalf.local.home> <20180724173008.454cdf10@gandalf.local.home> From: Tom Zanussi Message-ID: Date: Wed, 25 Jul 2018 11:01:22 -0500 MIME-Version: 1.0 In-Reply-To: <20180724173008.454cdf10@gandalf.local.home> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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, &file->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(&data->list, &file->triggers); > ret++; > > update_cond_flag(file); > if (trace_event_trigger_enable_disable(file, 1) < 0) { > list_del_rcu(&data->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 >