From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E1717C67790 for ; Wed, 25 Jul 2018 16:01:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A2C1120891 for ; Wed, 25 Jul 2018 16:01:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A2C1120891 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729285AbeGYRNr (ORCPT ); Wed, 25 Jul 2018 13:13:47 -0400 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 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jul 2018 09:01:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,401,1526367600"; d="scan'208";a="60603989" Received: from tzanussi-mobl2.amr.corp.intel.com (HELO [10.252.195.127]) ([10.252.195.127]) by orsmga006.jf.intel.com with ESMTP; 25 Jul 2018 09:01:23 -0700 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 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 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: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >