linux-ext4 - Re: [PATCH] ext4: release page cache in ext4_mb_load

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTinCdpMoEf_Qwq48vPsfxmsFtoOG3g@mail.gmail.com>
Date:	Thu, 14 Apr 2011 10:46:00 +0300
From:	Amir Goldstein <amir73il@...il.com>
To:	Yang Rui Rui <ruirui.r.yang@...to.com>
Cc:	Yang Ruirui R <ruirui.r.yang@...toenator.com>,
	"tytso@....edu" <tytso@....edu>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: release page cache in ext4_mb_load_buddy error path

On Thu, Apr 14, 2011 at 10:29 AM, Yang Rui Rui <ruirui.r.yang@...to.com> wrote:
> Hi,
>
> Thanks for your comment. BTW, why linux-kernel mainling list is removed from
> cc?
> Is it ext4 list prefer or something else?

ext4 list is the place to post fixes for ext4.
LKML need not be bothered with these specific patches.

>
> On 04/14/2011 03:01 PM, Amir Goldstein wrote:
>>
>> Hi Yang ,
>>
>> The patch looks correct, but in my opinion a nicer fix would be to set
>> e4b->bd_bitmap_page = page;
>> or
>> e4b->bd_buddy_page = page;
>> right after assigning a new value to the temp variable 'page'.
>> and keeping the cleanup code in the error path as it is.
>>
>> It's a matter of taste and code readability.
>
> I agree with you for common case, but this function is not so readable
> already.
> Two many if conditions and indent. I would prefer just fix this problem as
> this patch.

As I said it's a matter of taste so I'm not going to argue.
One argument in favor of your patch is that it adds fewer lines.

> Then rewrite the function as more small size functions for example the get
> page part.
>

If you are going to audit code or rework functions in mballoc.c,
I suggest that you first take look at my patches to remove alloc_semp.
Those already involve some rework in mballoc.c.
See online version at:
https://github.com/amir73il/ext4-snapshots/tree/alloc_semp

>>
>> Amir.
>>
>> On Thu, Apr 14, 2011 at 9:44 AM, Yang Ruirui<ruirui.r.yang@...to.com>
>>  wrote:
>>>
>>> Add missing page_cache_release in the error path of ext4_mb_load_buddy
>>>
>>> Signed-off-by: Yang Ruirui<ruirui.r.yang@...to.com>
>>> ---
>>> �fs/ext4/mballoc.c | � �2 ++
>>> �1 file changed, 2 insertions(+)
>>>
>>> --- linux-2.6.orig/fs/ext4/mballoc.c � �2011-04-14 14:04:48.000000000
>>> +0800
>>> +++ linux-2.6/fs/ext4/mballoc.c 2011-04-14 14:33:28.702958245 +0800
>>> @@ -1273,6 +1273,8 @@ repeat_load_buddy:
>>> � � � �return 0;
>>>
>>> �err:
>>> + � � � if (page)
>>> + � � � � � � � page_cache_release(page);
>>> � � � �if (e4b->bd_bitmap_page)
>>> � � � � � � � �page_cache_release(e4b->bd_bitmap_page);
>>> � � � �if (e4b->bd_buddy_page)
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at �http://vger.kernel.org/majordomo-info.html
>>>
>
>
> --
> Thanks
> Yang Ruirui
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html