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: <3cf60868-fb3b-54cd-2177-09c4827e75e7@kernel.org>
Date:   Mon, 25 Jul 2022 17:22:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] rtla: fix double free
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>,
        Andreas Schwab <schwab@suse.de>
Cc:     linux-trace-devel@vger.kernel.org, linux-kernel@vger.kernel.org
References: <mvmzggxl4n1.fsf@suse.de>
 <fd0888a4-099c-95fe-7e20-82be1489061f@kernel.org> <mvmv8rll2yn.fsf@suse.de>
 <20220725105639.295a7d7d@gandalf.local.home>
From:   Daniel Bristot de Oliveira <bristot@kernel.org>
In-Reply-To: <20220725105639.295a7d7d@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/25/22 16:56, Steven Rostedt wrote:
> On Mon, 25 Jul 2022 15:46:40 +0200
> Andreas Schwab <schwab@suse.de> wrote:
> 
>> On Jul 25 2022, Daniel Bristot de Oliveira wrote:
>>
>>> Hi Andreas
>>>
>>> On 7/25/22 15:10, Andreas Schwab wrote:  
>>>> Don't call trace_instance_destroy in trace_instance_init when it fails,
>>>> this is done by the caller.  
>>>
>>> Regarding the Subject, are you seeing a double-free error, or it is just an
>>> optimization?  
>>
>> A double free nowadays is almost always an error, due to better malloc
>> checking.
>>
>>> AFAICS, trace_instance_destroy() checks the pointers before calling free().  
>>
>> That doesn't help when the pointer is not cleared afterwards.  Do you
>> prefer that?
>>
>>> Why am I asking? because if it is a double-free bug, we need to add the "Fixes:"
>>> tag,  
>>
>> It's the first time I tried running rtla, so I don't know whether it is
>> a regression, but from looking at the history it appears to have been
>> introduced already in commit 0605bf009f18 ("rtla: Add osnoise tool")
>>
> 
> I think the real fix is to make trace_instance_destroy() be able to be
> called more than once.
> 
> void trace_instance_destroy(struct trace_instance *trace)
> {
>         if (trace->inst) {
>                 disable_tracer(trace->inst);
>                 destroy_instance(trace->inst);
> 		trace->inst = NULL
ah! right, it was missing this... ^^^
-- Daniel