Re: [kbuild-all] [PATCH] OPTIONAL: cpufreq/intel_pstate: fix debugfs_simple_attr.cocci warnings

From: Julia Lawall <julia.lawall@lip6.fr>
To: Nicolai Stange <nstange@suse.de>
Cc: Fabio Estevam <festevam@gmail.com>,
	Francisco Jerez <currojerez@riseup.net>,
	linux-pm@vger.kernel.org, Viresh Kumar <viresh.kumar@linaro.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	kbuild-all@01.org,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	0day robot <fengguang.wu@intel.com>, Len Brown <lenb@kernel.org>
Subject: Re: [kbuild-all] [PATCH] OPTIONAL: cpufreq/intel_pstate: fix debugfs_simple_attr.cocci warnings
Date: Fri, 30 Mar 2018 08:22:58 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.20.1803300821330.2870@hadrien> (raw)
In-Reply-To: <87po3mxf73.fsf@suse.de>

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On Fri, 30 Mar 2018, Nicolai Stange wrote:

> Julia Lawall <julia.lawall@lip6.fr> writes:
>
> > On Thu, 29 Mar 2018, Fabio Estevam wrote:
> >
> >> Hi Julia,
> >>
> >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <julia.lawall@lip6.fr> 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)
>