Re: [PATCH] lib/scatterlist: Fix memory leak in sgl_alloc_order()

From: Douglas Gilbert <dgilbert@interlog.com>
To: Markus Elfring <Markus.Elfring@web.de>, linux-block@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
	Bart Van Assche <bvanassche@acm.org>,
	Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH] lib/scatterlist: Fix memory leak in sgl_alloc_order()
Date: Mon, 21 Sep 2020 00:57:19 +0000	[thread overview]
Message-ID: <9ee6e304-0f96-ea5b-f1ca-84e57345b237@interlog.com> (raw)
In-Reply-To: <d8eb3d0e-52e2-1dab-ac02-774fdbd9c18c@web.de>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 2486 bytes --]

On 2020-09-20 4:11 p.m., Markus Elfring wrote:
>>>> Noticed that when sgl_alloc_order() failed with order > 0 that
>>>> free memory on my machine shrank. That function shouldn't call
>>>> sgl_free() on its error path since that is only correct when
>>>> order=0 .
>>>
>>> * Would an imperative wording become helpful for the change description?
> â€¦
>> â€¦ and the term "imperative wording" rings no
>> bells in my grammatical education. â€¦
> 
> I suggest to take another look at the published Linux development documentation.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id½cf11de8f776152c82d2197b255c2d04603f976#n151
> 
> 
>>> * How do you think about to add the tag â€œFixesâ€ to the commit message?r
>>
>> In the workflow I'm used to, others (closer to LT) make that decision.
>> Why waste my time?
> 
> I find another bit of guidance relevant.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id½cf11de8f776152c82d2197b255c2d04603f976#n183
> 
> 
>>> * Will an other patch subject be more appropriate?
>>
>> Twas testing a 6 GB allocation with said function on my 8 GB laptop.
>> It failed and free told me 5 GB had disappeared â€¦
> â€¦
> 
> Have we got any different expectations for the canonical patch subject line?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id½cf11de8f776152c82d2197b255c2d04603f976#n684
> 
> I am curious how the software will evolve further also according to your
> system test experiences.

Sorry, I didn't come down in the last shower, it's not my first bug fix.
Try consulting 'git log' and look for my name or the MAINTAINERS file.
The culprits are usually happy as was the case with this patch. It's
ack-ed and I would be very surprised if Jens Axboe doesn't accept it.

It is an obvious flaw. Fix it and move on. Alternatively supply your own
patch that ticks all the above boxes.

If you want to talk about something substantial, then why do we have a
function named sgl_free() that only works properly if, for example, the
sgl_alloc_order() function creating the sgl used order=0 ? IMO sgl_free()
should be removed or renamed.

Doug Gilbert

BTW The "imperative mood" stuff in that document is nonsense, at least
in English. Wikipedia maps that term back to "the imperative" as in
"Get thee to a nunnery" and "Et tu, Brute".