From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751254AbeC3GXC (ORCPT ); Fri, 30 Mar 2018 02:23:02 -0400 Received: from mail3-relais-sop.national.inria.fr ([192.134.164.104]:17465 "EHLO mail3-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750725AbeC3GXA (ORCPT ); Fri, 30 Mar 2018 02:23:00 -0400 X-IronPort-AV: E=Sophos;i="5.48,380,1517871600"; d="scan'208";a="260355671" Date: Fri, 30 Mar 2018 08:22:58 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@hadrien To: Nicolai Stange cc: Fabio Estevam , Francisco Jerez , linux-pm@vger.kernel.org, Viresh Kumar , "Rafael J. Wysocki" , linux-kernel , kbuild-all@01.org, Srinivas Pandruvada , 0day robot , Len Brown Subject: Re: [kbuild-all] [PATCH] OPTIONAL: cpufreq/intel_pstate: fix debugfs_simple_attr.cocci warnings In-Reply-To: <87po3mxf73.fsf@suse.de> Message-ID: References: <87po3mxf73.fsf@suse.de> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-449675951-1522390979=:2870" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --8323329-449675951-1522390979=:2870 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Fri, 30 Mar 2018, Nicolai Stange wrote: > Julia Lawall writes: > > > On Thu, 29 Mar 2018, Fabio Estevam wrote: > > > >> Hi Julia, > >> > >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall wrote: > >> > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE > >> > for debugfs files. > >> > > >> > Semantic patch information: > >> > Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file() > >> > imposes some significant overhead as compared to > >> > DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe(). > >> > >> Just curious: could you please expand on what "imposes some > >> significant overhead" means? > > > > I don't know. I didn't write this rule. Nicolai, can you explain? > > From commit 49d200deaa68 ("debugfs: prevent access to removed files' private > data"): > > Upon return of debugfs_remove()/debugfs_remove_recursive(), it might > still be attempted to access associated private file data through > previously opened struct file objects. If that data has been freed by > the caller of debugfs_remove*() in the meanwhile, the reading/writing > process would either encounter a fault or, if the memory address in > question has been reassigned again, unrelated data structures could get > overwritten. > [...] > Currently, there are ~1000 call sites of debugfs_create_file() spread > throughout the whole tree and touching all of those struct file_operations > in order to make them file removal aware by means of checking the result of > debugfs_use_file_start() from within their methods is unfeasible. > > Instead, wrap the struct file_operations by a lifetime managing proxy at > file open [...] > > The additional overhead comes in terms of additional memory needed: for > debugs files created through debugfs_create_file(), one such struct > file_operations proxy is allocated for each struct file instantiation, > c.f. full_proxy_open(). > > This was needed to "repair" the ~1000 call sites without touching them. > > New debugfs users should make their file_operations removal aware > themselves by means of DEFINE_DEBUGFS_ATTRIBUTE() and signal that fact to > the debugfs core by instantiating them through > debugfs_create_file_unsafe(). > > See commit c64688081490 ("debugfs: add support for self-protecting > attribute file fops") for further information. Thanks. Perhaps it would be good to add a reference to this commit in the message generated by the semantic patch. Would it be sufficient to just apply the semantic patch everywhere and submit the patches? julia > > > Thanks, > > Nicolai > > > -- > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, > HRB 21284 (AG Nürnberg) > --8323329-449675951-1522390979=:2870--