Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7b562ff6-3f44-053b-d8e6-3c40be145446@linux.intel.com>
Date:   Mon, 11 Jul 2022 09:27:44 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2] tracing/histograms: Fix memory leak problem
To:     Zheng Yejian <zhengyejian1@huawei.com>, rostedt@goodmis.org
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com,
        stable@vger.kernel.org, trix@redhat.com, zhangjinhao2@huawei.com
References: <20220708210335.79a38356@gandalf.local.home>
 <20220711014731.69520-1-zhengyejian1@huawei.com>
From:   "Zanussi, Tom" <tom.zanussi@linux.intel.com>
In-Reply-To: <20220711014731.69520-1-zhengyejian1@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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