On Fri, Dec 17, 2021 at 1:08 PM Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>
> Can you please elaborate on what is problematic with the two patches
> you've highlighted.
Commit ec1ade6a0448 ("nfs: account for selinux security context when
deciding to share superblock") adds the call to
security_sb_mnt_opts_compat() from the nfs_compare_mount_options()
function.
But nfs_compare_mount_options() is called from nfs_compare_super(),
which is used as the the "test" callback function for the "sget_fc()"
call:
s = sget_fc(fc, compare_super, nfs_set_super);
and sget_fc() traverses all the mounted filesystems of this type -
while holding the superblock lock that protects that list.
So nfs_compare_super() may not sleep. It's called while holding a very
core low-level lock, and it really is supposed to only do a "test".
Not some complex operation that may do dynamic memory allocations and
sleep.
Yet that is exactly what security_sb_mnt_opts_compat() does, as done
in 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an
existing mount").
So those two patches are completely broken.
Now, commit cc274ae7763d ("selinux: fix sleeping function called from
invalid context") that I just merged "fixes" this by making the
allocations in parse_sid() be GFP_NOWAIT.
That is a *HORRIBLE* fix. It's a horrible fix because
(a) GFP_NOWAIT can fail very easily, causing the mount to randomly
fail for non-obvious reasons.
(b) even when it doesn't fail, you really shouldn't do things like
this under a very core spinlock.
Also, the original place - nfs_compare_mount_options() is called over
and over for each mount, so you're parsing those same mount options
over and over again. So not only was this sequence buggy, it's really
not very smart to begin with.
That's why I say that a much better approach would have been to parse
the mount options _once_ at the beginning, saving them off in some
temporary supoerblock (or whatever - anything that can hold those
pre-parsed mount options), and then have the "test" callback literally
just check those parsed options.
That's not necessarily the only way to go about this - there are
probably other approaches too, I didn't really think too much about
this. But those two commits on their own are buggy, and the fix is
somewhat problematic too.
Linus
