* [PATCH v2] xfs: test case for handling io errors when reading extended attributes @ 2025-08-29 4:24 Donald Douwsma 2025-08-29 19:49 ` Zorro Lang 0 siblings, 1 reply; 3+ messages in thread From: Donald Douwsma @ 2025-08-29 4:24 UTC (permalink / raw) To: linux-xfs, fstests Cc: Eric Sandeen, Darrick J . Wong, Carlos Maiolino, Donald Douwsma We've seen reports from the field panicing in xfs_trans_brelse after an io error for an attribute block. sd 0:0:23:0: [sdx] tag#271 CDB: Read(16) 88 00 00 00 00 00 9b df 5e 78 00 00 00 08 00 00 critical medium error, dev sdx, sector 2615107192 op 0x0:(READ) flags 0x1000 phys_seg 1 prio class 2 XFS (sdx1): metadata I/O error in "xfs_da_read_buf+0xe1/0x140 [xfs]" at daddr 0x9bdf5678 len 8 error 61 BUG: kernel NULL pointer dereference, address: 00000000000000e0 ... RIP: 0010:xfs_trans_brelse+0xb/0xe0 [xfs] ... Call Trace: <TASK> ... ? xfs_trans_brelse+0xb/0xe0 [xfs] xfs_attr_leaf_get+0xb6/0xc0 [xfs] xfs_attr_get+0xa0/0xd0 [xfs] xfs_xattr_get+0x75/0xb0 [xfs] __vfs_getxattr+0x53/0x70 inode_doinit_use_xattr+0x63/0x180 inode_doinit_with_dentry+0x196/0x510 security_d_instantiate+0x2f/0x50 d_splice_alias+0x46/0x2b0 xfs_vn_lookup+0x8b/0xb0 [xfs] __lookup_slow+0x84/0x130 walk_component+0x158/0x1d0 path_lookupat+0x6e/0x1c0 filename_lookup+0xcf/0x1d0 vfs_statx+0x8d/0x170 vfs_fstatat+0x54/0x70 __do_sys_newfstatat+0x26/0x60 As this is specific to ENODATA test using scsi_debug instead of dmerror which returns EIO. Signed-off-by: Donald Douwsma <ddouwsma@redhat.com> --- tests/xfs/999 | 193 ++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/999.out | 2 + 2 files changed, 195 insertions(+) create mode 100755 tests/xfs/999 create mode 100644 tests/xfs/999.out diff --git a/tests/xfs/999 b/tests/xfs/999 new file mode 100755 index 00000000..2a45eb7c --- /dev/null +++ b/tests/xfs/999 @@ -0,0 +1,193 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2025 Red Hat Inc. All Rights Reserved. +# +# FS QA Test 999 +# +# Regression test for panic following IO error when reading extended attribute blocks +# +# XFS (sda): metadata I/O error in "xfs_da_read_buf+0xe1/0x140 [xfs]" at daddr 0x78 len 8 error 61 +# BUG: kernel NULL pointer dereference, address: 00000000000000e0 +# ... +# RIP: 0010:xfs_trans_brelse+0xb/0xe0 [xfs] +# +# [53887.310123] Call Trace: +# <TASK> +# ? show_trace_log_lvl+0x1c4/0x2df +# ? show_trace_log_lvl+0x1c4/0x2df +# ? xfs_attr_leaf_get+0xb6/0xc0 [xfs] +# ? __die_body.cold+0x8/0xd +# ? page_fault_oops+0x134/0x170 +# ? xfs_trans_read_buf_map+0x133/0x300 [xfs] +# ? exc_page_fault+0x62/0x150 +# ? asm_exc_page_fault+0x22/0x30 +# ? xfs_trans_brelse+0xb/0xe0 [xfs] +# xfs_attr_leaf_get+0xb6/0xc0 [xfs] +# xfs_attr_get+0xa0/0xd0 [xfs] +# xfs_xattr_get+0x88/0xd0 [xfs] +# __vfs_getxattr+0x7b/0xb0 +# inode_doinit_use_xattr+0x63/0x180 +# inode_doinit_with_dentry+0x196/0x510 +# security_d_instantiate+0x3a/0xb0 +# d_splice_alias+0x46/0x2b0 +# xfs_vn_lookup+0x8b/0xb0 [xfs] +# __lookup_slow+0x81/0x130 +# walk_component+0x158/0x1d0 +# ? path_init+0x2c5/0x3f0 +# path_lookupat+0x6e/0x1c0 +# filename_lookup+0xcf/0x1d0 +# ? tlb_finish_mmu+0x65/0x150 +# vfs_statx+0x82/0x160 +# vfs_fstatat+0x54/0x70 +# __do_sys_newfstatat+0x26/0x60 +# do_syscall_64+0x5c/0xe0 +# +# For SELinux enabled filesystems the attribute security.selinux will be +# created before any additional attributes are added. Any attempt to open the +# file will read this entry, leading to the panic being triggered as above, via +# security_d_instantiate, rather than fgetxattr(2), to test via fgetxattr mount +# with a fixed context= +# +# Kernels prior to v4.16 don't have medium_error_start, and only return errors +# for a specific location, making scsi_debug unsuitable for checking old kernels. +# See d9da891a892a scsi: scsi_debug: Add two new parameters to scsi_debug driver +# + +. ./common/preamble +_begin_fstest auto + +_fixed_by_kernel_commit ae668cd567a6 "xfs: do not propagate ENODATA disk errors into xattr code" + +# Override the default cleanup function. +_cleanup() +{ + cd / + rm -f $tmp.* + _unmount $SCRATCH_MNT + _put_scsi_debug_dev +} + +# Import common functions. +. ./common/scsi_debug + +_require_scsi_debug +modinfo scsi_debug | grep -wq medium_error_start || _notrun "test requires scsi_debug medium_error_start" +scsi_debug_dev=$(_get_scsi_debug_dev) +test -b $scsi_debug_dev || _notrun "Failed to initialize scsi debug device" +echo "SCSI debug device $scsi_debug_dev" >>$seqres.full + +_mkfs_dev $scsi_debug_dev >> $seqres.full +_mount $scsi_debug_dev $SCRATCH_MNT >> $seqres.full + +blocksize=$(_get_block_size $SCRATCH_MNT) >> $seqres.full +echo Blocksize $blocksize >> $seqres.full + +# Use dry_run=1 when verifying the test to avoid panicking. +enable_error=1 +[ ${dry_run:-0} -eq 1 ] && enable_error=0 + +test_attr() +{ + test=$1 + testfile=$SCRATCH_MNT/$test + attr_size=$2 # bytes + error_at_block=${3:-0} + shift 3 + + value_size=$attr_size + + echo 0 > /sys/module/scsi_debug/parameters/opts + + echo -e "\nTesting : $test" >> $seqres.full + echo -e "attr size: $attr_size" >> $seqres.full + echo -e "error at block: $error_at_block\n" >> $seqres.full + + touch $testfile + + inode=$(ls -i $testfile|awk '{print $1}') + printf "inode: %d Testfile: %s\n" $inode $testfile >> $seqres.full + setfattr -n user.test_attr -v $(printf "%0*d" $value_size $value_size) $testfile + + $XFS_IO_PROG -c "bmap -al" $testfile >> $seqres.full + start_blocks=($($XFS_IO_PROG -c "bmap -al" $testfile | awk 'match($3, /[0-9]+/, a) {print a[0]}')) + echo "Attribute fork extent(s) start at ${attrblocks[*]}" >> $seqres.full + + _unmount $SCRATCH_MNT >>$seqres.full 2>&1 + + echo "Dump inode $inode details with xfs_db" >> $seqres.full + $XFS_DB_PROG -c "inode $inode" -c "print core.aformat core.naextents a" $scsi_debug_dev >> $seqres.full + inode_daddr=$($XFS_DB_PROG -c "inode $inode" -c "daddr" $scsi_debug_dev | awk '{print $4}') + echo inode daddr $inode_daddr >> $seqres.full + + + _mount $scsi_debug_dev $SCRATCH_MNT >> $seqres.full + + if [[ start_blocks[0] -ne 0 ]]; then + # Choose the block to error, currently only works with a single extent. + error_daddr=$((start_blocks[0] + error_at_block*blocksize/512)) + else + # Default to the inode daddr when no extents were found. + # Fails reading the inode during stat, so arguably useless + # even for testing the upper layers of getfattr. + error_daddr=$inode_daddr + fi + + echo "Setup scsi_debug to error when reading attributes from block" \ + "$error_at_block at daddr $error_daddr" >> $seqres.full + echo $error_daddr > /sys/module/scsi_debug/parameters/medium_error_start + echo $(($blocksize/512)) > /sys/module/scsi_debug/parameters/medium_error_count + + if [ $enable_error -eq 1 ]; then + echo Enabling error >> $seqres.full + echo 2 > /sys/module/scsi_debug/parameters/opts + fi + + grep ^ /sys/module/scsi_debug/parameters/{medium_error_start,medium_error_count,opts} >> $seqres.full + + echo "Re-read the extended attribute on $testfile, panics on IO errors" >> $seqres.full + sync # Let folk see where we failed in the results. + getfattr -d -m - $testfile >> $seqres.full 2>&1 # Panic here on failure +} + + +# TODO: Avoid assumptions about inode size using _xfs_get_inode_size and +# _xfs_get_inode_core_bytes, currently assuming 512 byte inodes. + +# aformat shortform +# Include shortform for completeness, we can only inject errors for attributes +# stored outside of the inode. +test_attr "attr_local" 1 0 + +# aformat extents +# Single leaf block, known to panic +test_attr "attr_extent_one_block" 512 0 + +# Other tests to exercise multi block extents and multi extent attribute forks. +# Before the panic was understood it seemed possible that failing on the second +# block of a multi block attribute fork was involved. Seems like it may be +# worth testing in the future. + +test_attr "attr_extent_two_blocks_1" 5000 0 +test_attr "attr_extent_two_blocks_2" 5000 1 +# test_attr "attr_extent_many_blocks" 16000 4 +# +# When using a single name=value pair to fill the space these tend to push out +# to multiple extents, rather than a single long extent, so don't yet test +# failing subsequent blocks within the first extent. +# +# i.e. +# xfs_bmap -va $testfile +# /mnt/test/testfile: +# EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL +# 0: [0..7]: 96..103 0 (96..103) 8 +# 1: [8..23]: 80..95 0 (80..95) 16 + +# aformat btree +# test_attr "attr_extents" 50000 0 +# test_attr "attr_extents" 50000 $blocksize +# test_attr "attr_extents" 50000 $((blocksize+1)) + +# success, all done +echo Silence is golden +status=0 +exit diff --git a/tests/xfs/999.out b/tests/xfs/999.out new file mode 100644 index 00000000..3b276ca8 --- /dev/null +++ b/tests/xfs/999.out @@ -0,0 +1,2 @@ +QA output created by 999 +Silence is golden -- 2.47.3 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] xfs: test case for handling io errors when reading extended attributes 2025-08-29 4:24 [PATCH v2] xfs: test case for handling io errors when reading extended attributes Donald Douwsma @ 2025-08-29 19:49 ` Zorro Lang 2025-09-02 4:12 ` Donald Douwsma 0 siblings, 1 reply; 3+ messages in thread From: Zorro Lang @ 2025-08-29 19:49 UTC (permalink / raw) To: Donald Douwsma Cc: linux-xfs, fstests, Eric Sandeen, Darrick J . Wong, Carlos Maiolino On Fri, Aug 29, 2025 at 02:24:19PM +1000, Donald Douwsma wrote: > We've seen reports from the field panicing in xfs_trans_brelse after > an io error for an attribute block. > > sd 0:0:23:0: [sdx] tag#271 CDB: Read(16) 88 00 00 00 00 00 9b df 5e 78 00 00 00 08 00 00 > critical medium error, dev sdx, sector 2615107192 op 0x0:(READ) flags 0x1000 phys_seg 1 prio class 2 > XFS (sdx1): metadata I/O error in "xfs_da_read_buf+0xe1/0x140 [xfs]" at daddr 0x9bdf5678 len 8 error 61 > BUG: kernel NULL pointer dereference, address: 00000000000000e0 > ... > RIP: 0010:xfs_trans_brelse+0xb/0xe0 [xfs] > ... > Call Trace: > <TASK> > ... > ? xfs_trans_brelse+0xb/0xe0 [xfs] > xfs_attr_leaf_get+0xb6/0xc0 [xfs] > xfs_attr_get+0xa0/0xd0 [xfs] > xfs_xattr_get+0x75/0xb0 [xfs] > __vfs_getxattr+0x53/0x70 > inode_doinit_use_xattr+0x63/0x180 > inode_doinit_with_dentry+0x196/0x510 > security_d_instantiate+0x2f/0x50 > d_splice_alias+0x46/0x2b0 > xfs_vn_lookup+0x8b/0xb0 [xfs] > __lookup_slow+0x84/0x130 > walk_component+0x158/0x1d0 > path_lookupat+0x6e/0x1c0 > filename_lookup+0xcf/0x1d0 > vfs_statx+0x8d/0x170 > vfs_fstatat+0x54/0x70 > __do_sys_newfstatat+0x26/0x60 > > As this is specific to ENODATA test using scsi_debug instead of dmerror > which returns EIO. > > Signed-off-by: Donald Douwsma <ddouwsma@redhat.com> > --- > tests/xfs/999 | 193 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/999.out | 2 + > 2 files changed, 195 insertions(+) > create mode 100755 tests/xfs/999 > create mode 100644 tests/xfs/999.out > > diff --git a/tests/xfs/999 b/tests/xfs/999 > new file mode 100755 > index 00000000..2a45eb7c > --- /dev/null > +++ b/tests/xfs/999 > @@ -0,0 +1,193 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2025 Red Hat Inc. All Rights Reserved. > +# > +# FS QA Test 999 > +# > +# Regression test for panic following IO error when reading extended attribute blocks > +# > +# XFS (sda): metadata I/O error in "xfs_da_read_buf+0xe1/0x140 [xfs]" at daddr 0x78 len 8 error 61 > +# BUG: kernel NULL pointer dereference, address: 00000000000000e0 > +# ... > +# RIP: 0010:xfs_trans_brelse+0xb/0xe0 [xfs] > +# > +# [53887.310123] Call Trace: > +# <TASK> > +# ? show_trace_log_lvl+0x1c4/0x2df > +# ? show_trace_log_lvl+0x1c4/0x2df > +# ? xfs_attr_leaf_get+0xb6/0xc0 [xfs] > +# ? __die_body.cold+0x8/0xd > +# ? page_fault_oops+0x134/0x170 > +# ? xfs_trans_read_buf_map+0x133/0x300 [xfs] > +# ? exc_page_fault+0x62/0x150 > +# ? asm_exc_page_fault+0x22/0x30 > +# ? xfs_trans_brelse+0xb/0xe0 [xfs] > +# xfs_attr_leaf_get+0xb6/0xc0 [xfs] > +# xfs_attr_get+0xa0/0xd0 [xfs] > +# xfs_xattr_get+0x88/0xd0 [xfs] > +# __vfs_getxattr+0x7b/0xb0 > +# inode_doinit_use_xattr+0x63/0x180 > +# inode_doinit_with_dentry+0x196/0x510 > +# security_d_instantiate+0x3a/0xb0 > +# d_splice_alias+0x46/0x2b0 > +# xfs_vn_lookup+0x8b/0xb0 [xfs] > +# __lookup_slow+0x81/0x130 > +# walk_component+0x158/0x1d0 > +# ? path_init+0x2c5/0x3f0 > +# path_lookupat+0x6e/0x1c0 > +# filename_lookup+0xcf/0x1d0 > +# ? tlb_finish_mmu+0x65/0x150 > +# vfs_statx+0x82/0x160 > +# vfs_fstatat+0x54/0x70 > +# __do_sys_newfstatat+0x26/0x60 > +# do_syscall_64+0x5c/0xe0 > +# > +# For SELinux enabled filesystems the attribute security.selinux will be > +# created before any additional attributes are added. Any attempt to open the > +# file will read this entry, leading to the panic being triggered as above, via > +# security_d_instantiate, rather than fgetxattr(2), to test via fgetxattr mount > +# with a fixed context= > +# > +# Kernels prior to v4.16 don't have medium_error_start, and only return errors > +# for a specific location, making scsi_debug unsuitable for checking old kernels. > +# See d9da891a892a scsi: scsi_debug: Add two new parameters to scsi_debug driver Haha, I think we don't need to write such long comment, how about shorten it a bit, for example remove the call trace details :) > +# > + > +. ./common/preamble > +_begin_fstest auto ^^^^ attr > + > +_fixed_by_kernel_commit ae668cd567a6 "xfs: do not propagate ENODATA disk errors into xattr code" > + > +# Override the default cleanup function. > +_cleanup() > +{ > + cd / > + rm -f $tmp.* > + _unmount $SCRATCH_MNT > + _put_scsi_debug_dev > +} > + > +# Import common functions. > +. ./common/scsi_debug > + > +_require_scsi_debug > +modinfo scsi_debug | grep -wq medium_error_start || _notrun "test requires scsi_debug medium_error_start" Oh, maybe we can improve _require_scsi_debug, help it to accept arguments, likes: _require_scsi_debug medium_error_start > +scsi_debug_dev=$(_get_scsi_debug_dev) > +test -b $scsi_debug_dev || _notrun "Failed to initialize scsi debug device" > +echo "SCSI debug device $scsi_debug_dev" >>$seqres.full > + > +_mkfs_dev $scsi_debug_dev >> $seqres.full > +_mount $scsi_debug_dev $SCRATCH_MNT >> $seqres.full Hmm... if you use $SCRATCH_MNT, you might need _require_scratch. You can set SCRATCH_DEV=$scsi_debug_dev, then use _scratch_mkfs and _scratch_mount and other _scratch_* functions in this case. > + > +blocksize=$(_get_block_size $SCRATCH_MNT) >> $seqres.full ^^^^^^^^^^^^^^^ what's this for? Better to use _get_file_block_size. > +echo Blocksize $blocksize >> $seqres.full > + > +# Use dry_run=1 when verifying the test to avoid panicking. > +enable_error=1 > +[ ${dry_run:-0} -eq 1 ] && enable_error=0 I think the dry_run is useless for this case in xfstests, except you hope to create a new global parameter named "dry_run" for xfstests. I think we don't need that. Please use other method to decide enable_error. Or always "enable error" if that's a necessary condition to reproduce the bug. > + > +test_attr() > +{ > + test=$1 > + testfile=$SCRATCH_MNT/$test > + attr_size=$2 # bytes > + error_at_block=${3:-0} Better to use "local" for variables in a function. > + shift 3 But there might be not the 3rd argument, as you set it to 0 by default. I didn't see you use any $4, $* or $@ below, so maybe we don't need this line ? > + > + value_size=$attr_size Looks like the attr_size and value_size are duplicated, due to I didn't see you change any of them below. > + > + echo 0 > /sys/module/scsi_debug/parameters/opts > + > + echo -e "\nTesting : $test" >> $seqres.full > + echo -e "attr size: $attr_size" >> $seqres.full > + echo -e "error at block: $error_at_block\n" >> $seqres.full > + > + touch $testfile > + > + inode=$(ls -i $testfile|awk '{print $1}') inode=$(stat -c '%i' $testfile) > + printf "inode: %d Testfile: %s\n" $inode $testfile >> $seqres.full > + setfattr -n user.test_attr -v $(printf "%0*d" $value_size $value_size) $testfile As this case is "attr" related, so you need these: . common/attr _require_attrs user $SETFATTR_PROG .... > + > + $XFS_IO_PROG -c "bmap -al" $testfile >> $seqres.full > + start_blocks=($($XFS_IO_PROG -c "bmap -al" $testfile | awk 'match($3, /[0-9]+/, a) {print a[0]}')) > + echo "Attribute fork extent(s) start at ${attrblocks[*]}" >> $seqres.full > + > + _unmount $SCRATCH_MNT >>$seqres.full 2>&1 > + > + echo "Dump inode $inode details with xfs_db" >> $seqres.full > + $XFS_DB_PROG -c "inode $inode" -c "print core.aformat core.naextents a" $scsi_debug_dev >> $seqres.full > + inode_daddr=$($XFS_DB_PROG -c "inode $inode" -c "daddr" $scsi_debug_dev | awk '{print $4}') _scratch_xfs_get_metadata_field ... > + echo inode daddr $inode_daddr >> $seqres.full > + > + > + _mount $scsi_debug_dev $SCRATCH_MNT >> $seqres.full > + > + if [[ start_blocks[0] -ne 0 ]]; then ^^^ > + # Choose the block to error, currently only works with a single extent. > + error_daddr=$((start_blocks[0] + error_at_block*blocksize/512)) > + else > + # Default to the inode daddr when no extents were found. > + # Fails reading the inode during stat, so arguably useless > + # even for testing the upper layers of getfattr. > + error_daddr=$inode_daddr I'm curious, won't this affect the inode core ? If inode core can't be read, won't this cause other problem which isn't as you expect? > + fi ^^^ Bad indentation ... > + > + echo "Setup scsi_debug to error when reading attributes from block" \ > + "$error_at_block at daddr $error_daddr" >> $seqres.full > + echo $error_daddr > /sys/module/scsi_debug/parameters/medium_error_start > + echo $(($blocksize/512)) > /sys/module/scsi_debug/parameters/medium_error_count > + > + if [ $enable_error -eq 1 ]; then > + echo Enabling error >> $seqres.full > + echo 2 > /sys/module/scsi_debug/parameters/opts > + fi > + > + grep ^ /sys/module/scsi_debug/parameters/{medium_error_start,medium_error_count,opts} >> $seqres.full > + > + echo "Re-read the extended attribute on $testfile, panics on IO errors" >> $seqres.full > + sync # Let folk see where we failed in the results. > + getfattr -d -m - $testfile >> $seqres.full 2>&1 # Panic here on failure If you don't care about the getfattr output, you don't need to call _getfattr, but please use $GETFATTR_PROG at least. > +} > + > + > +# TODO: Avoid assumptions about inode size using _xfs_get_inode_size and > +# _xfs_get_inode_core_bytes, currently assuming 512 byte inodes. > + > +# aformat shortform > +# Include shortform for completeness, we can only inject errors for attributes > +# stored outside of the inode. > +test_attr "attr_local" 1 0 > + > +# aformat extents > +# Single leaf block, known to panic > +test_attr "attr_extent_one_block" 512 0 > + > +# Other tests to exercise multi block extents and multi extent attribute forks. > +# Before the panic was understood it seemed possible that failing on the second > +# block of a multi block attribute fork was involved. Seems like it may be > +# worth testing in the future. > + > +test_attr "attr_extent_two_blocks_1" 5000 0 > +test_attr "attr_extent_two_blocks_2" 5000 1 > +# test_attr "attr_extent_many_blocks" 16000 4 > +# > +# When using a single name=value pair to fill the space these tend to push out > +# to multiple extents, rather than a single long extent, so don't yet test > +# failing subsequent blocks within the first extent. > +# > +# i.e. > +# xfs_bmap -va $testfile > +# /mnt/test/testfile: > +# EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL > +# 0: [0..7]: 96..103 0 (96..103) 8 > +# 1: [8..23]: 80..95 0 (80..95) 16 > + > +# aformat btree > +# test_attr "attr_extents" 50000 0 > +# test_attr "attr_extents" 50000 $blocksize > +# test_attr "attr_extents" 50000 $((blocksize+1)) Can you really get a btree aformat through this ? I doubt that. You might need lots of attribute entries, not only one attribute with huge data. > + > +# success, all done > +echo Silence is golden > +status=0 > +exit > diff --git a/tests/xfs/999.out b/tests/xfs/999.out > new file mode 100644 > index 00000000..3b276ca8 > --- /dev/null > +++ b/tests/xfs/999.out > @@ -0,0 +1,2 @@ > +QA output created by 999 > +Silence is golden > -- > 2.47.3 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] xfs: test case for handling io errors when reading extended attributes 2025-08-29 19:49 ` Zorro Lang @ 2025-09-02 4:12 ` Donald Douwsma 0 siblings, 0 replies; 3+ messages in thread From: Donald Douwsma @ 2025-09-02 4:12 UTC (permalink / raw) To: Zorro Lang Cc: linux-xfs, fstests, Eric Sandeen, Darrick J . Wong, Carlos Maiolino On 30/8/25 05:49, Zorro Lang wrote: > On Fri, Aug 29, 2025 at 02:24:19PM +1000, Donald Douwsma wrote: >> We've seen reports from the field panicing in xfs_trans_brelse after >> an io error for an attribute block. >> >> sd 0:0:23:0: [sdx] tag#271 CDB: Read(16) 88 00 00 00 00 00 9b df 5e 78 00 00 00 08 00 00 >> critical medium error, dev sdx, sector 2615107192 op 0x0:(READ) flags 0x1000 phys_seg 1 prio class 2 >> XFS (sdx1): metadata I/O error in "xfs_da_read_buf+0xe1/0x140 [xfs]" at daddr 0x9bdf5678 len 8 error 61 >> BUG: kernel NULL pointer dereference, address: 00000000000000e0 >> ... >> RIP: 0010:xfs_trans_brelse+0xb/0xe0 [xfs] >> ... >> Call Trace: >> <TASK> >> ... >> ? xfs_trans_brelse+0xb/0xe0 [xfs] >> xfs_attr_leaf_get+0xb6/0xc0 [xfs] >> xfs_attr_get+0xa0/0xd0 [xfs] >> xfs_xattr_get+0x75/0xb0 [xfs] >> __vfs_getxattr+0x53/0x70 >> inode_doinit_use_xattr+0x63/0x180 >> inode_doinit_with_dentry+0x196/0x510 >> security_d_instantiate+0x2f/0x50 >> d_splice_alias+0x46/0x2b0 >> xfs_vn_lookup+0x8b/0xb0 [xfs] >> __lookup_slow+0x84/0x130 >> walk_component+0x158/0x1d0 >> path_lookupat+0x6e/0x1c0 >> filename_lookup+0xcf/0x1d0 >> vfs_statx+0x8d/0x170 >> vfs_fstatat+0x54/0x70 >> __do_sys_newfstatat+0x26/0x60 >> >> As this is specific to ENODATA test using scsi_debug instead of dmerror >> which returns EIO. >> >> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com> >> --- >> tests/xfs/999 | 193 ++++++++++++++++++++++++++++++++++++++++++++++ >> tests/xfs/999.out | 2 + >> 2 files changed, 195 insertions(+) >> create mode 100755 tests/xfs/999 >> create mode 100644 tests/xfs/999.out >> >> diff --git a/tests/xfs/999 b/tests/xfs/999 >> new file mode 100755 >> index 00000000..2a45eb7c >> --- /dev/null >> +++ b/tests/xfs/999 >> @@ -0,0 +1,193 @@ >> +#!/bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (c) 2025 Red Hat Inc. All Rights Reserved. >> +# >> +# FS QA Test 999 >> +# >> +# Regression test for panic following IO error when reading extended attribute blocks >> +# >> +# XFS (sda): metadata I/O error in "xfs_da_read_buf+0xe1/0x140 [xfs]" at daddr 0x78 len 8 error 61 >> +# BUG: kernel NULL pointer dereference, address: 00000000000000e0 >> +# ... >> +# RIP: 0010:xfs_trans_brelse+0xb/0xe0 [xfs] >> +# >> +# [53887.310123] Call Trace: >> +# <TASK> >> +# ? show_trace_log_lvl+0x1c4/0x2df >> +# ? show_trace_log_lvl+0x1c4/0x2df >> +# ? xfs_attr_leaf_get+0xb6/0xc0 [xfs] >> +# ? __die_body.cold+0x8/0xd >> +# ? page_fault_oops+0x134/0x170 >> +# ? xfs_trans_read_buf_map+0x133/0x300 [xfs] >> +# ? exc_page_fault+0x62/0x150 >> +# ? asm_exc_page_fault+0x22/0x30 >> +# ? xfs_trans_brelse+0xb/0xe0 [xfs] >> +# xfs_attr_leaf_get+0xb6/0xc0 [xfs] >> +# xfs_attr_get+0xa0/0xd0 [xfs] >> +# xfs_xattr_get+0x88/0xd0 [xfs] >> +# __vfs_getxattr+0x7b/0xb0 >> +# inode_doinit_use_xattr+0x63/0x180 >> +# inode_doinit_with_dentry+0x196/0x510 >> +# security_d_instantiate+0x3a/0xb0 >> +# d_splice_alias+0x46/0x2b0 >> +# xfs_vn_lookup+0x8b/0xb0 [xfs] >> +# __lookup_slow+0x81/0x130 >> +# walk_component+0x158/0x1d0 >> +# ? path_init+0x2c5/0x3f0 >> +# path_lookupat+0x6e/0x1c0 >> +# filename_lookup+0xcf/0x1d0 >> +# ? tlb_finish_mmu+0x65/0x150 >> +# vfs_statx+0x82/0x160 >> +# vfs_fstatat+0x54/0x70 >> +# __do_sys_newfstatat+0x26/0x60 >> +# do_syscall_64+0x5c/0xe0 >> +# >> +# For SELinux enabled filesystems the attribute security.selinux will be >> +# created before any additional attributes are added. Any attempt to open the >> +# file will read this entry, leading to the panic being triggered as above, via >> +# security_d_instantiate, rather than fgetxattr(2), to test via fgetxattr mount >> +# with a fixed context= >> +# >> +# Kernels prior to v4.16 don't have medium_error_start, and only return errors >> +# for a specific location, making scsi_debug unsuitable for checking old kernels. >> +# See d9da891a892a scsi: scsi_debug: Add two new parameters to scsi_debug driver > > Haha, I think we don't need to write such long comment, how about shorten it a bit, > for example remove the call trace details :) I'll try. I wanted to document that this involves the call path through xfs_attr_get and xfs_attr_leaf_get, since I'd like to cover the different fork's in xfs_attr_get_ilocked (inlined in xfs_attr_get). > >> +# >> + >> +. ./common/preamble >> +_begin_fstest auto > ^^^^ > attr Ack > >> + >> +_fixed_by_kernel_commit ae668cd567a6 "xfs: do not propagate ENODATA disk errors into xattr code" >> + >> +# Override the default cleanup function. >> +_cleanup() >> +{ >> + cd / >> + rm -f $tmp.* >> + _unmount $SCRATCH_MNT >> + _put_scsi_debug_dev >> +} >> + >> +# Import common functions. >> +. ./common/scsi_debug >> + >> +_require_scsi_debug >> +modinfo scsi_debug | grep -wq medium_error_start || _notrun "test requires scsi_debug medium_error_start" > > Oh, maybe we can improve _require_scsi_debug, help it to accept arguments, likes: > > _require_scsi_debug medium_error_start I started down that track, but wanted to keep it self contained. Happy to move it there for you. >> +scsi_debug_dev=$(_get_scsi_debug_dev) >> +test -b $scsi_debug_dev || _notrun "Failed to initialize scsi debug device" >> +echo "SCSI debug device $scsi_debug_dev" >>$seqres.full >> + >> +_mkfs_dev $scsi_debug_dev >> $seqres.full >> +_mount $scsi_debug_dev $SCRATCH_MNT >> $seqres.full > > Hmm... if you use $SCRATCH_MNT, you might need _require_scratch. You can set > SCRATCH_DEV=$scsi_debug_dev, then use _scratch_mkfs and _scratch_mount and > other _scratch_* functions in this case. That would be clearer and allow the scratch options to be used, I'll check it out. > >> + >> +blocksize=$(_get_block_size $SCRATCH_MNT) >> $seqres.full > ^^^^^^^^^^^^^^^ > what's this for? Reflex, fixed. > Better to use _get_file_block_size. Ack >> +echo Blocksize $blocksize >> $seqres.full >> + >> +# Use dry_run=1 when verifying the test to avoid panicking. >> +enable_error=1 >> +[ ${dry_run:-0} -eq 1 ] && enable_error=0 > > I think the dry_run is useless for this case in xfstests, except you > hope to create a new global parameter named "dry_run" for xfstests. > I think we don't need that. Please use other method to decide enable_error. > Or always "enable error" if that's a necessary condition to reproduce the > bug. I agree a one off here is bad. But it was useful when developing a test script that was expected to panic. How else can folk validate tests designed to panic a system? >> + >> +test_attr() >> +{ >> + test=$1 >> + testfile=$SCRATCH_MNT/$test >> + attr_size=$2 # bytes >> + error_at_block=${3:-0} > > Better to use "local" for variables in a function. Ack >> + shift 3 > > But there might be not the 3rd argument, as you set it to 0 by default. > > I didn't see you use any $4, $* or $@ below, so maybe we don't need this > line ? Ouch, yes, removed. > >> + >> + value_size=$attr_size > > Looks like the attr_size and value_size are duplicated, due to I didn't > see you change any of them below. When I first started work on this it wasn't clear to me how the error from the SCSI layer provoked it. Was it only the first failing attr block, did it behave differently on the second, could it involve the name and value split across block boundaries. The variable value_size was the start of working out how to cover this. After Eric figured out the cause of the panic it became more obvious as to which caused it, but I think it may be worth putting some effort into covering the different possibilities. If anyone else thinks this is worth the effort to get more coverage this would need to calculate the sizes of name/value and count required to create the required block and extent layout. Taking into account the current size of the attribute fork, with any security attributes added on file create. Or I could simplify this back down to just the case that that triggers the panic. > >> + >> + echo 0 > /sys/module/scsi_debug/parameters/opts >> + >> + echo -e "\nTesting : $test" >> $seqres.full >> + echo -e "attr size: $attr_size" >> $seqres.full >> + echo -e "error at block: $error_at_block\n" >> $seqres.full >> + >> + touch $testfile >> + >> + inode=$(ls -i $testfile|awk '{print $1}') > > inode=$(stat -c '%i' $testfile) Ack, much nicer >> + printf "inode: %d Testfile: %s\n" $inode $testfile >> $seqres.full >> + setfattr -n user.test_attr -v $(printf "%0*d" $value_size $value_size) $testfile > > As this case is "attr" related, so you need these: > > . common/attr > > _require_attrs user > > $SETFATTR_PROG .... Ack > >> + >> + $XFS_IO_PROG -c "bmap -al" $testfile >> $seqres.full >> + start_blocks=($($XFS_IO_PROG -c "bmap -al" $testfile | awk 'match($3, /[0-9]+/, a) {print a[0]}')) >> + echo "Attribute fork extent(s) start at ${attrblocks[*]}" >> $seqres.full >> + >> + _unmount $SCRATCH_MNT >>$seqres.full 2>&1 >> + >> + echo "Dump inode $inode details with xfs_db" >> $seqres.full >> + $XFS_DB_PROG -c "inode $inode" -c "print core.aformat core.naextents a" $scsi_debug_dev >> $seqres.full >> + inode_daddr=$($XFS_DB_PROG -c "inode $inode" -c "daddr" $scsi_debug_dev | awk '{print $4}') > > _scratch_xfs_get_metadata_field ... OK > >> + echo inode daddr $inode_daddr >> $seqres.full >> + >> + >> + _mount $scsi_debug_dev $SCRATCH_MNT >> $seqres.full >> + >> + if [[ start_blocks[0] -ne 0 ]]; then > ^^^ You mean the white space in the indent? If so ack. >> + # Choose the block to error, currently only works with a single extent. >> + error_daddr=$((start_blocks[0] + error_at_block*blocksize/512)) >> + else >> + # Default to the inode daddr when no extents were found. >> + # Fails reading the inode during stat, so arguably useless >> + # even for testing the upper layers of getfattr. >> + error_daddr=$inode_daddr > > I'm curious, won't this affect the inode core ? If inode core can't be read, won't > this cause other problem which isn't as you expect? Yes, the read for the inode fails and getfattr(1) will never get far enough to call getfattr(2) to read the attribute. But does end up testing how getfattr(1) handles this case. >> + fi > ^^^ > > Bad indentation ... > >> + >> + echo "Setup scsi_debug to error when reading attributes from block" \ >> + "$error_at_block at daddr $error_daddr" >> $seqres.full >> + echo $error_daddr > /sys/module/scsi_debug/parameters/medium_error_start >> + echo $(($blocksize/512)) > /sys/module/scsi_debug/parameters/medium_error_count >> + >> + if [ $enable_error -eq 1 ]; then >> + echo Enabling error >> $seqres.full >> + echo 2 > /sys/module/scsi_debug/parameters/opts >> + fi >> + >> + grep ^ /sys/module/scsi_debug/parameters/{medium_error_start,medium_error_count,opts} >> $seqres.full >> + >> + echo "Re-read the extended attribute on $testfile, panics on IO errors" >> $seqres.full >> + sync # Let folk see where we failed in the results. >> + getfattr -d -m - $testfile >> $seqres.full 2>&1 # Panic here on failure > > If you don't care about the getfattr output, you don't need to call _getfattr, but > please use $GETFATTR_PROG at least. Ack, I'll look into _getfattr or use $GETFATTR_PROG > >> +} >> + >> + >> +# TODO: Avoid assumptions about inode size using _xfs_get_inode_size and >> +# _xfs_get_inode_core_bytes, currently assuming 512 byte inodes. >> + >> +# aformat shortform >> +# Include shortform for completeness, we can only inject errors for attributes >> +# stored outside of the inode. >> +test_attr "attr_local" 1 0 >> + >> +# aformat extents >> +# Single leaf block, known to panic >> +test_attr "attr_extent_one_block" 512 0 >> + >> +# Other tests to exercise multi block extents and multi extent attribute forks. >> +# Before the panic was understood it seemed possible that failing on the second >> +# block of a multi block attribute fork was involved. Seems like it may be >> +# worth testing in the future. >> + >> +test_attr "attr_extent_two_blocks_1" 5000 0 >> +test_attr "attr_extent_two_blocks_2" 5000 1 >> +# test_attr "attr_extent_many_blocks" 16000 4 >> +# >> +# When using a single name=value pair to fill the space these tend to push out >> +# to multiple extents, rather than a single long extent, so don't yet test >> +# failing subsequent blocks within the first extent. >> +# >> +# i.e. >> +# xfs_bmap -va $testfile >> +# /mnt/test/testfile: >> +# EXT: FILE-OFFSET BLOCK-RANGE AG AG-OFFSET TOTAL >> +# 0: [0..7]: 96..103 0 (96..103) 8 >> +# 1: [8..23]: 80..95 0 (80..95) 16 >> + >> +# aformat btree >> +# test_attr "attr_extents" 50000 0 >> +# test_attr "attr_extents" 50000 $blocksize >> +# test_attr "attr_extents" 50000 $((blocksize+1)) > > Can you really get a btree aformat through this ? I doubt that. You > might need lots of attribute entries, not only one attribute with huge > data. This is indeed the question, but the code supports btrees. Thanks for all the feedback! Now I just need to test the changes, with dry_run=1 ./check tests/xfs/999 Before removing dry_run :) Don ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-09-02 4:12 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-29 4:24 [PATCH v2] xfs: test case for handling io errors when reading extended attributes Donald Douwsma 2025-08-29 19:49 ` Zorro Lang 2025-09-02 4:12 ` Donald Douwsma
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.