Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Christian Brauner <christian@brauner.io>]

[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]

On Mon, Sep 24, 2018 at 07:50:38AM +0100, David Howells wrote:
> Christian Brauner <christian@brauner.io> wrote:
> 
> > Ok, understood. What about passing the different attrs as a struct?
> > 
> > struct mount_attr {
> >         unsigned int attr_cmd,
> >         unsigned int attr_values,
> >         unsigned int attr_mask,
> > 
> > };
> > 
> > mount_setattr(int dfd, const char *path, unsigned int atflags,
> >               struct mount_attr *attr);
> > 
> > I find that to be a little cleaner in all honesty.
> > One could also add a version argument similar to what we currently do
> > for vfs fcaps so that kernel and userspace can easily navigate
> > compabitility when a new member gets added or removed in later releases.
> 
> Yeah, we could do that - it's not like I expect mount_setattr() to have to be
> particularly performant in the user interface.  I would put the attr_cmd in
> the argument list, probably, so that you can use that to vary the struct in
> future (say we run out of attribute bits).

Yes, that makes sense and mimicks standard ioctl() behavior. So

struct mount_attr {
        unsigned int attr_values,
        unsigned int attr_mask,
}

mount_setattr(int dfd, const char *path, unsigned int atflags,
              unsigned int attr_cmd, struct mount_attr *attr);

I have thought a little more about splitting up the mount flags into
sensible sets. I think the following four sets make sense:

enum {
        MOUNT_ATTR_PROPAGATION = 1,
        MOUNT_ATTR_SECURITY,
        MOUNT_ATTR_SYNC,
        MOUNT_ATTR_TIME,
};

MOUNT_ATTR_PROPAGATION:
#define MOUNT_ATTR_PRIVATE    (1<<0)
#define MOUNT_ATTR_SHARED     (1<<1)
#define MOUNT_ATTR_SLAVE      (1<<2)
#define MOUNT_ATTR_UNBINDABLE (1<<3)

MOUNT_ATTR_SECURITY:
#define MOUNT_ATTR_MANDLOCK     (1<<0)
#define MOUNT_ATTR_NODEV        (1<<1)   
#define MOUNT_ATTR_NOEXEC       (1<<2)  
#define MOUNT_ATTR_NOSUID       (1<<3)  
#define MOUNT_ATTR_NOREMOTELOCK (1<<4)
#define MOUNT_ATTR_RDONLY       (1<<5)  
#define MOUNT_ATTR_POSIXACL     (1<<6)
#define MOUNT_ATTR_SILENT       (1<<7)  

MOUNT_ATTR_SYNC
#define MOUNT_ATTR_DIRSYNC     (1<<0)
#define MOUNT_ATTR_SYNCHRONOUS (1<<1)

MOUNT_ATTR_TIME:
#define MOUNT_ATTR_LAZYTIME    (1<<0)
#define MOUNT_ATTR_NOATIME     (1<<1)
#define MOUNT_ATTR_NODIRATIME  (1<<2)
#define MOUNT_ATTR_RELATIME    (1<<3)
#define MOUNT_ATTR_STRICTATIME (1<<4)

If we ever run out of flags in a specific set I suggest to introduce a
new enum member of the same name with a version number appended and an
alias with a (obvs lower) version number for the old set. A concrete
example would be:

enum {
        MOUNT_ATTR_PROPAGATION = 1,
        MOUNT_ATTR_SECURITY,
        MOUNT_ATTR_SECURITY_1 = MOUNT_ATTR_SECURITY,
        MOUNT_ATTR_SYNC,
        MOUNT_ATTR_TIME,
        MOUNT_ATTR_SECURITY_2,
};

These flags will likely become AT_* flags or be tied to a syscall
afaict.

#define MS_REMOUNT      32
#define MS_BIND	        4096
#define MS_MOVE	        8192
#define MS_REC	        16384

Internal sb flags will not be part of the new mount attr sets. (They
should - imho - not be exposed to userspace at all.):

#define MS_KERNMOUNT    (1<<22)
#define MS_SUBMOUNT     (1<<26)
#define MS_NOREMOTELOCK (1<<27)
#define MS_NOSEC        (1<<28)
#define MS_BORN	        (1<<29)
#define MS_ACTIVE       (1<<30)
#define MS_NOUSER       (1<<31)

What remains is an odd duck that probably could be thrown into security
but also *shrug*

#define MS_I_VERSION    (1<<23)

Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]