Re: [PATCH v2] ext4: test reserved GID block allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ext4/309: hardcoded ^has_journal and unchecked mkfs/tune2fs
  break test
- [Low] ext4/309: unverified su -g/-G support and unchecked space-fill
  step
--

commit 6c6eb6186b8e8ccb5609d413ece36c644ad760e0
Author: Sahaj Chaudhari <sahaj123.sc@gmail.com>

ext4: test reserved GID block allocation

This commit adds xfstests test ext4/309 to verify that a user whose primary
and supplementary group matches the superblock s_resgid can allocate blocks
from the reserved pool after ordinary free space is exhausted.

Link: https://lore.kernel.org/r/20261008065651.3517824-1-sahaj123.sc@gmail.com

> diff --git a/tests/ext4/309 b/tests/ext4/309
> new file mode 100755
> index 0000000000000..3bd120091202b
> --- /dev/null
> +++ b/tests/ext4/309
> @@ -0,0 +1,63 @@

[ ... ]

> +MKFS_OPTIONS="-m 10 -N 1024 -O ^has_journal $MKFS_OPTIONS" \
> +	_scratch_mkfs_sized $((16 * 1024 * 1024)) 4096 >>"$seqres".full 2>&1
> +"$TUNE2FS_PROG" -u 0 -g "$RESGID" "$SCRATCH_DEV" >>"$seqres".full 2>&1
> +_scratch_mount

[Severity: Medium]
Can formatting with -O ^has_journal here cause _scratch_mount to fail when
journal mount options are configured?

In __ext4_fill_super(), ext4 rejects mounting a journal-less filesystem
whenever any journal mount option (such as data=ordered, data=journal,
data=writeback, or commit=) is passed, which is not filtered out by
_exclude_scratch_mount_option.

Also, appending caller mkfs options after -m 10 -N 1024 -O ^has_journal lets
a preset such as -m 0 override the 10% reserved pool and cause the final
fallocate to fail.  Neither _scratch_mkfs_sized nor tune2fs checks its exit
status with _fail, so if mkfs fails on conflicting options (for example
-J size=... with -O ^has_journal), the test mounts the stale filesystem left
on the scratch device and fails when 16M does not fill it.

> +
> +touch "$TEST_FILE"
> +chmod 0666 "$TEST_FILE"
> +
> +su -s /bin/bash -g "$FILL_GROUP" -G "$FILL_GROUP" "$qa_user" -c \
> +	"$XFS_IO_PROG -f -c 'pwrite 0 16M' '$TEST_FILE'" \
> +	>>"$seqres".full 2>&1
> +
> +if su -s /bin/bash -g "$FILL_GROUP" -G "$FILL_GROUP" "$qa_user" -c \
> +	"$XFS_IO_PROG -c 'falloc 16M 4k' '$TEST_FILE'" \
> +	>>"$seqres".full 2>&1; then
> +	_fail "ordinary user allocated from the reserved pool"
> +fi
> +
> +su -s /bin/bash -g "$RES_GROUP" -G "$RES_GROUP" "$qa_user" -c \
> +	"$XFS_IO_PROG -c 'falloc 16M 4k' '$TEST_FILE'" \
> +	>>"$seqres".full 2>&1 || _fail "reserved GID failed to allocate a block"

[Severity: Low]
What happens if su does not support -g and -G, or if $qa_user cannot
traverse the scratch mount point?

The -g and -G flags are specific to util-linux su and are not checked before
use.  Because the pwrite step does not verify that any data was written and
the first falloc check treats any non-zero exit status from su as space
exhaustion, an early su or permission failure falls through to the final
step and fails with "reserved GID failed to allocate a block".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107193050.330685-1-sahaj123.sc@gmail.com?part=1