| 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: <5ed6c877-38ec-4dae-9911-0af0bfcd1f9e@linux.ibm.com>
Date: Sat, 23 Nov 2024 00:36:26 +0530
From: Nirjhar Roy <nirjhar@...ux.ibm.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: Ritesh Harjani <ritesh.list@...il.com>, 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/23/24 00:14, Darrick J. Wong wrote:
> On Fri, Nov 22, 2024 at 11:37:17PM +0530, Nirjhar Roy wrote:
>> 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?
> You don't have to check fsx_flags if you don't use fileattr_fill_xflags.
> ext4 doesn't use that.
>
> --D
Okay got it. Thank you. How does the overall logic of the
_require_scratch_extsize() look?
>
>> --NR
>>
>>
>>
>>>> -ritesh
>>>>
>> --
>> ---
>> Nirjhar Roy
>> Linux Kernel Developer
>> IBM, Bangalore
>>
--
---
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore
Powered by blists - more mailing lists