Re: [PATCH 03/14] VFS: Implement a filesystem superblock creation/configuration context [ver #6]

Adding linux-api@vger (I think you should add this Cc for patches
which add/change userspace APIs).

On Fri, Oct 27, 2017 at 4:35 PM, David Howells [off-list ref] wrote:

Miklos Szeredi [off-list ref] wrote:

quoted

Also, how about moving calls to vfs_parse_fs_option() into filesystem
code?   Even those options are not generic, some filesystem wants
this, some that.  It's just a historical accident that those are set
with MS_FOO and not "foo".  Filesystems that don't have any option
parsing could have a generic version that deals with "ro/rw", the
others can handle these options along with the rest.

Ummm...  I don't see how that would work.  vfs_parse_mount_option() (or
vfs_parse_fs_option() as it will become) is the way into the filesystem from
write(mfd, "o foo") and also applies the security policy before the filesystem
gets its hands on the option.

Did you mean vfs_parse_sb_flag_option()?  The point of that function is so
that the name->flag mapping tables don't have to be replicated in every
filesystem.

Yes I did mean vfs_parse_sb_flag_option().

Yes, I understand its purpose, but it would be cleaner if all the
option parsing was done in fc->ops->parse_option().

It might be worth introducing the vfs_parse_sb_flag_option(), to be
called from ->parse_option().

Also, filesystems can supply a ->validate() method that rejects any SB_* flags
they don't want to support, but for legacy purposes we probably can't do that.

quoted

Reset only makes sense in the context of reconfig (fka. remount).

Okay, that makes more sense.

quoted

But lets leave to later if it's not something trivial.

I don't think it is trivial - and it's something that would have to be dealt
with on an fs-by-fs basis and very well documented.

Btw, how would it affect the LSM?

LSM would have to reject a "reset" if not enough privileges to
*create* a new fs instance, since it essentially requires creating a
new config, which is what is done when creating an fs instance.

Also, how do you propose to use it?  I presume you're not thinking of someone
talking to the socket with a telnet-like interface.

No.  It would be an command line option for the relevant userspace utility:

  fs-reconfig /mnt/foo --reset "ro"

as opposed to

  fs-reconfig /mnt/foo "ro"

The former would change the options to default + "ro".

The latter would change "rw"->"ro" and leave all other options alone.

quoted

quoted

quoted

2/a) Shared sb:
2/b) Shared sb for legacy mount(2)

In the new-mount-of-live-sb case, I would validate the context script and
ignore any options that try to change things that can't be changed because
the fs is live.

Your sentence seems to imply that we do change those that can be
changed.   That's not what legacy does, it ignores *all* options
(except rw/ro for which it errors out on mismatch).  I don't think
that's a nice behavior, but we definitely need to keep it for legacy.

For non-legacy, do we want to extend the "error out on mismatch"
behavior to all options, rather than ignoring them?

Actually, we might want to ignore all the options.  That might itself be an
option, kind of like O_CREAT/O_EXCL.  I think someone suggested this before.

Okay, that makes sense.

quoted

quoted

There's the question of how far you allow a happens-to-share mount to
effect a reconfigure.  Seems a reasonable distinction to say that in your
case 2 you just ignore conflicts but possibly warn or reject in case 3.

Not sure I understand why we'd want to ignore conflicts in case 2 and
not in 3.   Can we not have consistency (error out on all conflicts)?

I was thinking that if you mount a source that's already mounted, it would do
a reconfigure instead, but I this is addressed above as "2) shared sb".

quoted

quoted

Except that ext4, f2fs, 9p, ... do take at least some of them.  I'm not
sure whether they ever see them, but without auditing userspace, there's
no way to know.

So moving possibly dead code to the level of VFS fixes things how?

It's not dead code.  You can call the mount() syscall directly, and something
like busybox might well do so.  Normally these are weeded out by userspace.

It's possible, even, in the ext4 case that you might store these options on
disk in the options string in the superblock.

quoted

Let filesystems deal with that crap and make sure they deal with it
only for legacy mount and not for the new, supposedly clean one.

Sorry, how does the new, clean one do it without handling these options?
There is no MS_* mask passed in, except to fsmount().

The new one certainly should.

quoted

Making it generic also possibly breaks uABI by allowing an option that
was rejected previously for some other fs.

That's not a particularly serious break, I wouldn't've thought.  Further, the
set of options that a filesystem will take evolves over time, and what was
rejected yesterday might be accepted today.

All the UAPI SB_* options can be passed in to mount(2) from userspace, and
filesystems all just ignore them if they don't want to support them as far as
I know.  If this is the case, I don't see a problem with letting generic code
parse these common options.

Ignoring unknown flags/options is generally a bad idea.

Thanks,
Miklos

Miklos Szeredi [off-list ref] wrote:

Yes I did mean vfs_parse_sb_flag_option().

Yes, I understand its purpose, but it would be cleaner if all the
option parsing was done in fc->ops->parse_option().

It might be worth introducing the vfs_parse_sb_flag_option(), to be
called from ->parse_option().

I was trying to relieve the filesystem of the requirement to have to deal with
common stuff and also the need to talk directly to the LSM.

quoted

Btw, how would it affect the LSM?

LSM would have to reject a "reset" if not enough privileges to
*create* a new fs instance, since it essentially requires creating a
new config, which is what is done when creating an fs instance.

That's not what I'm asking.  Would the reset change LSM state?  Reset security
labels and options?

quoted

Sorry, how does the new, clean one do it without handling these options?
There is no MS_* mask passed in, except to fsmount().

The new one certainly should.

Should what?

Ignoring unknown flags/options is generally a bad idea.

They're not unknown - just not of interest to the filesystem.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 27, 2017 at 6:03 PM, David Howells [off-list ref] wrote:

Miklos Szeredi [off-list ref] wrote:

quoted

Yes I did mean vfs_parse_sb_flag_option().

Yes, I understand its purpose, but it would be cleaner if all the
option parsing was done in fc->ops->parse_option().

It might be worth introducing the vfs_parse_sb_flag_option(), to be
called from ->parse_option().

I was trying to relieve the filesystem of the requirement to have to deal with
common stuff and also the need to talk directly to the LSM.

No need to talk directly to the LSM:
security_fs_context_parse_option() will do that in VFS code.

How common is common stuff?

dirsync/sync/rw: not handled by all filesystems, those that don't
handle it should reject the option on the new interface

lazytime: handled by generic code, AFAICS, but makes no sense on
read-only fs so those should probably reject it

mand: handled by generic code, but does not make sense for some
filesystems (e.g. those that don't have all the unixy permission
bits).

posixacl: there's no such mount option now.  The options is "acl" and
does not get translated to MS_POSIXACL in mount(8).   Makes zero sense
to add a previously nonexistent option to the new interface.

silent: makes no sense on the new interface, since we should no longer
be putting error messages into the kernel log.

So that leaves async/ro/nolazytime/nomand options to be handled by all
filesystems.

Not sure how to best handle these differences, but the current code
definitely seems lacking, and I cannot imagine a better way than to
pass all options to filesystem's ->parse_option() and add helper(s) to
handle the generic options.

quoted

quoted

Btw, how would it affect the LSM?

LSM would have to reject a "reset" if not enough privileges to
*create* a new fs instance, since it essentially requires creating a
new config, which is what is done when creating an fs instance.

That's not what I'm asking.  Would the reset change LSM state?  Reset security
labels and options?

No.   And it wouldn't reset any other option that is immutable (e.g.
server IP address).

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html