linux-ext4 - Re: [PATCH] ext4: correct best extent lstart adjustment logic

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] [day] [month] [year] [list]

Date: Thu, 1 Feb 2024 20:34:56 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>, Jan Kara <jack@...e.cz>
CC: <linux-ext4@...r.kernel.org>, <tytso@....edu>, <adilger.kernel@...ger.ca>,
	<ritesh.list@...il.com>, <linux-kernel@...r.kernel.org>,
	<yi.zhang@...wei.com>, <yangerkun@...wei.com>, <yukuai3@...wei.com>,
	<stable@...nel.org>, Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH] ext4: correct best extent lstart adjustment logic

On 2024/2/1 19:08, Ojaswin Mujoo wrote:

Hi Ojaswin, Jan

> Hi Baokun, Jan
>
> Thanks for the CC, I somehow missed this patch.
>
> As described in the discussion Jan linked [1] , there is a known bug in the
> normalize code (which i should probably get back to now ) where we sometimes
> end up with a goal range which doesn't completely cover the original extent and
> this was causing issues when we tried to cover the complete original request in
> the PA window adjustment logic. That and to minimize fragmentation, we ended up
> going with the logic we have right now.
>
> In short, I agree that in the example Baokun pointed out, it is not optimal to
> have to make an allocation request twice when we can get it in one go.
>
> I also think Baokun is correct that if keeping the best extent at the end doesn't
> cover the original start, then any other case should not lead to it overflowing out
> of goal extent, including the case where original extent is overflowing goal extent.
>
> So, as mentioned, it boils down to a trade off between multiple allocations and slightly
> increased fragmentation. iiuc preallocations are anyways dropped when the file closes
> so I think it shouldn't hurt too much fragmentation wise to prioritize less
> allocations. What are your thoughts on this Jan, Baokun?
>
> Coming to the code, the only thing I think might cause an issue is the following line:
>
> -		BUG_ON(ac->ac_o_ex.fe_len > ac->ac_b_ex.fe_len);
> +		BUG_ON(o_ex_end > extent_logical_end(sbi, &ex));
>
> So as discussed towards the end here [1] we could have ac_o_ex that
> overflows the goal and hence would be beyond the best length. I'll try
> to look into the normalize logic to fix this however till then, I think
> we should not have this BUG_ON since it would crash the kernel if this
> happens.
>
> Rest of it looks good to me.
>
> Regards,
> Ojaswin
>
> [1]
> https://lore.kernel.org/all/Y+UzQJRIJEiAr4Z4@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/
I will remove the problematic BUG_ON and add some comments in
the next version.

Thanks to Ojaswin and Jan for the review!
-- 
With Best Regards,
Baokun Li
.