| 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
| ||
|
Message-ID: <7bf1c177-1213-4c35-80bc-620d02a441c2@linux.ibm.com>
Date: Fri, 22 Nov 2024 23:37:17 +0530
From: Nirjhar Roy <nirjhar@...ux.ibm.com>
To: "Darrick J. Wong" <djwong@...nel.org>,
Ritesh Harjani <ritesh.list@...il.com>
Cc: fstests@...r.kernel.org, linux-ext4@...r.kernel.org,
linux-xfs@...r.kernel.org, ojaswin@...ux.ibm.com, zlang@...nel.org
Subject: Re: [PATCH v3 2/3] common/rc: Add a new _require_scratch_extsize
helper function
On 11/22/24 21:34, Darrick J. Wong wrote:
> On Fri, Nov 22, 2024 at 12:22:41AM +0530, Ritesh Harjani wrote:
>> Nirjhar Roy <nirjhar@...ux.ibm.com> writes:
>>
>>> On 11/21/24 13:23, Ritesh Harjani (IBM) wrote:
>>>> Nirjhar Roy <nirjhar@...ux.ibm.com> writes:
>>>>
>>>>> _require_scratch_extsize helper function will be used in the
>>>>> the next patch to make the test run only on filesystems with
>>>>> extsize support.
>>>>>
>>>>> Reviewed-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
>>>>> Signed-off-by: Nirjhar Roy <nirjhar@...ux.ibm.com>
>>>>> ---
>>>>> common/rc | 17 +++++++++++++++++
>>>>> 1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/common/rc b/common/rc
>>>>> index cccc98f5..995979e9 100644
>>>>> --- a/common/rc
>>>>> +++ b/common/rc
>>>>> @@ -48,6 +48,23 @@ _test_fsxattr_xflag()
>>>>> grep -q "fsxattr.xflags.*\[.*$2.*\]" <($XFS_IO_PROG -c "stat -v" "$1")
>>>>> }
>>>>>
>>>>> +# This test requires extsize support on the filesystem
>>>>> +_require_scratch_extsize()
>>>>> +{
>>>>> + _require_scratch
>>>> _require_xfs_io_command "extsize"
>>>>
>>>> ^^^ Don't we need this too?
>>> Yes, good point. I will add this in the next revision.
>>>>> + _scratch_mkfs > /dev/null
>>>>> + _scratch_mount
>>>>> + local filename=$SCRATCH_MNT/$RANDOM
>>>>> + local blksz=$(_get_block_size $SCRATCH_MNT)
>>>>> + local extsz=$(( blksz*2 ))
>>>>> + local res=$($XFS_IO_PROG -c "open -f $filename" -c "extsize $extsz" \
>>>>> + -c "extsize")
>>>>> + _scratch_unmount
>>>>> + grep -q "\[$extsz\] $filename" <(echo $res) || \
>>>>> + _notrun "this test requires extsize support on the filesystem"
>>>> Why grep when we can simply just check the return value of previous xfs_io command?
>>> No, I don't think we can rely on the return value of xfs_io. For ex,
>>> let's look at the following set of commands which are ran on an ext4 system:
>>>
>>> root@...RPC: /mnt1/test$ xfs_io -V
>>> xfs_io version 5.13.0
>>> root@...RPC: /mnt1/test$ touch new
>>> root@...RPC: /mnt1/test$ xfs_io -c "extsize 8k" new
>>> foreign file active, extsize command is for XFS filesystems only
>>> root@...RPC: /mnt1/test$ echo "$?"
>>> 0
>>> This incorrect return value might have been fixed in some later versions
>>> of xfs_io but there are still versions where we can't solely rely on the
>>> return value.
>> Ok. That's bad, we then have to rely on grep.
>> Sure, thanks for checking and confirming that.
> You all should add CMD_FOREIGN_OK to the extsize command in xfs_io,
> assuming that you've not already done that in your dev workspace.
>
> --D
Yes, I have tested with that as well. I have applied the following patch
to xfsprogs and tested with the modified xfs_io.
diff --git a/io/open.c b/io/open.c
index 15850b55..6407b7e8 100644
--- a/io/open.c
+++ b/io/open.c
@@ -980,7 +980,7 @@ open_init(void)
extsize_cmd.args = _("[-D | -R] [extsize]");
extsize_cmd.argmin = 0;
extsize_cmd.argmax = -1;
- extsize_cmd.flags = CMD_NOMAP_OK;
+ extsize_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
extsize_cmd.oneline =
_("get/set preferred extent size (in bytes) for the
open file");
extsize_cmd.help = extsize_help;
The return values are similar.
root@...RPC: /mnt1/scratch$ touch new
root@...RPC: /mnt1/scratch$ /home/ubuntu/xfsprogs-dev/io/xfs_io -c
"extsize 8k" new
root@...RPC: /mnt1/scratch$ echo "$?"
0
root@...RPC: /mnt1/scratch$ /home/ubuntu/xfsprogs-dev/io/xfs_io -c
"extsize" new
[0] new
This is the reason I am not relying on the return value, instead I am
checking if only the extsize gets changed, we will assume that the
extsize support is there, else the test will _notrun.
Also,
root@...RPC: /mnt1/scratch$ strace -f /mnt1/scratch$
/home/ubuntu/xfsprogs-dev/io/xfs_io -c "extsize 16k" new
...
...
ioctl(3, FS_IOC_FSGETXATTR, {fsx_xflags=0, fsx_extsize=0,
fsx_nextents=0, fsx_projid=0, fsx_cowextsize=0}) = 0
ioctl(3, FS_IOC_FSSETXATTR, {fsx_xflags=FS_XFLAG_EXTSIZE,
fsx_extsize=16384, fsx_projid=0, fsx_cowextsize=0}) = 0
exit_group(0)
Looking at the existing code for ext4_fileattr_set(), We validate the
flags but I think we silently don't validate(and ignore) the xflags.
Like, we have
int err = -EOPNOTSUPP;
if (flags & ~EXT4_FL_USER_VISIBLE)
goto out;
BUT we do NOT have something like
int err = -EOPNOTSUPP;
if (fa->fsx_flags & ~EXT4_VALID_XFLAGS) // where EXT4_VALID_XFLAGS
should be an || of all the supported xflags in ext4.
goto out;
I am not sure what other filesystems do, but if we check whether the
extsize got changed, then we can correctly determine extsize support.
Does that make sense?
--NR
>
>> -ritesh
>>
--
---
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore
Powered by blists - more mailing lists