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

Date: Thu, 1 Feb 2024 16:38:33 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Baokun Li <libaokun1@...wei.com>
Cc: Jan Kara <jack@...e.cz>, 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
Subject: Re: [PATCH] ext4: correct best extent lstart adjustment logic

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/