On Fri, 21 Feb 2025 at 09:20, Qu Wenruo <wqu@xxxxxxxx> wrote:
>
>
>
> 在 2025/2/21 18:31, Daniel Vacek 写道:
> > On Fri, 21 Feb 2025 at 08:45, Qu Wenruo <wqu@xxxxxxxx> wrote:
> >>
> >>
> >>
> >> 在 2025/2/21 18:10, Daniel Vacek 写道:
> >>> On Thu, 20 Feb 2025 at 22:54, Qu Wenruo <wqu@xxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> 在 2025/2/21 08:06, Qu Wenruo 写道:
> >>>>>
> >>>>>
> >>>>> 在 2025/2/21 01:27, Daniel Vacek 写道:
> >>>>>> When SELinux is enabled this test fails unable to receive a file with
> >>>>>> security label attribute:
> >>>>>>
> >>>>>> --- tests/btrfs/314.out
> >>>>>> +++ results//btrfs/314.out.bad
> >>>>>> @@ -17,5 +17,6 @@
> >>>>>> At subvol TEST_DIR/314/tempfsid_mnt/snap1
> >>>>>> Receive SCRATCH_MNT
> >>>>>> At subvol snap1
> >>>>>> +ERROR: lsetxattr foo
> >>>>>> security.selinux=unconfined_u:object_r:unlabeled_t:s0 failed:
> >>>>>> Operation not supported
> >>>>>> Send: 42d69d1a6d333a7ebdf64792a555e392 TEST_DIR/314/
> >>>>>> tempfsid_mnt/foo
> >>>>>> -Recv: 42d69d1a6d333a7ebdf64792a555e392 SCRATCH_MNT/snap1/foo
> >>>>>> +Recv: d41d8cd98f00b204e9800998ecf8427e SCRATCH_MNT/snap1/foo
> >>>>>> ...
> >>>>>>
> >>>>>> Setting the security label file attribute fails due to the default mount
> >>>>>> option implied by fstests:
> >>>>>>
> >>>>>> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sdb /mnt/
> >>>>>> scratch
> >>>>>>
> >>>>>> See commit 3839d299 ("xfstests: mount xfs with a context when selinux
> >>>>>> is on")
> >>>>>>
> >>>>>> fstests by default mount test and scratch devices with forced SELinux
> >>>>>> context to get rid of the additional file attributes when SELinux is
> >>>>>> enabled. When a test mounts additional devices from the pool, it may need
> >>>>>> to honor this option to keep on par. Otherwise failures may be expected.
> >>>>>>
> >>>>>> Moreover this test is perfectly fine labeling the files so let's just
> >>>>>> disable the forced context for this one.
> >>>>>>
> >>>>>> Signed-off-by: Daniel Vacek <neelx@xxxxxxxx>
> >>>>>
> >>>>> Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
> >>>>>
> >>>>> Thanks,
> >>>>> Qu
> >>>>>
> >>>>>> ---
> >>>>>> tests/btrfs/314 | 6 +++++-
> >>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/tests/btrfs/314 b/tests/btrfs/314
> >>>>>> index 76dccc41..cc1a2264 100755
> >>>>>> --- a/tests/btrfs/314
> >>>>>> +++ b/tests/btrfs/314
> >>>>>> @@ -21,6 +21,10 @@ _cleanup()
> >>>>>> . ./common/filter.btrfs
> >>>>>> +# Disable the forced SELinux context. We are fine testing the
> >>>>>> +# security labels with this test when SELinux is enabled.
> >>>>>> +SELINUX_MOUNT_OPTIONS=
> >>>>
> >>>> Wait for a minute, this means you're disabling SELINUX mount options
> >>>> completely.
> >>>>
> >>>> I'm not sure if this is really needed.
> >>>>>> +
> >>>>>> _require_scratch_dev_pool 2
> >>>>>> _require_btrfs_fs_feature temp_fsid
> >>>>>> @@ -38,7 +42,7 @@ send_receive_tempfsid()
> >>>>>> # Use first 2 devices from the SCRATCH_DEV_POOL
> >>>>>> mkfs_clone ${SCRATCH_DEV} ${SCRATCH_DEV_NAME[1]}
> >>>>>> _scratch_mount
> >>>>>> - _mount ${SCRATCH_DEV_NAME[1]} ${tempfsid_mnt}
> >>>>>> + _mount ${SELINUX_MOUNT_OPTIONS} ${SCRATCH_DEV_NAME[1]}
> >>>>>> ${tempfsid_mnt}
> >>>>
> >>>> The problem of the old code is it doesn't have any SELinux related mount
> >>>> option, thus later receive will fail to set SELinux context.
> >>>>
> >>>> But since you have already added SELINUX_MOUNT_OPTIONS, I think you do
> >>>> not need to disable the SELINUX_MOUNT_OPTIONS.
> >>>>
> >>>> Have you tested with only this change, without resetting
> >>>> SELINUX_MOUNT_OPTIONS?
> >>>
> >>> Yes, I tested both. Actually resetting this option was the first fix I
> >>> came up with, that's why I kept it. This option breaks the test case
> >>> when SELinux is enabled.
> >>> But then I figured the other way around (using the option consistently
> >>> with all the mounts) also works. So I added it for consistency.
> >>> At that point, resetting the option is not really strictly needed
> >>> anymore (as you correctly suspect).
> >>>
> >>> So there are two possible solutions. Each one makes the testcase 314
> >>> pass. But they can as well be combined.
> >>
> >> Resetting the SELINUX one is not a good solution, that just means we
> >> reduce the coverage (No more SELINUX coverage for this test case anymore).
> >
> > I understand it's the other way around. Forcing a default mount
> > context basically disables SELinux. Well, more precisely it partially
> > cripples it.
> > Removing this option enables the usual default SELinux behavior. Note,
> > SELinux is always enabled unless you cripple it.
>
> Nope, it's the other way around.
>
> You only disable SELinux when there is a reason that SELinux context is
> going to change.
> E.g. we're mounting two different filesystems, like btrfs/012.
>
> Which can created different SELinux context since the converted fs is a
> different one (Well, completely different fs type).
>
> Unless you have a strong reason that the security context is definitely
> going to be different, you should not override the existing one.
> Especially I believe the mount fix is already enough.
>
> Then you have no reason to keep the SELINUX override.
>
>
> Remember, user can provide their own mount options (including the
> SELinux ones) through MOUNT_OPTIONS environmental variables.
>
> So you at least need a full reason why SElinux context must be disable
> for this case.
> And I see none.
It does not need to be disabled. But it also does not have to be
enabled for this test.
At least not with the default policy on the latest Tumbleweed I was testing on.
But your mileage may vary, I guess.
> >
> > Or do you use such an option with any of your mounts? I doubt so.
> >
> > Check the mentioned commit 3839d299. fstests cripple SELinux by
> > default. Which doesn't look good by itself.
>
> Do you really believe that commit is going crippling SELINUX?
Well, in a way, yes.
What it does is that it overrides the system's default policy. Which
may make sense, as for example your system policy may deny some
operations the tests do, eventually resulting in failed tests.
Though as a side-effect it also prevents writing the security label
file attribute by design with the mount option override. In such a
case SELinux just returns with -EOPNOTSUPP.
3213 sbsec = selinux_superblock(inode->i_sb);
3214 if (!(sbsec->flags & SBLABEL_MNT))
3215 return -EOPNOTSUPP;
... ^^^^^^^^^^^^^^^^^^^
The documentation says that this option is usually used with external
devices like USB flash key-drives or filesystems where you do not want
to mess/break their labels. Like mounting a filesystem from another
machine.
Or eventually this option can be used for filesystems which do not
support extended file attributes, like for example the FAT filesystem.
In that case all files inherit the forced mount label and SELinux
treats them using that context.
> All it does are just:
>
> - Allow scratch mount filter to ripple off selinux context
> This is only to make certain golden output to skip the SElinux ones.
>
> - Make sure scratch mount follows the SELINUX context
>
> Please explain why you believe that commit "cripples" the whole SELinux
> thing.
Well, maybe not cripples. It overrides the system policy.
The SELinux is always tested, just with different options/configuration.
Thinking about it again, I guess fstests are not supposed to test the
SELinux policy and it's better to override it. SELinux should have
it's own tests after all.
That said, I'm fine with dropping the first hunk when merged.
Thank you for the review, btw.
> >
> > At least I'd say it's good for diversity to have one test different.
> > Diverse tests are prefered with testing, right?
>
> What diversity? You just ripped off the whole SELinux for this test
> case, that's killing the diversity.
>
> Reasons please, and "just to make it pass" doesn't count.
>
> >
> >> So please only fix the mount command (with the extra selinux mount
> >> option), without overriding the existing SElinux config.
> >>
> >> Thanks,
> >> Qu
> >>>
> >>> Anyways, this test is fine without forcing the default mount context,
> >>> which is more a bandaid for other fstests, IIUC. There's no need for
> >>> this option in this case, at least with my testing. Hence I disabled
> >>> it. Does it fail for you?
> >>>
> >>>> Thanks,
> >>>> Qu
> >>>>>> $XFS_IO_PROG -fc 'pwrite -S 0x61 0 9000' ${src}/foo |
> >>>>>> _filter_xfs_io
> >>>>>> _btrfs subvolume snapshot -r ${src} ${src}/snap1
> >>>>>
> >>>>>
> >>>>
> >>
>
