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

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

[Francisco Jerez]

[-- Attachment #1.1: Type: text/plain, Size: 1810 bytes --]

Looks okay to me, I'll squash this into the original patch.

Julia Lawall <julia.lawall@lip6.fr> 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 <currojerez@riseup.net>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
> ---
>
> 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;
>  	}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[PATCH] OPTIONAL: cpufreq/intel_pstate: fix debugfs_simple_attr.cocci warnings

[Julia Lawall]

 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 <currojerez@riseup.net>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
---

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;
 	}

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

[Francisco Jerez]

[-- Attachment #1.1: Type: text/plain, Size: 731 bytes --]

Fabio Estevam <festevam@gmail.com> writes:

> 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?
>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

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

[Fabio Estevam]

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?

Thanks

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

[Julia Lawall]

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?

thanks,
julia

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

[Nicolai Stange]

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,

Nicolai


-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)

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

[Julia Lawall]

[-- 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)
>

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

[Rafael J. Wysocki]

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 <currojerez@riseup.net>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
> ---
> 
> 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;
>  	}
> 

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

[Fabio Estevam]

On Fri, Mar 30, 2018 at 3:22 AM, Julia Lawall <julia.lawall@lip6.fr> 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

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

[Nicolai Stange]

Julia Lawall <julia.lawall@lip6.fr> writes:

> 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.

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)