On Mon, Jun 22, 2020 at 06:50:17PM -0700, Eric Biggers wrote:

On Tue, Jun 23, 2020 at 10:46:36AM +1000, Dave Chinner wrote:

quoted

On Wed, Jun 17, 2020 at 08:19:35PM -0700, Eric Biggers wrote:

quoted

Are you objecting to the use of a SB_* flag, or just to showing the flag in
show_sb_opts() instead of in the individual filesystems?  Note that the SB_*
flag was requested by Christoph
(https://lkml.kernel.org/r/20191031183217.GF23601@infradead.org/,
https://lkml.kernel.org/r/20191031212103.GA6244@infradead.org/).  We originally
used a function fscrypt_operations::inline_crypt_enabled() instead.

I'm objecting to the layering violations of having the filesystem
control the mount option parsing and superblock feature flags, but
then having no control over whether features that the filesystem has
indicated to the VFS it is using get emitted as a mount option or
not, and then having the VFS code unconditionally override the
functionality that the filesystem uses because it thinks it's a
mount option the filesystem supports....

For example, the current mess that has just come to light:
filesystems like btrfs and XFS v5 which set SB_IVERSION
unconditionally (i.e. it's not a mount option!) end up having that
functionality turned off on remount because the VFS conflates
MS_IVERSION with SB_IVERSION and so unconditionally clears
SB_IVERSION because MS_IVERSION is not set on remount by userspace.
Which userspace will never set be because the filesystems don't put
"iversion" in their mount option strings because -its not a mount
option- for those filesystems.

See the problem?  MS_IVERSION should be passed to the filesystem to
deal with as a mount option, not treated as a flag to directly
change SB_IVERSION in the superblock.

We really need to stop with the "global mount options for everyone
at the VFS" and instead pass everything down to the filesystems to
parse appropriately. Yes, provide generic helper functions to deal
with the common flags that everything supports, but the filesystems
should be masking off mount options they doesn't support changing
before changing their superblock feature support mask....

I think the MS_* flags are best saved for mount options that are applicable to
many/most filesystems and are mostly/entirely implementable at the VFS level.

That's the theory, but so far it's caused nothing but pain.

In reality, I think ithe only sane way forward if to stop mount
option parsing in userspace (i.e. no new MS_* flags) for any new
functionality as it only leads to future pain. i.e. all new mount
options should be parsed entirely in the kernel by the filesystem
parsing code....

I don't think "inlinecrypt" qualifies, since while it will be shared by the
block device-based filesystems that support fscrypt, that is only 2 filesystems
currently; and while some of the code needed to implement it is shared in
fs/crypto/, there are still substantial filesystem-specific hooks needed.

Right. I wasn't suggesting this patchset should use an MS_ flag -
it was pointing out the problem with the VFS code using SB_ flags to
indicate enabled filesystem functionality unconditionally as a mount
option that can be changed by userspace.

Hence this patchset intentionally does *not* allocate an MS_INLINECRYPT flag.

I believe that already addresses half of your concern, as it means
SB_INLINECRYPT can only be set/cleared by the filesystem itself, not by the VFS.
(But the commit message could use an explanation of this.)

The other half would be addressed by the following change, right?

Yes, it does. Thanks, Eric!

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com