Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf

From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Pavel Skripkin <paskripkin@gmail.com>,
	Jaehee Park <jhpark1013@gmail.com>
Cc: Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
	gregkh@linuxfoundation.org, linux-staging@lists.linux.dev,
	linux-kernel@vger.kernel.org, outreachy@lists.linux.dev
Subject: Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss_buf
Date: Sun, 17 Apr 2022 23:13:50 +0200	[thread overview]
Message-ID: <3164900.aeNJFYEL58@leap> (raw)
In-Reply-To: <20220417204200.GA236965@jaehee-ThinkPad-X1-Extreme>

On domenica 17 aprile 2022 22:42:00 CEST Jaehee Park wrote:
> On Sun, Apr 17, 2022 at 11:16:38PM +0300, Pavel Skripkin wrote:
> > Hi Jaehee,
> > 
> > On 4/17/22 23:14, Jaehee Park wrote:
> > > My understanding of Pavel's response is the free_bss_buf member of 
the
> > > pmlmepriv structure wasn't being used anywhere and that the
> > > rtw_free_mlme_riv_ie_data function frees the memory of the pmlmepriv
> > > structure so the second check is redundant.
> > > 
> > > However, as Fabio said, the free_bss_buf member is being used and 
pbuf
> > > memory is not being freed.
> > > So I'll revert the patch as it was originally (which was just 
removing
> > > the {} around the single if statement).

No, Jaehee. This is not what I said :)

> > > 
> > 
> > Why just `pbuf` allocation can't be removed? This memory is just 
unused,
> > isn't it?

What Pavel said is what I said, but using a different argumentation.

> > 
> > 
> > With regards,
> > Pavel Skripkin
> 
> 
> The free_bss_buf member is unused.

Correct.

> So it can just be removed right?

No.

> I guess I'm confused by what Pablo is saying about causing a memory 
> leak

A memory leak is caused when you allocate some memory and then you lose any 
reference to its address so that it cannot be freed. Right?

> by getting rid of the pointer to the memory allocated by pbuf.

No.

> Sorry if I misunderstood. 

No problem. Let's rewind...

"pbuf" is assigned with the address of some memory allocated with a call to 
vzalloc(). Since "pbuf" is a local variable, you see that the above-
mentioned address is stored in free_bss_buf using the line "pmlmepriv-
>free_bss_buf = pbuf". Is it clear?

Well, you decided to delete the line that calls vfree(pmlmepriv-
>free_bss_buf). At this point you have that memory leak.

Pavel noted that pmlmepriv->free_bss_buf is unused, but it contains the 
address of a region of memory that was allocated for no purpose.

Therefore, a correct patch should also remove the allocation that was made 
using kzalloc(). If you merely remove the line with vfree() you cause a 
memory leak.

Please don't revert your patch. Just fix it with a new version that also 
delete the line where "pbuf" is assigned with the value returned by 
kzalloc().

I hope that now I've been clearer.

Thanks,

Fabio

> Thanks,
> Jaehee
> 