[PATCH] tracing: use kstrdup_const instead of private implementation

[PATCH] tracing: use kstrdup_const instead of private implementation

[Rasmus Villemoes]

The kernel now has kstrdup_const/kfree_const for reusing .rodata
(typically string literals) when possible; there's no reason to
duplicate that logic in the tracing system. Moreover, as the comment
above core_kernel_data states, it may not always return true for
.rodata - that is for example the case on x86_64, where we thus end up
kstrdup'ing all the passed-in strings.

Arguably, testing for .rodata explicitly (as kstrdup_const does) is
also more correct: I don't think one is supposed to be able to change
the name after creating the event_subsystem by passing the address of
a static char (but non-const) array.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 kernel/trace/trace_events.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 7ca09cdc20c2..ae97d73b31fa 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -38,21 +38,19 @@ static LIST_HEAD(ftrace_common_fields);
 static struct kmem_cache *field_cachep;
 static struct kmem_cache *file_cachep;
 
-#define SYSTEM_FL_FREE_NAME		(1 << 31)
-
 static inline int system_refcount(struct event_subsystem *system)
 {
-	return system->ref_count & ~SYSTEM_FL_FREE_NAME;
+	return system->ref_count;
 }
 
 static int system_refcount_inc(struct event_subsystem *system)
 {
-	return (system->ref_count++) & ~SYSTEM_FL_FREE_NAME;
+	return system->ref_count++;
 }
 
 static int system_refcount_dec(struct event_subsystem *system)
 {
-	return (--system->ref_count) & ~SYSTEM_FL_FREE_NAME;
+	return --system->ref_count;
 }
 
 /* Double loops, do not use break, only goto's work */
@@ -460,8 +458,7 @@ static void __put_system(struct event_subsystem *system)
 		kfree(filter->filter_string);
 		kfree(filter);
 	}
-	if (system->ref_count & SYSTEM_FL_FREE_NAME)
-		kfree(system->name);
+	kfree_const(system->name);
 	kfree(system);
 }
 
@@ -1492,13 +1489,9 @@ create_new_subsystem(const char *name)
 	system->ref_count = 1;
 
 	/* Only allocate if dynamic (kprobes and modules) */
-	if (!core_kernel_data((unsigned long)name)) {
-		system->ref_count |= SYSTEM_FL_FREE_NAME;
-		system->name = kstrdup(name, GFP_KERNEL);
-		if (!system->name)
-			goto out_free;
-	} else
-		system->name = name;
+	system->name = kstrdup_const(name, GFP_KERNEL);
+	if (!system->name)
+		goto out_free;
 
 	system->filter = NULL;
 
@@ -1511,8 +1504,7 @@ create_new_subsystem(const char *name)
 	return system;
 
  out_free:
-	if (system->ref_count & SYSTEM_FL_FREE_NAME)
-		kfree(system->name);
+	kfree_const(system->name);
 	kfree(system);
 	return NULL;
 }
-- 
2.1.3

Re: [PATCH] tracing: use kstrdup_const instead of private implementation

[Rasmus Villemoes]

On Wed, Sep 09 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

ping

> The kernel now has kstrdup_const/kfree_const for reusing .rodata
> (typically string literals) when possible; there's no reason to
> duplicate that logic in the tracing system. Moreover, as the comment
> above core_kernel_data states, it may not always return true for
> .rodata - that is for example the case on x86_64, where we thus end up
> kstrdup'ing all the passed-in strings.
>
> Arguably, testing for .rodata explicitly (as kstrdup_const does) is
> also more correct: I don't think one is supposed to be able to change
> the name after creating the event_subsystem by passing the address of
> a static char (but non-const) array.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  kernel/trace/trace_events.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 7ca09cdc20c2..ae97d73b31fa 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -38,21 +38,19 @@ static LIST_HEAD(ftrace_common_fields);
>  static struct kmem_cache *field_cachep;
>  static struct kmem_cache *file_cachep;
>  
> -#define SYSTEM_FL_FREE_NAME		(1 << 31)
> -
>  static inline int system_refcount(struct event_subsystem *system)
>  {
> -	return system->ref_count & ~SYSTEM_FL_FREE_NAME;
> +	return system->ref_count;
>  }
>  
>  static int system_refcount_inc(struct event_subsystem *system)
>  {
> -	return (system->ref_count++) & ~SYSTEM_FL_FREE_NAME;
> +	return system->ref_count++;
>  }
>  
>  static int system_refcount_dec(struct event_subsystem *system)
>  {
> -	return (--system->ref_count) & ~SYSTEM_FL_FREE_NAME;
> +	return --system->ref_count;
>  }
>  
>  /* Double loops, do not use break, only goto's work */
> @@ -460,8 +458,7 @@ static void __put_system(struct event_subsystem *system)
>  		kfree(filter->filter_string);
>  		kfree(filter);
>  	}
> -	if (system->ref_count & SYSTEM_FL_FREE_NAME)
> -		kfree(system->name);
> +	kfree_const(system->name);
>  	kfree(system);
>  }
>  
> @@ -1492,13 +1489,9 @@ create_new_subsystem(const char *name)
>  	system->ref_count = 1;
>  
>  	/* Only allocate if dynamic (kprobes and modules) */
> -	if (!core_kernel_data((unsigned long)name)) {
> -		system->ref_count |= SYSTEM_FL_FREE_NAME;
> -		system->name = kstrdup(name, GFP_KERNEL);
> -		if (!system->name)
> -			goto out_free;
> -	} else
> -		system->name = name;
> +	system->name = kstrdup_const(name, GFP_KERNEL);
> +	if (!system->name)
> +		goto out_free;
>  
>  	system->filter = NULL;
>  
> @@ -1511,8 +1504,7 @@ create_new_subsystem(const char *name)
>  	return system;
>  
>   out_free:
> -	if (system->ref_count & SYSTEM_FL_FREE_NAME)
> -		kfree(system->name);
> +	kfree_const(system->name);
>  	kfree(system);
>  	return NULL;
>  }

Re: [PATCH] tracing: use kstrdup_const instead of private implementation

[Steven Rostedt]

On Wed, 30 Sep 2015 21:43:00 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Wed, Sep 09 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> ping

Thanks for the reminder. I'll take a look at your patches today.

-- Steve

> 
> > The kernel now has kstrdup_const/kfree_const for reusing .rodata
> > (typically string literals) when possible; there's no reason to
> > duplicate that logic in the tracing system. Moreover, as the comment
> > above core_kernel_data states, it may not always return true for
> > .rodata - that is for example the case on x86_64, where we thus end up
> > kstrdup'ing all the passed-in strings.
> >