All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Revert "tracing: fix double free"
@ 2022-06-30  1:31 Zheng Yejian
  2022-07-09  1:03 ` Steven Rostedt
  0 siblings, 1 reply; 7+ messages in thread
From: Zheng Yejian @ 2022-06-30  1:31 UTC (permalink / raw)
  To: rostedt, mingo, linux-kernel, tom.zanussi, trix
  Cc: stable, zhangjinhao2, zhengyejian1

This reverts commit 46bbe5c671e06f070428b9be142cc4ee5cedebac.

As commit 46bbe5c671e0 ("tracing: fix double free") said, the
"double free" problem reported by clang static analyzer is:
  > In parse_var_defs() if there is a problem allocating
  > var_defs.expr, the earlier var_defs.name is freed.
  > This free is duplicated by free_var_defs() which frees
  > the rest of the list.

However, if there is a problem allocating N-th var_defs.expr:
  + in parse_var_defs(), the freed 'earlier var_defs.name' is
    actually the N-th var_defs.name;
  + then in free_var_defs(), the names from 0th to (N-1)-th are freed;

                        IF ALLOCATING PROBLEM HAPPENED HERE!!! -+
                                                                 \
                                                                  |
          0th           1th                 (N-1)-th      N-th    V
          +-------------+-------------+-----+-------------+-----------
var_defs: | name | expr | name | expr | ... | name | expr | name | ///
          +-------------+-------------+-----+-------------+-----------

These two frees don't act on same name, so there was no "double free"
problem before. Conversely, after that commit, we get a "memory leak"
problem because the above "N-th var_defs.name" is not freed.

If enable CONFIG_DEBUG_KMEMLEAK and inject a fault at where the N-th
var_defs.expr allocated, then execute on shell like:
  $ echo 'hist:key=call_site:val=$v1,$v2:v1=bytes_req,v2=bytes_alloc' > \
/sys/kernel/debug/tracing/events/kmem/kmalloc/trigger

Then kmemleak reports:
  unreferenced object 0xffff8fb100ef3518 (size 8):
    comm "bash", pid 196, jiffies 4295681690 (age 28.538s)
    hex dump (first 8 bytes):
      76 31 00 00 b1 8f ff ff                          v1......
    backtrace:
      [<0000000038fe4895>] kstrdup+0x2d/0x60
      [<00000000c99c049a>] event_hist_trigger_parse+0x206f/0x20e0
      [<00000000ae70d2cc>] trigger_process_regex+0xc0/0x110
      [<0000000066737a4c>] event_trigger_write+0x75/0xd0
      [<000000007341e40c>] vfs_write+0xbb/0x2a0
      [<0000000087fde4c2>] ksys_write+0x59/0xd0
      [<00000000581e9cdf>] do_syscall_64+0x3a/0x80
      [<00000000cf3b065c>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Cc: stable@vger.kernel.org
Fixes: 46bbe5c671e0 ("tracing: fix double free")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
---
 kernel/trace/trace_events_hist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 48e82e141d54..2784951e0fc8 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4430,6 +4430,7 @@ static int parse_var_defs(struct hist_trigger_data *hist_data)
 
 			s = kstrdup(field_str, GFP_KERNEL);
 			if (!s) {
+				kfree(hist_data->attrs->var_defs.name[n_vars]);
 				ret = -ENOMEM;
 				goto free;
 			}
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH] Revert "tracing: fix double free"
  2022-06-30  1:31 [PATCH] Revert "tracing: fix double free" Zheng Yejian
@ 2022-07-09  1:03 ` Steven Rostedt
  2022-07-11  1:47   ` [PATCH v2] tracing/histograms: Fix memory leak problem Zheng Yejian
  0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2022-07-09  1:03 UTC (permalink / raw)
  To: Zheng Yejian; +Cc: mingo, linux-kernel, tom.zanussi, trix, stable, zhangjinhao2

On Thu, 30 Jun 2022 09:31:37 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 48e82e141d54..2784951e0fc8 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4430,6 +4430,7 @@ static int parse_var_defs(struct hist_trigger_data *hist_data)
>  
>  			s = kstrdup(field_str, GFP_KERNEL);
>  			if (!s) {
> +				kfree(hist_data->attrs->var_defs.name[n_vars]);

Instead of doing just a revert, can we also add, for safety:

				hist_data->attrs->var_defs.name[n_vars] = NULL;

Thanks,

-- Steve

>  				ret = -ENOMEM;
>  				goto free;
>  			}

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH v2] tracing/histograms: Fix memory leak problem
  2022-07-09  1:03 ` Steven Rostedt
@ 2022-07-11  1:47   ` Zheng Yejian
  2022-07-11 14:27     ` Zanussi, Tom
  0 siblings, 1 reply; 7+ messages in thread
From: Zheng Yejian @ 2022-07-11  1:47 UTC (permalink / raw)
  To: rostedt
  Cc: linux-kernel, mingo, stable, tom.zanussi, trix, zhangjinhao2,
	zhengyejian1

This reverts commit 46bbe5c671e06f070428b9be142cc4ee5cedebac.

As commit 46bbe5c671e0 ("tracing: fix double free") said, the
"double free" problem reported by clang static analyzer is:
  > In parse_var_defs() if there is a problem allocating
  > var_defs.expr, the earlier var_defs.name is freed.
  > This free is duplicated by free_var_defs() which frees
  > the rest of the list.

However, if there is a problem allocating N-th var_defs.expr:
  + in parse_var_defs(), the freed 'earlier var_defs.name' is
    actually the N-th var_defs.name;
  + then in free_var_defs(), the names from 0th to (N-1)-th are freed;

                        IF ALLOCATING PROBLEM HAPPENED HERE!!! -+
                                                                 \
                                                                  |
          0th           1th                 (N-1)-th      N-th    V
          +-------------+-------------+-----+-------------+-----------
var_defs: | name | expr | name | expr | ... | name | expr | name | ///
          +-------------+-------------+-----+-------------+-----------

These two frees don't act on same name, so there was no "double free"
problem before. Conversely, after that commit, we get a "memory leak"
problem because the above "N-th var_defs.name" is not freed.

If enable CONFIG_DEBUG_KMEMLEAK and inject a fault at where the N-th
var_defs.expr allocated, then execute on shell like:
  $ echo 'hist:key=call_site:val=$v1,$v2:v1=bytes_req,v2=bytes_alloc' > \
/sys/kernel/debug/tracing/events/kmem/kmalloc/trigger

Then kmemleak reports:
  unreferenced object 0xffff8fb100ef3518 (size 8):
    comm "bash", pid 196, jiffies 4295681690 (age 28.538s)
    hex dump (first 8 bytes):
      76 31 00 00 b1 8f ff ff                          v1......
    backtrace:
      [<0000000038fe4895>] kstrdup+0x2d/0x60
      [<00000000c99c049a>] event_hist_trigger_parse+0x206f/0x20e0
      [<00000000ae70d2cc>] trigger_process_regex+0xc0/0x110
      [<0000000066737a4c>] event_trigger_write+0x75/0xd0
      [<000000007341e40c>] vfs_write+0xbb/0x2a0
      [<0000000087fde4c2>] ksys_write+0x59/0xd0
      [<00000000581e9cdf>] do_syscall_64+0x3a/0x80
      [<00000000cf3b065c>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

Cc: stable@vger.kernel.org
Fixes: 46bbe5c671e0 ("tracing: fix double free")
Reported-by: Hulk Robot <hulkci@huawei.com>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
---
 kernel/trace/trace_events_hist.c | 2 ++
 1 file changed, 2 insertions(+)

Changes since v1:
- Assign 'NULL' after 'kfree' for safety as suggested by Steven
- Rename commit title and add Suggested-by tag

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 48e82e141d54..e87a46794079 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4430,6 +4430,8 @@ static int parse_var_defs(struct hist_trigger_data *hist_data)
 
 			s = kstrdup(field_str, GFP_KERNEL);
 			if (!s) {
+				kfree(hist_data->attrs->var_defs.name[n_vars]);
+				hist_data->attrs->var_defs.name[n_vars] = NULL;
 				ret = -ENOMEM;
 				goto free;
 			}
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing/histograms: Fix memory leak problem
  2022-07-11  1:47   ` [PATCH v2] tracing/histograms: Fix memory leak problem Zheng Yejian
@ 2022-07-11 14:27     ` Zanussi, Tom
  2022-07-11 15:52       ` Steven Rostedt
  0 siblings, 1 reply; 7+ messages in thread
From: Zanussi, Tom @ 2022-07-11 14:27 UTC (permalink / raw)
  To: Zheng Yejian, rostedt; +Cc: linux-kernel, mingo, stable, trix, zhangjinhao2

Hi Yejian,

On 7/10/2022 8:47 PM, Zheng Yejian wrote:
> This reverts commit 46bbe5c671e06f070428b9be142cc4ee5cedebac.
> 
> As commit 46bbe5c671e0 ("tracing: fix double free") said, the
> "double free" problem reported by clang static analyzer is:
>   > In parse_var_defs() if there is a problem allocating
>   > var_defs.expr, the earlier var_defs.name is freed.
>   > This free is duplicated by free_var_defs() which frees
>   > the rest of the list.
> 
> However, if there is a problem allocating N-th var_defs.expr:
>   + in parse_var_defs(), the freed 'earlier var_defs.name' is
>     actually the N-th var_defs.name;
>   + then in free_var_defs(), the names from 0th to (N-1)-th are freed;
> 
>                         IF ALLOCATING PROBLEM HAPPENED HERE!!! -+
>                                                                  \
>                                                                   |
>           0th           1th                 (N-1)-th      N-th    V
>           +-------------+-------------+-----+-------------+-----------
> var_defs: | name | expr | name | expr | ... | name | expr | name | ///
>           +-------------+-------------+-----+-------------+-----------
> 
> These two frees don't act on same name, so there was no "double free"
> problem before. Conversely, after that commit, we get a "memory leak"
> problem because the above "N-th var_defs.name" is not freed.

Good catch, thanks for fixing it.

So I'm wondering if this means that that the original unnecessary bugfix
was based on a bug in the clang static analyzer or if that would just be
considered a false positive...

Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>

Tom  

> 
> If enable CONFIG_DEBUG_KMEMLEAK and inject a fault at where the N-th
> var_defs.expr allocated, then execute on shell like:
>   $ echo 'hist:key=call_site:val=$v1,$v2:v1=bytes_req,v2=bytes_alloc' > \
> /sys/kernel/debug/tracing/events/kmem/kmalloc/trigger
> 
> Then kmemleak reports:
>   unreferenced object 0xffff8fb100ef3518 (size 8):
>     comm "bash", pid 196, jiffies 4295681690 (age 28.538s)
>     hex dump (first 8 bytes):
>       76 31 00 00 b1 8f ff ff                          v1......
>     backtrace:
>       [<0000000038fe4895>] kstrdup+0x2d/0x60
>       [<00000000c99c049a>] event_hist_trigger_parse+0x206f/0x20e0
>       [<00000000ae70d2cc>] trigger_process_regex+0xc0/0x110
>       [<0000000066737a4c>] event_trigger_write+0x75/0xd0
>       [<000000007341e40c>] vfs_write+0xbb/0x2a0
>       [<0000000087fde4c2>] ksys_write+0x59/0xd0
>       [<00000000581e9cdf>] do_syscall_64+0x3a/0x80
>       [<00000000cf3b065c>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> Cc: stable@vger.kernel.org
> Fixes: 46bbe5c671e0 ("tracing: fix double free")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
> ---
>  kernel/trace/trace_events_hist.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> Changes since v1:
> - Assign 'NULL' after 'kfree' for safety as suggested by Steven
> - Rename commit title and add Suggested-by tag
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 48e82e141d54..e87a46794079 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4430,6 +4430,8 @@ static int parse_var_defs(struct hist_trigger_data *hist_data)
>  
>  			s = kstrdup(field_str, GFP_KERNEL);
>  			if (!s) {
> +				kfree(hist_data->attrs->var_defs.name[n_vars]);
> +				hist_data->attrs->var_defs.name[n_vars] = NULL;
>  				ret = -ENOMEM;
>  				goto free;
>  			}

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing/histograms: Fix memory leak problem
  2022-07-11 14:27     ` Zanussi, Tom
@ 2022-07-11 15:52       ` Steven Rostedt
  2022-07-12 11:48         ` Zheng Yejian
  0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2022-07-11 15:52 UTC (permalink / raw)
  To: Zanussi, Tom
  Cc: Zheng Yejian, linux-kernel, mingo, stable, trix, zhangjinhao2

On Mon, 11 Jul 2022 09:27:44 -0500
"Zanussi, Tom" <tom.zanussi@linux.intel.com> wrote:

> So I'm wondering if this means that that the original unnecessary bugfix
> was based on a bug in the clang static analyzer or if that would just be
> considered a false positive...

Good question. I'd like to know this, as if it is the case, I want to
report that in my pull request to Linus.

-- Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing/histograms: Fix memory leak problem
  2022-07-11 15:52       ` Steven Rostedt
@ 2022-07-12 11:48         ` Zheng Yejian
  2022-07-12 14:51           ` Steven Rostedt
  0 siblings, 1 reply; 7+ messages in thread
From: Zheng Yejian @ 2022-07-12 11:48 UTC (permalink / raw)
  To: rostedt
  Cc: linux-kernel, mingo, stable, tom.zanussi, trix, zhangjinhao2,
	zhengyejian1

On Mon, 11 Jul 2022 11:52:11 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 11 Jul 2022 09:27:44 -0500
> "Zanussi, Tom" <tom.zanussi@linux.intel.com> wrote:
> 
> > So I'm wondering if this means that that the original unnecessary bugfix
> > was based on a bug in the clang static analyzer or if that would just be
> > considered a false positive...
> 
> Good question. I'd like to know this, as if it is the case, I want to
> report that in my pull request to Linus.
> 
> -- Steve

I didn't use clang static analyzer before, but from its home page,
'False Positives' seems to exist, see https://clang-analyzer.llvm.org/:
    > Static analysis is not perfect. It can falsely flag bugs in a program
    > where the code behaves correctly. Because some code checks require more
    > analysis precision than others, the frequency of false positives can
    > vary widely between different checks. Our long-term goal is to have the
    > analyzer have a low false positive rate for most code on all checks.

So I try the clang-14 which comes from
https://github.com/llvm/llvm-project/releases/tag/llvmorg-14.0.0,
then execute like:
  $ scan-build make -j16

Then I take a rough look at following warnings related to 'trace_events_hist.c'
(serial number is manually added, no double free warning, maybe due to clang version):
  1. kernel/trace/trace_events_hist.c:1540:6: warning: Branch condition evaluates to a garbage value [core.uninitialized.Branch]
          if (!attrs->keys_str) {
              ^~~~~~~~~~~~~~~~
  2. kernel/trace/trace_events_hist.c:1580:9: warning: Array access (via field 'field_var_str') results in a null pointer dereference [core.NullDereference]
                  kfree(elt_data->field_var_str[i]);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
  3. kernel/trace/trace_events_hist.c:1898:3: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
                  destroy_hist_field(hist_field->operands[i], level + 1);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  4. kernel/trace/trace_events_hist.c:2095:2: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
          kfree(ref_field->system);
          ^~~~~~~~~~~~~~~~~~~~~~~~
  5. kernel/trace/trace_events_hist.c:2099:2: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
          kfree(ref_field->name);
          ^~~~~~~~~~~~~~~~~~~~~~
  6. kernel/trace/trace_events_hist.c:2158:4: warning: Potential leak of memory pointed to by 'ref_field' [unix.Malloc]
                          return NULL;

Since I'm not very familiar with trace_events_hist.c, I roughly conclude that:
  1. warning 1/3/6 are plausible but false-positive;
  2. warning 2/4/5 seems positive although they don't cause practical problems because
elt_data->field_var_str[i] / ref_field->system / ref_field->name can be 'NULL'
on 'kfree'. Do we need to explicitly check 'NULL' there?

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing/histograms: Fix memory leak problem
  2022-07-12 11:48         ` Zheng Yejian
@ 2022-07-12 14:51           ` Steven Rostedt
  0 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2022-07-12 14:51 UTC (permalink / raw)
  To: Zheng Yejian; +Cc: linux-kernel, mingo, stable, tom.zanussi, trix, zhangjinhao2

On Tue, 12 Jul 2022 19:48:44 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:

> Since I'm not very familiar with trace_events_hist.c, I roughly conclude that:
>   1. warning 1/3/6 are plausible but false-positive;
>   2. warning 2/4/5 seems positive although they don't cause practical problems because
> elt_data->field_var_str[i] / ref_field->system / ref_field->name can be 'NULL'
> on 'kfree'. Do we need to explicitly check 'NULL' there?

It's confusing code. Both Tom and I missed it as well.

-- Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-07-12 14:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-30  1:31 [PATCH] Revert "tracing: fix double free" Zheng Yejian
2022-07-09  1:03 ` Steven Rostedt
2022-07-11  1:47   ` [PATCH v2] tracing/histograms: Fix memory leak problem Zheng Yejian
2022-07-11 14:27     ` Zanussi, Tom
2022-07-11 15:52       ` Steven Rostedt
2022-07-12 11:48         ` Zheng Yejian
2022-07-12 14:51           ` Steven Rostedt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.