On Fri, 21 Feb 2025 at 10:48, Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote:
>
>
>
> 在 2025/2/21 19:43, Daniel Vacek 写道:
> > On Fri, 21 Feb 2025 at 09:20, Qu Wenruo <wqu@xxxxxxxx> 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
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>
> >
>
