在 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.
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
