Re: [f2fs-dev] [PATCH 1/4] fs: introduce SB_INLINECRYPT

[Dave Chinner <david@fromorbit.com>]

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:
> > On Wed, Jun 17, 2020 at 08:19:35PM -0700, Eric Biggers wrote:
> > > 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