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;
Date:   Mon, 18 Apr 2022 00:49:25 -0400
From:   Jaehee Park <jhpark1013@gmail.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     Pavel Skripkin <paskripkin@gmail.com>, 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
Message-ID: <20220418044925.GA1127014@jaehee-ThinkPad-X1-Extreme>
References: <cover.1649990500.git.jhpark1013@gmail.com>
 <a28f445f-f088-620e-6baf-4cad3e1a8146@gmail.com>
 <20220417204200.GA236965@jaehee-ThinkPad-X1-Extreme>
 <3164900.aeNJFYEL58@leap>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3164900.aeNJFYEL58@leap>
Precedence: bulk

On Sun, Apr 17, 2022 at 11:13:50PM +0200, Fabio M. De Francesco wrote:
> 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.

Hi Fabio, Thank you so much for explaining this so patiently!
This makes sense. I'll remove the pbuf vzalloc. 
I think I was having trouble because of of how pnetwork was defined
in this function. I'll have to think a little more about how to 
intialize it.
Thanks,
Jaehee

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