* [PATCH 1/2] tracing: creates 'tracing_is_disabled()'
@ 2013-10-18 20:59 Geyslan G. Bem
2013-10-18 20:59 ` [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code Geyslan G. Bem
2013-10-19 0:09 ` [PATCH 1/2] tracing: creates 'tracing_is_disabled()' Steven Rostedt
0 siblings, 2 replies; 6+ messages in thread
From: Geyslan G. Bem @ 2013-10-18 20:59 UTC (permalink / raw)
To: kernel-br
Cc: Geyslan G. Bem, Steven Rostedt, Frederic Weisbecker, Ingo Molnar,
open list
This patch creates the function 'tracing_is_disabled ", which
can be used outside trace.c.
Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
kernel/trace/trace.c | 5 +++++
kernel/trace/trace.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 7974ba2..a120a73 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2964,6 +2964,11 @@ int tracing_open_generic(struct inode *inode, struct file *filp)
return 0;
}
+inline bool tracing_is_disabled(void)
+{
+ return (tracing_disabled) ? true: false;
+}
+
/*
* Open and update trace_array ref count.
* Must have the current trace_array passed to it.
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 10c86fb..5deaa3b 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -514,6 +514,7 @@ void tracing_reset_online_cpus(struct trace_buffer *buf);
void tracing_reset_current(int cpu);
void tracing_reset_all_online_cpus(void);
int tracing_open_generic(struct inode *inode, struct file *filp);
+bool tracing_is_disabled(void);
struct dentry *trace_create_file(const char *name,
umode_t mode,
struct dentry *parent,
--
1.8.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code
2013-10-18 20:59 [PATCH 1/2] tracing: creates 'tracing_is_disabled()' Geyslan G. Bem
@ 2013-10-18 20:59 ` Geyslan G. Bem
2013-11-06 16:17 ` Steven Rostedt
2013-10-19 0:09 ` [PATCH 1/2] tracing: creates 'tracing_is_disabled()' Steven Rostedt
1 sibling, 1 reply; 6+ messages in thread
From: Geyslan G. Bem @ 2013-10-18 20:59 UTC (permalink / raw)
To: kernel-br
Cc: Geyslan G. Bem, Steven Rostedt, Frederic Weisbecker, Ingo Molnar,
open list
In 'system_tr_open()':
Fix possible 'dir' assignment after freeing it.
In both functions:
Restructures logic conditions testing 'tracing_is_disabled()'
return before the others tests.
Centralizes the exiting in accordance to Coding Style, Chapter 7.
Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
kernel/trace/trace_events.c | 46 +++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 368a4d5..0f56ebf 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1060,7 +1060,10 @@ static int subsystem_open(struct inode *inode, struct file *filp)
struct event_subsystem *system = NULL;
struct ftrace_subsystem_dir *dir = NULL; /* Initialize for gcc */
struct trace_array *tr;
- int ret;
+ int ret = -ENODEV;
+
+ if (tracing_is_disabled())
+ return ret;
/* Make sure the system still exists */
mutex_lock(&trace_types_lock);
@@ -1082,23 +1085,21 @@ static int subsystem_open(struct inode *inode, struct file *filp)
mutex_unlock(&trace_types_lock);
if (!system)
- return -ENODEV;
+ return ret;
/* Some versions of gcc think dir can be uninitialized here */
WARN_ON(!dir);
/* Still need to increment the ref count of the system */
- if (trace_array_get(tr) < 0) {
- put_system(dir);
- return -ENODEV;
- }
+ ret = trace_array_get(tr);
+ if (ret)
+ goto err_get;
- ret = tracing_open_generic(inode, filp);
- if (ret < 0) {
- trace_array_put(tr);
- put_system(dir);
- }
+ filp->private_data = dir;
+ return 0;
+err_get:
+ put_system(dir);
return ret;
}
@@ -1106,28 +1107,29 @@ static int system_tr_open(struct inode *inode, struct file *filp)
{
struct ftrace_subsystem_dir *dir;
struct trace_array *tr = inode->i_private;
- int ret;
+ int ret = -ENODEV;
- if (trace_array_get(tr) < 0)
- return -ENODEV;
+ if (tracing_is_disabled())
+ return ret;
+
+ ret = trace_array_get(tr);
+ if (ret)
+ return ret;
/* Make a temporary dir that has no system but points to tr */
dir = kzalloc(sizeof(*dir), GFP_KERNEL);
if (!dir) {
- trace_array_put(tr);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_dir;
}
dir->tr = tr;
- ret = tracing_open_generic(inode, filp);
- if (ret < 0) {
- trace_array_put(tr);
- kfree(dir);
- }
-
filp->private_data = dir;
+ return 0;
+err_dir:
+ trace_array_put(tr);
return ret;
}
--
1.8.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] tracing: creates 'tracing_is_disabled()'
2013-10-18 20:59 [PATCH 1/2] tracing: creates 'tracing_is_disabled()' Geyslan G. Bem
2013-10-18 20:59 ` [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code Geyslan G. Bem
@ 2013-10-19 0:09 ` Steven Rostedt
2013-10-19 0:22 ` Geyslan Gregório Bem
1 sibling, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2013-10-19 0:09 UTC (permalink / raw)
To: Geyslan G. Bem; +Cc: kernel-br, Frederic Weisbecker, Ingo Molnar, open list
On Fri, 18 Oct 2013 17:59:41 -0300
"Geyslan G. Bem" <geyslan@gmail.com> wrote:
> This patch creates the function 'tracing_is_disabled ", which
> can be used outside trace.c.
>
> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> ---
> kernel/trace/trace.c | 5 +++++
> kernel/trace/trace.h | 1 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 7974ba2..a120a73 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2964,6 +2964,11 @@ int tracing_open_generic(struct inode *inode, struct file *filp)
> return 0;
> }
>
> +inline bool tracing_is_disabled(void)
Nuke the "inline". Gcc and other compilers are smart enough to know to
inline it on a -O2, that we don't need it. Especially, when it's used
in another file.
-- Steve
> +{
> + return (tracing_disabled) ? true: false;
> +}
> +
> /*
> * Open and update trace_array ref count.
> * Must have the current trace_array passed to it.
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] tracing: creates 'tracing_is_disabled()'
2013-10-19 0:09 ` [PATCH 1/2] tracing: creates 'tracing_is_disabled()' Steven Rostedt
@ 2013-10-19 0:22 ` Geyslan Gregório Bem
0 siblings, 0 replies; 6+ messages in thread
From: Geyslan Gregório Bem @ 2013-10-19 0:22 UTC (permalink / raw)
To: Steven Rostedt; +Cc: kernel-br, Frederic Weisbecker, Ingo Molnar, open list
2013/10/18 Steven Rostedt <rostedt@goodmis.org>:
> On Fri, 18 Oct 2013 17:59:41 -0300
> "Geyslan G. Bem" <geyslan@gmail.com> wrote:
>
>> This patch creates the function 'tracing_is_disabled ", which
>> can be used outside trace.c.
>>
>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> ---
>> kernel/trace/trace.c | 5 +++++
>> kernel/trace/trace.h | 1 +
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 7974ba2..a120a73 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -2964,6 +2964,11 @@ int tracing_open_generic(struct inode *inode, struct file *filp)
>> return 0;
>> }
>>
>> +inline bool tracing_is_disabled(void)
>
> Nuke the "inline". Gcc and other compilers are smart enough to know to
> inline it on a -O2, that we don't need it. Especially, when it's used
> in another file.
>
> -- Steve
Done. Version 3 sent.
Thanks.
>> +{
>> + return (tracing_disabled) ? true: false;
>> +}
>> +
>> /*
>> * Open and update trace_array ref count.
>> * Must have the current trace_array passed to it.
>> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code
2013-10-18 20:59 ` [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code Geyslan G. Bem
@ 2013-11-06 16:17 ` Steven Rostedt
2013-11-06 19:08 ` Geyslan Gregório Bem
0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2013-11-06 16:17 UTC (permalink / raw)
To: Geyslan G. Bem; +Cc: kernel-br, Frederic Weisbecker, Ingo Molnar, open list
Sorry for the late review, I was busy getting ready for kernel summit
and then traveling too much.
On Fri, 18 Oct 2013 17:59:42 -0300
"Geyslan G. Bem" <geyslan@gmail.com> wrote:
> In 'system_tr_open()':
> Fix possible 'dir' assignment after freeing it.
>
> In both functions:
> Restructures logic conditions testing 'tracing_is_disabled()'
> return before the others tests.
> Centralizes the exiting in accordance to Coding Style, Chapter 7.
>
> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> ---
> kernel/trace/trace_events.c | 46 +++++++++++++++++++++++----------------------
> 1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 368a4d5..0f56ebf 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1060,7 +1060,10 @@ static int subsystem_open(struct inode *inode, struct file *filp)
> struct event_subsystem *system = NULL;
> struct ftrace_subsystem_dir *dir = NULL; /* Initialize for gcc */
> struct trace_array *tr;
> - int ret;
> + int ret = -ENODEV;
> +
> + if (tracing_is_disabled())
> + return ret;
>
> /* Make sure the system still exists */
> mutex_lock(&trace_types_lock);
> @@ -1082,23 +1085,21 @@ static int subsystem_open(struct inode *inode, struct file *filp)
> mutex_unlock(&trace_types_lock);
>
> if (!system)
> - return -ENODEV;
> + return ret;
>
> /* Some versions of gcc think dir can be uninitialized here */
> WARN_ON(!dir);
>
> /* Still need to increment the ref count of the system */
> - if (trace_array_get(tr) < 0) {
> - put_system(dir);
> - return -ENODEV;
> - }
> + ret = trace_array_get(tr);
> + if (ret)
> + goto err_get;
>
> - ret = tracing_open_generic(inode, filp);
> - if (ret < 0) {
> - trace_array_put(tr);
> - put_system(dir);
> - }
> + filp->private_data = dir;
> + return 0;
>
> +err_get:
> + put_system(dir);
> return ret;
I don't see any improvement in the above code, except for the initial
check of tracing_is_disabled(). Just add:
if (tracing_is_disabled())
return -ENODEV;
no need for all the fancy work with using ret and goto.
> }
>
> @@ -1106,28 +1107,29 @@ static int system_tr_open(struct inode *inode, struct file *filp)
> {
> struct ftrace_subsystem_dir *dir;
> struct trace_array *tr = inode->i_private;
> - int ret;
> + int ret = -ENODEV;
>
> - if (trace_array_get(tr) < 0)
> - return -ENODEV;
> + if (tracing_is_disabled())
> + return ret;
> +
> + ret = trace_array_get(tr);
> + if (ret)
> + return ret;
>
> /* Make a temporary dir that has no system but points to tr */
> dir = kzalloc(sizeof(*dir), GFP_KERNEL);
> if (!dir) {
> - trace_array_put(tr);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto err_dir;
> }
>
> dir->tr = tr;
>
> - ret = tracing_open_generic(inode, filp);
> - if (ret < 0) {
> - trace_array_put(tr);
> - kfree(dir);
> - }
> -
> filp->private_data = dir;
> + return 0;
>
Again, we don't need the goto and err_dir. The simple fix is to finish
the function with:
ret = tracing_open_generic(inode, filp);
if (ret < 0) {
trace_array_put(tr);
kfree(dir);
return ret;
}
filp->private_data = dir;
return 0;
-- Steve
> +err_dir:
> + trace_array_put(tr);
> return ret;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code
2013-11-06 16:17 ` Steven Rostedt
@ 2013-11-06 19:08 ` Geyslan Gregório Bem
0 siblings, 0 replies; 6+ messages in thread
From: Geyslan Gregório Bem @ 2013-11-06 19:08 UTC (permalink / raw)
To: Steven Rostedt; +Cc: kernel-br, Frederic Weisbecker, Ingo Molnar, open list
2013/11/6 Steven Rostedt <rostedt@goodmis.org>:
> Sorry for the late review, I was busy getting ready for kernel summit
> and then traveling too much.
>
No problem.
>
> On Fri, 18 Oct 2013 17:59:42 -0300
> "Geyslan G. Bem" <geyslan@gmail.com> wrote:
>
>> In 'system_tr_open()':
>> Fix possible 'dir' assignment after freeing it.
>>
>> In both functions:
>> Restructures logic conditions testing 'tracing_is_disabled()'
>> return before the others tests.
>> Centralizes the exiting in accordance to Coding Style, Chapter 7.
>>
>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> ---
>> kernel/trace/trace_events.c | 46 +++++++++++++++++++++++----------------------
>> 1 file changed, 24 insertions(+), 22 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 368a4d5..0f56ebf 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -1060,7 +1060,10 @@ static int subsystem_open(struct inode *inode, struct file *filp)
>> struct event_subsystem *system = NULL;
>> struct ftrace_subsystem_dir *dir = NULL; /* Initialize for gcc */
>> struct trace_array *tr;
>> - int ret;
>> + int ret = -ENODEV;
>> +
>> + if (tracing_is_disabled())
>> + return ret;
>>
>> /* Make sure the system still exists */
>> mutex_lock(&trace_types_lock);
>> @@ -1082,23 +1085,21 @@ static int subsystem_open(struct inode *inode, struct file *filp)
>> mutex_unlock(&trace_types_lock);
>>
>> if (!system)
>> - return -ENODEV;
>> + return ret;
>>
>> /* Some versions of gcc think dir can be uninitialized here */
>> WARN_ON(!dir);
>>
>> /* Still need to increment the ref count of the system */
>> - if (trace_array_get(tr) < 0) {
>> - put_system(dir);
>> - return -ENODEV;
>> - }
>> + ret = trace_array_get(tr);
>> + if (ret)
>> + goto err_get;
>>
>> - ret = tracing_open_generic(inode, filp);
>> - if (ret < 0) {
>> - trace_array_put(tr);
>> - put_system(dir);
>> - }
>> + filp->private_data = dir;
>> + return 0;
>>
>> +err_get:
>> + put_system(dir);
>> return ret;
>
> I don't see any improvement in the above code, except for the initial
> check of tracing_is_disabled(). Just add:
>
> if (tracing_is_disabled())
> return -ENODEV;
>
> no need for all the fancy work with using ret and goto.
>
>
>> }
>>
>> @@ -1106,28 +1107,29 @@ static int system_tr_open(struct inode *inode, struct file *filp)
>> {
>> struct ftrace_subsystem_dir *dir;
>> struct trace_array *tr = inode->i_private;
>> - int ret;
>> + int ret = -ENODEV;
>>
>> - if (trace_array_get(tr) < 0)
>> - return -ENODEV;
>> + if (tracing_is_disabled())
>> + return ret;
>> +
>> + ret = trace_array_get(tr);
>> + if (ret)
>> + return ret;
>>
>> /* Make a temporary dir that has no system but points to tr */
>> dir = kzalloc(sizeof(*dir), GFP_KERNEL);
>> if (!dir) {
>> - trace_array_put(tr);
>> - return -ENOMEM;
>> + ret = -ENOMEM;
>> + goto err_dir;
>> }
>>
>> dir->tr = tr;
>>
>> - ret = tracing_open_generic(inode, filp);
>> - if (ret < 0) {
>> - trace_array_put(tr);
>> - kfree(dir);
>> - }
>> -
>> filp->private_data = dir;
>> + return 0;
>>
>
> Again, we don't need the goto and err_dir. The simple fix is to finish
> the function with:
>
> ret = tracing_open_generic(inode, filp);
> if (ret < 0) {
> trace_array_put(tr);
> kfree(dir);
> return ret;
> }
>
> filp->private_data = dir;
>
> return 0;
>
> -- Steve
>
>> +err_dir:
>> + trace_array_put(tr);
>> return ret;
>> }
>>
>
Done:
[PATCH v2] tracing: fix referencing after memory freeing and refactors code
--
Regards,
Geyslan G. Bem
hackingbits.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-06 19:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-18 20:59 [PATCH 1/2] tracing: creates 'tracing_is_disabled()' Geyslan G. Bem
2013-10-18 20:59 ` [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code Geyslan G. Bem
2013-11-06 16:17 ` Steven Rostedt
2013-11-06 19:08 ` Geyslan Gregório Bem
2013-10-19 0:09 ` [PATCH 1/2] tracing: creates 'tracing_is_disabled()' Steven Rostedt
2013-10-19 0:22 ` Geyslan Gregório Bem
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®