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

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

[Douglas Gilbert]

On 2020-09-20 1:09 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?

No passive tense there. Or do you mean usage like: "Go to hell" or
"Fix memory leak in ..."? I studied French and Latin at school; at a
guess, my mother tongue got its grammar from the former. My mother
taught English grammar and the term "imperative wording" rings no
bells in my grammatical education. Google agrees with me.
Please define: "imperative wording".
> * 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?

> * 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 (and
'cat /sys/kernel/debug/kmemleak' told me _nothing_). Umm, it is
potentially a HUGE f@#$ing memory LEAK! Best to call a spade a spade.

Doug Gilbert

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

[Douglas Gilbert]

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=bdcf11de8f776152c82d2197b255c2d04603f976#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=bdcf11de8f776152c82d2197b255c2d04603f976#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=bdcf11de8f776152c82d2197b255c2d04603f976#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".

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

[Dan Carpenter]

We asked Markus over and over to stop sending these emails but he
refused so eventually he had to be banned from vger.  Unless you're
directly mentioned in the CC list then you can't see his emails.

regards,
dan carpenter