From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755271AbbHQMvc (ORCPT ); Mon, 17 Aug 2015 08:51:32 -0400 Received: from mga01.intel.com ([192.55.52.88]:12319 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754225AbbHQMvb (ORCPT ); Mon, 17 Aug 2015 08:51:31 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,694,1432623600"; d="scan'208";a="770293226" From: Alexander Shishkin To: Peter Zijlstra Cc: Ingo Molnar , linux-kernel@vger.kernel.org, adrian.hunter@intel.com, Arnaldo Carvalho de Melo , Vince Weaver , Stephane Eranian Subject: Re: [PATCH RFC v1 1/4] perf: Introduce extended syscall error reporting In-Reply-To: <20150805154553.GN19282@twins.programming.kicks-ass.net> References: <1437738359-23920-1-git-send-email-alexander.shishkin@linux.intel.com> <1437738359-23920-2-git-send-email-alexander.shishkin@linux.intel.com> <20150805154553.GN19282@twins.programming.kicks-ass.net> User-Agent: Notmuch/0.20.2 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Mon, 17 Aug 2015 15:51:06 +0300 Message-ID: <87614ejcxx.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Peter Zijlstra writes: > On Fri, Jul 24, 2015 at 02:45:56PM +0300, Alexander Shishkin wrote: >> +static void perf_error_report_site(struct perf_event_attr *attr, >> + const struct perf_err_site *site) >> +{ >> + void *buffer; >> + >> + if (!site || !extended_reporting_enabled(attr)) >> + return; >> + >> + /* in case of nested perf_err()s, which you shouldn't really do */ >> + while (site->code <= -PERF_ERRNO) >> + site = perf_errno_to_site(site->code); >> + >> + buffer = kasprintf(GFP_KERNEL, >> + "{\n" >> + "\t\"code\": %d,\n" >> + "\t\"module\": \"%s\",\n" >> + "\t\"message\": \"%s\"\n" >> + "}\n", >> + site->code, site->owner, site->message >> + ); >> + if (!buffer) >> + return; >> + >> + if (copy_to_user((void __user *)attr->perf_err, buffer, >> + attr->perf_err_size)) { > > Should that not be min(attr->perf_err_size, strlen(buffer)) ? Indeed. > Also, should we not '\0' the last char in attr->perf_err in case buffer > is longer. I'm guessing we should, because otherwise the user is likely to get all confused. They can tell, however, if the buffer they allocated was too small by looking for (and not finding) the closing bracket. > >> + /* if we failed to copy once, don't bother later */ >> + attr->perf_err_size = 0; >> + } > > So we want update the user's perf_err_size with the actual size we > copied in? We *could* do that for the perf_event_open() case where we have the user's copy of the attribute and can mess around with it, but as you noticed we're also using this error reporting in the other syscalls that involve perf events and with those we won't have the user's attribute any more. Regards, -- Alex