* Re: [PATCH v1] xfs: prevent readdir infinite loop with billions subdirs
2025-08-01 8:41 [PATCH v1] xfs: prevent readdir infinite loop with billions subdirs liuhuan01
@ 2025-08-11 10:36 ` Christoph Hellwig
2025-08-12 0:22 ` Dave Chinner
1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2025-08-11 10:36 UTC (permalink / raw)
To: liuhuan01; +Cc: cem, djwong, linux-xfs, linux-kernel
On Fri, Aug 01, 2025 at 04:41:46PM +0800, liuhuan01@kylinos.cn wrote:
> From: liuh <liuhuan01@kylinos.cn>
>
> When a directory contains billions subdirs, readdir() repeatedly
> got same data and goes to infinate loop.
> The root cause is that the pos gets truncated during assignment.
> Fix it.
This got added in:
15440319767942a363f282d6585303d3d75088ba
Author: Christoph Hellwig <hch@infradead.org>
Date: Thu Jan 8 14:00:00 2009 -0500
[XFS] truncate readdir offsets to signed 32 bit values
I promised to come up with something better back them, but I could for
the fear of my life not remember what that would be.
I guess the most compatible thing we could do would be to make the
truncation dependent on is_compat_task(). You'd still get the same
issue on 32-bit systems, but I don't really see what else we could
do for them.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v1] xfs: prevent readdir infinite loop with billions subdirs
2025-08-01 8:41 [PATCH v1] xfs: prevent readdir infinite loop with billions subdirs liuhuan01
2025-08-11 10:36 ` Christoph Hellwig
@ 2025-08-12 0:22 ` Dave Chinner
1 sibling, 0 replies; 3+ messages in thread
From: Dave Chinner @ 2025-08-12 0:22 UTC (permalink / raw)
To: liuhuan01; +Cc: cem, djwong, linux-xfs, linux-kernel
On Fri, Aug 01, 2025 at 04:41:46PM +0800, liuhuan01@kylinos.cn wrote:
> From: liuh <liuhuan01@kylinos.cn>
>
> When a directory contains billions subdirs, readdir() repeatedly
> got same data and goes to infinate loop.
FWIW, we don't support "billions of dirents in a directory" in XFS.
The max capacity is 1.43 billion dirents, and much less than that is
filenames are anything but minimum length to encode 1.43 billion
unique names.
e.g. if we limit outselves to ascii hex (e.g. for hash based
filenames in an object store), we need 31 characters to encode
enough entries to fill the entire directory data space (32GB).
At this point, the dirent size is 48 bytes (instead of 24 for the
minimum length encoding), and so the maximum
number of entries ends up being around 700 million.
In this case, we'd hit the looping problem at about 350 million
entries into the getdents operation.
The issue is that when we start filling the upper 16GB of the data
segment, the dataptr exceeds 2^31 in length and that high bit is
then filtered off, even on 64 bit systems.
IOWs, the problem is not triggered by the number of entries, but by
the amount of space being consumed in the directory data segment.
Thing is, the kernel directory context uses a loff_t for the dirent
position (i.e. the readdir cookie). So, in the kernel, it is always
64 bits because:
typedef long long __kernel_loff_t;
And so the low level directory iteration code in XFS does not need
to truncate the dir_context->pos value to 31 bits, especially as
the position is always a 32 bit value.
> @@ -491,9 +491,9 @@ xfs_dir2_leaf_getdents(
> * All done. Set output offset value to current offset.
> */
> if (curoff > xfs_dir2_dataptr_to_byte(XFS_DIR2_MAX_DATAPTR))
> - ctx->pos = XFS_DIR2_MAX_DATAPTR & 0x7fffffff;
> + ctx->pos = XFS_DIR2_MAX_DATAPTR;
I think that code is wrong to begin with: if the curoff is beyond
32GB, something is badly wrong with the directory structure. i.e.
we've had a directory data segement overrun.
This can only happen if there's been a corruption we haven't caught
or some kind of bug was tripped over. This condition should result
in failing the operation and returning -EFSCORRUPTED, not truncating
the directory offset....
This also points out the big problem with the seekdir/telldir APIs
on 32 bit systems. telldir returns a signed long for the dirent
cookie, and whilst the man page says:
Application programs should treat this strictly as an opaque
value, making no assumptions about its contents.
Despite this, the value of -1 (0xffffffffff on 32 bit systems) is
not allowed to be used as the dir cookie on 32 bit systems as this
is the indicator that telldir() uses to inform the application that
it encountered an error.
Hence we cannot return XFS_DIR2_MAX_DATAPTR as a valid file position
during getdents on 32 bit systems, nor should we accept it from
seekdir() operations on directories.....
Similarly, seekdir() on a 32 bit system won't support cookie
values over 2^31 (i.e. negative 32 bit values) because XFS doesn't
set FOP_UNSIGNED_OFFSET for directory file objects. Hence seekdir()
will fail with -EINVAL as the offset gets interpretted as being less
than zero...
IOWs, 32 bit APIs are a mess w.r.t. unsigned 32 bit dir cookies,
and so the filtering of the high bit was an attempt to avoid those
issues. Using hundreds of millions of entries in a single directory
is pretty much always a bad idea, so this really hasn't been an
issue that anyone has cared about in production systems.
If we want to fix it, the handling of 32 bit offset overflows stuff
should really be moved to a higher level (e.g. to xfs_file_readdir()
and, perhaps, new directory llseek functions). We probably should
also be configuring directory files on 32 bit systems with
FOP_UNSIGNED_OFFSET so that everything knows that we are using the
whole 32 bit cookie space with seekdir/telldir...
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 3+ messages in thread