| Message ID | 20250220145723.1526907-1-neelx@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fstests: btrfs/314: fix the failure when SELinux is enabled | expand |
在 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@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> 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= > + > _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} > > $XFS_IO_PROG -fc 'pwrite -S 0x61 0 9000' ${src}/foo | _filter_xfs_io > _btrfs subvolume snapshot -r ${src} ${src}/snap1
在 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@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > 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? Thanks, Qu >> $XFS_IO_PROG -fc 'pwrite -S 0x61 0 9000' ${src}/foo | >> _filter_xfs_io >> _btrfs subvolume snapshot -r ${src} ${src}/snap1 > >
On Thu, 20 Feb 2025 at 22:54, Qu Wenruo <wqu@suse.com> 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@suse.com> > > > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > > > 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. 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 > > > > >
在 2025/2/21 18:10, Daniel Vacek 写道: > On Thu, 20 Feb 2025 at 22:54, Qu Wenruo <wqu@suse.com> 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@suse.com> >>> >>> Reviewed-by: Qu Wenruo <wqu@suse.com> >>> >>> 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). 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 >>> >>> >>
On Fri, 21 Feb 2025 at 08:45, Qu Wenruo <wqu@suse.com> wrote: > > > > 在 2025/2/21 18:10, Daniel Vacek 写道: > > On Thu, 20 Feb 2025 at 22:54, Qu Wenruo <wqu@suse.com> 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@suse.com> > >>> > >>> Reviewed-by: Qu Wenruo <wqu@suse.com> > >>> > >>> 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. 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. At least I'd say it's good for diversity to have one test different. Diverse tests are prefered with testing, right? > 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 > >>> > >>> > >> >
在 2025/2/21 18:31, Daniel Vacek 写道: > On Fri, 21 Feb 2025 at 08:45, Qu Wenruo <wqu@suse.com> wrote: >> >> >> >> 在 2025/2/21 18:10, Daniel Vacek 写道: >>> On Thu, 20 Feb 2025 at 22:54, Qu Wenruo <wqu@suse.com> 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@suse.com> >>>>> >>>>> Reviewed-by: Qu Wenruo <wqu@suse.com> >>>>> >>>>> 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. > > 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? 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. > > 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 >>>>> >>>>> >>>> >>
On Fri, 21 Feb 2025 at 09:20, Qu Wenruo <wqu@suse.com> wrote: > > > > 在 2025/2/21 18:31, Daniel Vacek 写道: > > On Fri, 21 Feb 2025 at 08:45, Qu Wenruo <wqu@suse.com> wrote: > >> > >> > >> > >> 在 2025/2/21 18:10, Daniel Vacek 写道: > >>> On Thu, 20 Feb 2025 at 22:54, Qu Wenruo <wqu@suse.com> 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@suse.com> > >>>>> > >>>>> Reviewed-by: Qu Wenruo <wqu@suse.com> > >>>>> > >>>>> 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 > >>>>> > >>>>> > >>>> > >> >
在 2025/2/21 19:43, Daniel Vacek 写道: > On Fri, 21 Feb 2025 at 09:20, Qu Wenruo <wqu@suse.com> wrote: [...] >> 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. Then good lucky if one day some QA guy finds out the send/receive behavior has a SELINUX specific bug, and you need to explain to them why it's a good idea to not test SELinux for this particular workload. Your mindset of "XX feature doesn't have to be tested" makes nosense for a test suite. > 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; > ... ^^^^^^^^^^^^^^^^^^^ This only explains why your mount option fix is correct. The send stream has SELinux attrs, that's because the original fs is mounted with SELinux context (the regular _scratch_mount() helper added SELinux context). But later we manually mounted a btrfs, not using _scratch_mount(), thus the new mounted btrfs doesn't have SELinux context, thus unable to set the SELinux attrs at receive side. It doesn't show why you need both the mount fix and overriding SELINUX context at all. > >> 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. Not if you override the SELINUX context, then the test case will never have SELinux tested. And you never know if someone in the future will find a bug in send/receive with SElinux enabled. And I do not think your "combining two working fixes is fine" mindset makes any sense either. Every fix should have a reason, if you have different ways to fix a test case, you need to evaluate the pros and cons. Overriding SElinux means we will never test SElinux for this particular workload, which is not a small trade-off. Fixing the mount command brings no obvious problem. So the choice should be obvious. But combining the two? You have all the cons (no more SElinux for this test case), but not any new benefit. > > Thinking about it again, I guess fstests are not supposed to test the > SELinux policy and it's better to override it. It not your call. > SELinux should have > it's own tests after all. Say it again loud to all the SElinux guys, and better CC them. > > 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 >>>>>>> >>>>>>> >>>>>> >>>> >> >
On Fri, 21 Feb 2025 at 10:48, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2025/2/21 19:43, Daniel Vacek 写道: > > On Fri, 21 Feb 2025 at 09:20, Qu Wenruo <wqu@suse.com> wrote: > [...] > >> 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. > > Then good lucky if one day some QA guy finds out the send/receive > behavior has a SELINUX specific bug, and you need to explain to them why > it's a good idea to not test SELinux for this particular workload. > > Your mindset of "XX feature doesn't have to be tested" makes nosense for > a test suite. Huh, this is a huge misunderstanding.You still got it the other way around. I'm sorry if I was confusing you. I never said I'm against testing SELinux. Quite the opposite, to be honest. I did argue that testing the default system policy without overriding the context also works for this test, at least with my testing on the latest TW. SELinux is always tested. Just either with the default system policy or with the given forced context. > > 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; > > ... ^^^^^^^^^^^^^^^^^^^ > > This only explains why your mount option fix is correct. Nope. This triggers precisely when the context is forced with the mount option. In that case setting the file attribute is not supported. > The send stream has SELinux attrs, that's because the original fs is > mounted with SELinux context (the regular _scratch_mount() helper added > SELinux context). Nope. Again, the other way around. The send stream has the `security.selinux` attribute precisely because the mount was missing the option and hence the file labels were used (and not refused). But the receive side fails as that mount actually did use the context mount option and so it was refusing setting the file label returning this -EOPNOTSUPP error. > But later we manually mounted a btrfs, not using _scratch_mount(), thus > the new mounted btrfs doesn't have SELinux context, thus unable to set > the SELinux attrs at receive side. This is just wrong. > It doesn't show why you need both the mount fix and overriding SELINUX > context at all. I'm saying you don't need both from the very beginning. Where did I say you do? > > > >> 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. > > Not if you override the SELINUX context, then the test case will never > have SELinux tested. Wrong. SELinux is always tested. Either with the system-defined policy or the overridden context. > And you never know if someone in the future will find a bug in > send/receive with SElinux enabled. More likely you may find the test failing in the future as you'd be testing on a distribution which changed it's policy to forbid access to /mnt path. Or you configure fstests to mount somewhere else where it's forbidden by the default system policy. Again, SELinux is always tested. And it may just be easier for fstests to force the context to prevent test failures due to the system default policy. That's why I also said that I'm fine with dropping that hunk. > And I do not think your "combining two working fixes is fine" mindset > makes any sense either. One is a fix, one is a configuration. You change the configuration (ie, keep the default system policy, no forced context) and it works. You apply the fix and it works no matter what configuration. So the configuration does not really need to be changed, which I'm saying from the beginning. But I left it there as no biggie. Now, after the discussion with you, I agree that fstests may be better forcing the context explicitly instead of relying on the default system policy. > Every fix should have a reason, if you have different ways to fix a test > case, you need to evaluate the pros and cons. The pro was to test the real thing and not force override it. But that may not be a good idea for fstests. > Overriding SElinux means we will never test SElinux for this particular > workload, which is not a small trade-off. Wrong. Again, SELinux is always used and tested. Only the policy is overridden or the default one. > Fixing the mount command brings no obvious problem. > > So the choice should be obvious. > > But combining the two? You have all the cons (no more SElinux for this > test case), but not any new benefit. You're just confused by what the configuration changes. SELinux is still tested. > > > > Thinking about it again, I guess fstests are not supposed to test the > > SELinux policy and it's better to override it. > > It not your call. Agreed. The intention was to cover it as an additional bonus but in the end that may not be wanted. In the end I agree fstests may be better to force the given context and not be eventually broken by the default system policy. Eventhough it does not seem likely to happen at this point. > > SELinux should have > > it's own tests after all. > > Say it again loud to all the SElinux guys, and better CC them. What I meant is that for sure they have way more fundamental tests then checking the correctness of the applied policy. That's more a user/devops thing. That's what we're talking here about, whether to use one policy or the other. Not whether to use SELinux as it's enabled in both cases. > > > > 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 > >>>>>>> > >>>>>>> > >>>>>> > >>>> > >> > > >
在 2025/2/21 22:23, Daniel Vacek 写道: > On Fri, 21 Feb 2025 at 10:48, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> 在 2025/2/21 19:43, Daniel Vacek 写道: >>> On Fri, 21 Feb 2025 at 09:20, Qu Wenruo <wqu@suse.com> wrote: >> [...] >>>> 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. >> >> Then good lucky if one day some QA guy finds out the send/receive >> behavior has a SELINUX specific bug, and you need to explain to them why >> it's a good idea to not test SELinux for this particular workload. >> >> Your mindset of "XX feature doesn't have to be tested" makes nosense for >> a test suite. > > Huh, this is a huge misunderstanding.You still got it the other way > around. I'm sorry if I was confusing you. > I never said I'm against testing SELinux. Quite the opposite, to be > honest. I did argue that testing the default system policy without > overriding the context also works for this test, at least with my > testing on the latest TW. > SELinux is always tested. Just either with the default system policy > or with the given forced context. > >>> 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; >>> ... ^^^^^^^^^^^^^^^^^^^ >> >> This only explains why your mount option fix is correct. > > Nope. This triggers precisely when the context is forced with the > mount option. In that case setting the file attribute is not > supported. BS, overriding the SELINUX context just means disabling SELINUX. Your "fix" from the very beginning is doing two conflicting things: - Disable SELINUX context by overriding it - Add back the empty SELinux context for mount Explain why it makes any sense? > >> The send stream has SELinux attrs, that's because the original fs is >> mounted with SELinux context (the regular _scratch_mount() helper added >> SELinux context). > > Nope. Again, the other way around. The send stream has the > `security.selinux` attribute precisely because the mount was missing > the option and hence the file labels were used (and not refused). There is no SELinux xattrs because you damn disabled SELinux for this test case!! But > the receive side fails as that mount actually did use the context > mount option and so it was refusing setting the file label returning > this -EOPNOTSUPP error. > >> But later we manually mounted a btrfs, not using _scratch_mount(), thus >> the new mounted btrfs doesn't have SELinux context, thus unable to set >> the SELinux attrs at receive side. > > This is just wrong. Whatever you believe, I think I have wasted too much time on this non-sense. > >> It doesn't show why you need both the mount fix and overriding SELINUX >> context at all. > > I'm saying you don't need both from the very beginning. Where did I say you do? Then why damn putting the override and mount fix in the same damn patch?
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= + _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} $XFS_IO_PROG -fc 'pwrite -S 0x61 0 9000' ${src}/foo | _filter_xfs_io _btrfs subvolume snapshot -r ${src} ${src}/snap1
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@suse.com> --- tests/btrfs/314 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)