Re: [PATCH] btrfs/012: fix false alerts when SELinux is enabled

[Anand Jain <anand.jain@oracle.com>]

On 23/10/24 12:12, Zorro Lang wrote:
> On Tue, Oct 22, 2024 at 01:12:15PM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/10/19 08:45, Anand Jain 写道:
>>> On 18/10/24 08:04, Qu Wenruo wrote:
>>>> [FALSE FAILURE]
>>>> If SELinux is enabled, the test btrfs/012 will fail due to metadata
>>>> mismatch:
>>>>
>>>> FSTYP         -- btrfs
>>>> PLATFORM      -- Linux/x86_64 localhost 6.4.0-150600.23.25-default #1
>>>> SMP PREEMPT_DYNAMIC Tue Oct  1 10:54:01 UTC 2024 (ea7c56d)
>>>> MKFS_OPTIONS  -- /dev/loop1
>>>> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/loop1 /
>>>> mnt/scratch
>>>>
>>>> btrfs/012       - output mismatch (see /home/adam/xfstests-dev/
>>>> results//btrfs/012.out.bad)
>>>>       --- tests/btrfs/012.out    2024-10-18 10:15:29.132894338 +1030
>>>>       +++ /home/adam/xfstests-dev/results//btrfs/012.out.bad
>>>> 2024-10-18 10:25:51.834819708 +1030
>>>>       @@ -1,6 +1,1390 @@
>>>>        QA output created by 012
>>>>        Checking converted btrfs against the original one:
>>>>       -OK
>>>>       +metadata mismatch in /p0/d2/f4
>>>>       +metadata mismatch in /p0/d2/f5
>>>>       +metadata and data mismatch in /p0/d2/
>>>>       +metadata and data mismatch in /p0/
>>>>       ...
>>>>
>>>> [CAUSE]
>>>> All the mismatch happens in the metadata, to be more especific, it's the
>>>> security xattrs.
>>>>
>>>> Although btrfs-convert properly convert all xattrs including the
>>>> security ones, at mount time we will get new SELinux labels, causing the
>>>> mismatch between the converted and original fs.
>>>>
>>>> [FIX]
>>>> Override SELINUX_MOUNT_OPTIONS so that we will not touch the security
>>>> xattrs, and that should fix the false alert.
>>>>
>>>> Reported-by: Long An <lan@suse.com>
>>>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1231524
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    tests/btrfs/012 | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/tests/btrfs/012 b/tests/btrfs/012
>>>> index b23e039f4c9f..5811b3b339cb 100755
>>>> --- a/tests/btrfs/012
>>>> +++ b/tests/btrfs/012
>>>> @@ -32,6 +32,11 @@ _require_extra_fs ext4
>>>>    BASENAME="stressdir"
>>>>    BLOCK_SIZE=`_get_block_size $TEST_DIR`
>>>> +# Override the SELinux mount options, or it will lead to unexpected
>>>> +# different security.selinux between the original and converted fs,
>>>> +# causing false metadata mismatch during fssum.
>>>> +export SELINUX_MOUNT_OPTIONS=""
>>>> +
>>>
>>> SELINUX_MOUNT_OPTIONS is set only when SELinux is enabled on the system,
>>> so disabling SELinux will suffice.
>>
>> Are you suggesting to disable SELinux just to pass the test case?
>>
>> Then it doesn't sound correct to me at all.
>>
>> It should be the test case to adapt to all kinds of systems, not the
>> other way.
> 
> Hi Anand, I think Qu is right, it's not worth disable the whole SELinux
> (at the beginning of fstests running), just for a single test case.
> I just hope to make sure btrfs forks agree this's a failure which should
> be fixed in test side, but not change the selinux config for btrfs-progs.
> If you're sure about it, I'll merge this patch :)
> 

Yes, I realized that a bit later.

Reviewed-by: Anand Jain <anand.jain@oracle.com>



Even if we create _require_selinux() and _reset_selinux_mount_options(), 
there are only a few consumers, such as btrfs/075 and generic/700 for 
the former, and btrfs/008, btrfs/019, and generic/700 for the latter.
Do you think it is better?

Thx, Anand


> Thanks,
> Zorro
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> -------
>>> fstests/common/config:
>>> if [ -x /usr/sbin/selinuxenabled ] && /usr/sbin/selinuxenabled; then
>>>           : ${SELINUX_MOUNT_OPTIONS:="-o context=$(stat -c %C /)"}
>>>           export SELINUX_MOUNT_OPTIONS
>>> fi
>>> ----------
>>>
>>> Thanks, Anand
>>>
>>>>    # Create & populate an ext4 filesystem
>>>>    $MKFS_EXT4_PROG -F -b $BLOCK_SIZE $SCRATCH_DEV > $seqres.full 2>&1 || \
>>>>        _notrun "Could not create ext4 filesystem"
>>>
>>>
>>
>>
>