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().
Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
Fixes: 9eec7989e762 ("OPTIONAL: cpufreq/intel_pstate: Expose LP controller parameters via debugfs.")
CC: Francisco Jerez <[email protected]>
Signed-off-by: Fengguang Wu <[email protected]>
Signed-off-by: Julia Lawall <[email protected]>
---
I don't actually know anything about this issue. The change was suggested
by kbuild.
intel_pstate.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -885,7 +885,7 @@ static int lp_param_get(void *data, u64
*val = *(u32 *)data;
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n");
static struct dentry *debugfs_parent;
@@ -922,9 +922,10 @@ static void intel_pstate_debug_expose_pa
for (i = 0; lp_files[i].name; i++) {
struct dentry *dentry;
- dentry = debugfs_create_file(lp_files[i].name, 0660,
- debugfs_parent, lp_files[i].value,
- &fops_lp_param);
+ dentry = debugfs_create_file_unsafe(lp_files[i].name, 0660,
+ debugfs_parent,
+ lp_files[i].value,
+ &fops_lp_param);
if (!IS_ERR(dentry))
lp_files[i].dentry = dentry;
}
Looks okay to me, I'll squash this into the original patch.
Julia Lawall <[email protected]> writes:
> 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().
>
> Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>
> Fixes: 9eec7989e762 ("OPTIONAL: cpufreq/intel_pstate: Expose LP controller parameters via debugfs.")
> CC: Francisco Jerez <[email protected]>
> Signed-off-by: Fengguang Wu <[email protected]>
> Signed-off-by: Julia Lawall <[email protected]>
> ---
>
> I don't actually know anything about this issue. The change was suggested
> by kbuild.
>
> intel_pstate.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -885,7 +885,7 @@ static int lp_param_get(void *data, u64
> *val = *(u32 *)data;
> return 0;
> }
> -DEFINE_SIMPLE_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n");
>
> static struct dentry *debugfs_parent;
>
> @@ -922,9 +922,10 @@ static void intel_pstate_debug_expose_pa
> for (i = 0; lp_files[i].name; i++) {
> struct dentry *dentry;
>
> - dentry = debugfs_create_file(lp_files[i].name, 0660,
> - debugfs_parent, lp_files[i].value,
> - &fops_lp_param);
> + dentry = debugfs_create_file_unsafe(lp_files[i].name, 0660,
> + debugfs_parent,
> + lp_files[i].value,
> + &fops_lp_param);
> if (!IS_ERR(dentry))
> lp_files[i].dentry = dentry;
> }
Hi Julia,
On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <[email protected]> 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?
Thanks
Fabio Estevam <[email protected]> writes:
> Hi Julia,
>
> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <[email protected]> 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?
>
Probably negligible given that this code will only be run once at system
boot and then never used again in production systems. But I guess the
micro-optimization doesn't hurt either.
> Thanks
On Thu, 29 Mar 2018, Fabio Estevam wrote:
> Hi Julia,
>
> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <[email protected]> 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?
thanks,
julia
Julia Lawall <[email protected]> writes:
> On Thu, 29 Mar 2018, Fabio Estevam wrote:
>
>> Hi Julia,
>>
>> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <[email protected]> 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,
Nicolai
--
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)
On Fri, 30 Mar 2018, Nicolai Stange wrote:
> Julia Lawall <[email protected]> writes:
>
> > On Thu, 29 Mar 2018, Fabio Estevam wrote:
> >
> >> Hi Julia,
> >>
> >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <[email protected]> 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)
>
Hi Julia,
On Thursday, March 29, 2018 9:12:06 PM CEST 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().
>
> Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
We've dropped the debugfs bits from intel_pstate entirely, so this change
is not applicable any more.
Thanks!
> Fixes: 9eec7989e762 ("OPTIONAL: cpufreq/intel_pstate: Expose LP controller parameters via debugfs.")
> CC: Francisco Jerez <[email protected]>
> Signed-off-by: Fengguang Wu <[email protected]>
> Signed-off-by: Julia Lawall <[email protected]>
> ---
>
> I don't actually know anything about this issue. The change was suggested
> by kbuild.
>
> intel_pstate.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -885,7 +885,7 @@ static int lp_param_get(void *data, u64
> *val = *(u32 *)data;
> return 0;
> }
> -DEFINE_SIMPLE_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_lp_param, lp_param_get, lp_param_set, "%llu\n");
>
> static struct dentry *debugfs_parent;
>
> @@ -922,9 +922,10 @@ static void intel_pstate_debug_expose_pa
> for (i = 0; lp_files[i].name; i++) {
> struct dentry *dentry;
>
> - dentry = debugfs_create_file(lp_files[i].name, 0660,
> - debugfs_parent, lp_files[i].value,
> - &fops_lp_param);
> + dentry = debugfs_create_file_unsafe(lp_files[i].name, 0660,
> + debugfs_parent,
> + lp_files[i].value,
> + &fops_lp_param);
> if (!IS_ERR(dentry))
> lp_files[i].dentry = dentry;
> }
>
On Fri, Mar 30, 2018 at 3:22 AM, Julia Lawall <[email protected]> wrote:
>> 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 for the detailed explanation, Nicolai!
> Thanks. Perhaps it would be good to add a reference to this commit in
> the message generated by the semantic patch.
Yes, that would be very helpful indeed.
Thanks
Julia Lawall <[email protected]> writes:
> On Fri, 30 Mar 2018, Nicolai Stange wrote:
>
>> Julia Lawall <[email protected]> writes:
>>
>> > On Thu, 29 Mar 2018, Fabio Estevam wrote:
>> >
>> >> Hi Julia,
>> >>
>> >> On Thu, Mar 29, 2018 at 4:12 PM, Julia Lawall <[email protected]> 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.
Thanks for doing this!
>
> Would it be sufficient to just apply the semantic patch everywhere and
> submit the patches?
In principle yes. But I'm note sure whether such a mass application is
worth it: the proxy allocation happens only at file open and the
expectation is that there aren't that many debugfs files kept open at a
time. OTOH, a struct file_operation consumes 256 bytes with
sizeof(long) == 8.
In my opinion, new users should avoid this overhead as it's easily
doable. For existing ones, I don't know.
Thanks,
Nicolai
--
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)