On Sat, 22 Feb 2025 at 09:38, Zorro Lang <zlang@xxxxxxxxxx> wrote:
>
> On Thu, Feb 20, 2025 at 03:57:23PM +0100, Daniel Vacek wrote:
> > 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>
> > ---
>
> Take it easy, Thanks for both of you would like to help fstests to get
> better :)
>
> Firstly, SELINUX_MOUNT_OPTIONS isn't a parameter to enable or disable
> SELinux test. We just use it to avoid tons of ondisk selinux lables to
> mess up the testing. So mount with a specified SELINUX_MOUNT_OPTIONS
> to avoid new ondisk selinux labels always be created in each file's
> extended attributes field.
Well put. I was trying to explain that.
In fact it does not mess with this test and that's why originally I
kept it and I did not remove it after coming up with the other way to
address the bug. I thought a bit of diversity could be useful with the
testing after all.
> Secondly, I don't want to attend the argument :) Just for this patch review,
> I prefer just doing:
>
> _scratch_mount
> - _mount ${SCRATCH_DEV_NAME[1]} ${tempfsid_mnt}
> + _mount $SELINUX_MOUNT_OPTIONS ${SCRATCH_DEV_NAME[1]} ${tempfsid_mnt}
I already agreed with dropping the other hunk before.
> or if you concern MOUNT_OPTIONS and SELINUX_MOUNT_OPTIONS both, maybe:
>
> _scratch_mount
> - _mount ${SCRATCH_DEV_NAME[1]} ${tempfsid_mnt}
> + _mount $(_common_dev_mount_options) ${SCRATCH_DEV_NAME[1]} ${tempfsid_mnt}
That's even better. I'll send a v2 with this change.
Thanks for the hint.
> That's enough to help "_scratch_mount" and later "_mount" use same
> SELINUX_MOUNT_OPTIONS, and fix the test failure you hit.
>
> About resetting "SELINUX_MOUNT_OPTIONS", I think btrfs/314 isn't a test case
> cares about SELinux labels on-disk or not. So how about don't touch it.
Yup, as I said, I already agreed with that in the previous conversation.
> If you'd like to talk about if xfstests cases should test with a specified
> SELINUX_MOUNT_OPTIONS mount option or not, you can send another patch to talk
> about 3839d299 ("xfstests: mount xfs with a context when selinux is on").
>
> Now let's fix this failure at first :)
>
> Thanks,
> Zorro
>
> > 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
> > --
> > 2.48.1
> >
> >
>
