On Nov 8, 2024, at 8:30 PM, Yu Kuai yukuai1@huaweicloud.com wrote:
Hi,
在 2024/11/08 21:23, Chuck Lever III 写道:
On Nov 7, 2024, at 8:19 PM, Yu Kuai yukuai1@huaweicloud.com wrote:
Hi,
在 2024/11/07 22:41, Chuck Lever 写道:
On Thu, Nov 07, 2024 at 08:57:23AM +0800, Yu Kuai wrote:
Hi,
在 2024/11/06 23:19, Chuck Lever III 写道:
> On Nov 6, 2024, at 1:16 AM, Greg KH gregkh@linuxfoundation.org wrote: > > On Thu, Oct 24, 2024 at 09:19:41PM +0800, Yu Kuai wrote: >> From: Yu Kuai yukuai3@huawei.com >> >> Fix patch is patch 27, relied patches are from:
I assume patch 27 is:
libfs: fix infinite directory reads for offset dir
https://lore.kernel.org/stable/20241024132225.2271667-12-yukuai1@huaweicloud...
I don't think the Maple tree patches are a hard requirement for this fix. And note that libfs did not use Maple tree originally because I was told at that time that Maple tree was not yet mature.
So, a better approach might be to fit the fix onto linux-6.6.y while sticking with xarray.
The painful part is that using xarray is not acceptable, the offet is just 32 bit and if it overflows, readdir will read nothing. That's why maple_tree has to be used.
A 32-bit range should be entirely adequate for this usage.
- The offset allocator wraps when it reaches the maximum, it doesn't overflow unless there are actually billions of extant entries in the directory, which IMO is not likely.
Yes, it's not likely, but it's possible, and not hard to trigger for test.
I question whether such a test reflects any real-world workload. Besides, there are a number of other limits that will impact the ability to create that many entries in one directory. The number of inodes in one tmpfs instance is limited, for instance.
And please notice that the offset will increase for each new file, and file can be removed, while offset stays the same.
Did you see the above explanation? files can be removed, you don't have to store that much files to trigger the offset to overflow.
- The offset values are dense, so the directory can use all 2- or 4- billion in the 32-bit integer range before wrapping.
A simple math, if user create and remove 1 file in each seconds, it will cost about 130 years to overflow. And if user create and remove 1000 files in each second, it will cost about 1 month to overflow.
The problem is that if the next_offset overflows to 0, then after patch 27, offset_dir_open() will record the 0, and later offset_readdir will return directly, while there can be many files.
Let me revisit this for a moment. The xa_alloc_cyclic() call in simple_offset_add() has a range limit argument of 2 - U32_MAX.
So I'm not clear how an overflow (or, more precisely, the reuse of an offset value) would result in a "0" offset being recorded. The range limit prevents the use of 0 and 1.
A "0" offset value would be a bug, I agree, but I don't see how that can happen.
The question is what happens when there are no more offset values available. xa_alloc_cyclic should fail, and file creation is supposed to fail at that point. If it doesn't, that's a bug that is outside of the use of xarray or Maple.
Can you show me the code that xa_alloc_cyclic should fail? At least according to the commets, it will return 1 if the allocation succeeded after wrapping.
- Context: Any context. Takes and releases the xa_lock. May sleep if
- the @gfp flags permit.
- Return: 0 if the allocation succeeded without wrapping. 1 if the
- allocation succeeded after wrapping, -ENOMEM if memory could not be
- allocated or -EBUSY if there are no free entries in @limit.
*/ static inline int xa_alloc_cyclic(struct xarray *xa, u32 *id, void *entry, struct xa_limit limit, u32 *next, gfp_t gfp)
I recall (dimly) that directory entry offset value re-use is acceptable and preferred, so I think ignoring a "1" return value from xa_alloc_cyclic() is OK. If there are no unused offset values available, it will return -EBUSY, and file creation will fail.
Perhaps Christian or Al can chime in here on whether directory entry offset value re-use is indeed expected to be acceptable.
Further, my understanding is that:
https://lore.kernel.org/stable/20241024132225.2271667-12-yukuai1@huaweicloud...
fixes a rename issue that results in an infinite loop, and that's the (only) issue that underlies CVE-2024-46701.
You are suggesting that there are other overflow problems with the xarray-based simple_offset implementation. If I can confirm them, then I can get these fixed in v6.6. But so far, I'm not sure I completely understand these other failure modes.
Are you suggesting that the above fix /introduces/ the 0 offset problem?
-- Chuck Lever