Re: [PATCH] Using a pointer and kzalloc in place of a struct directly

From: Greg KH <gregkh@linuxfoundation.org>
To: Anant Thazhemadam <anant.thazhemadam@gmail.com>
Cc: andriin@fb.com, ast@kernel.org, bpf@vger.kernel.org,
	daniel@iogearbox.net, davem@davemloft.net, hawk@kernel.org,
	john.fastabend@gmail.com, kafai@fb.com, kpsingh@chromium.org,
	kuba@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Using a pointer and kzalloc in place of a struct directly
Date: Sun, 13 Sep 2020 13:49:08 +0200	[thread overview]
Message-ID: <20200913114908.GA929395@kroah.com> (raw)
In-Reply-To: <45d9f933-a5c8-ddbd-c014-2bdd5d911e13@gmail.com>

On Sun, Sep 13, 2020 at 01:32:43AM +0530, Anant Thazhemadam wrote:
> On 12/09/20 8:25 pm, Greg KH wrote:
> > On Sat, Sep 12, 2020 at 05:43:38PM +0530, Anant Thazhemadam wrote:
> >> On 12/09/20 5:17 pm, Greg KH wrote:
> >>> Note, your "To:" line seemed corrupted, and why not cc: the bpf mailing
> >>> list as well?
> >> Oh, I'm sorry about that. I pulled the emails of all the people to whom
> >> this mail was sent off from the header in lkml mail, and just cc-ed
> >> everyone.
> >>
> >>> You leaked memory :(
> >>>
> >>> Did you test this patch?  Where do you free this memory, I don't see
> >>> that happening anywhere in this patch, did I miss it?
> >> Yes, I did test this patch, which didn't seem to trigger any issues.
> >> It surprised me so much, that I ended up sending it in, to have
> >> it checked out.
> > You might not have noticed the memory leak if you were not looking for
> > it.
> >
> > How did you test this?
> Ah, that must be it. I tested this using syzbot, which wouldn't have looked
> for memory leaks, but only the issue that was reported. My apologies.
> >> I wasn't sure where exactly the memory allocated here was
> >> supposed to be freed (might be why the current implementation
> >> isn't exactly using kzalloc). I forgot to mention it in the initial mail,
> >> and I was hoping that someone would point me in the right direction
> >> (if this approach was actually going to be considered, that is, which in
> >> retrospect I now feel might not be the best thing)
> > It has to be freed somewhere, you wrote the patch  :)
> >
> > But back to the original question here, why do you feel this change is
> > needed?  What does this do better/faster/more correct than the code that
> > is currently there?  Unless you can provide that, the change should not
> > be needed, right?
> I was initially trying to see if allocating memory would be an appropriate
> heuristic in trying to get a better sense of the bug and crash report, and
> at that moment, that was my goal, and figured that I'd deal with rest
> (such as freeing the memory) later on, if this was a something that could work.
> 
> I was surprised when the patch (although it caused a memory leak), seemed
> to pass the test for the bug, without triggering any issues; since this patch
> basically only allocates memory as compared to locally declaring variables.
> 
> I wanted some input or explanation, about how is it that doing this no longer
> triggers the bug?

That really is up to you to work out, sorry.

Look at what the syzbot is testing, and look at the code change to see
the difference, and you should notice what memory is now being cleared
that previously was not.

good luck!

greg k-h