Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 24 Sep 2018 11:47:56 +0200
From:   Christian Brauner <christian@brauner.io>
To:     David Howells <dhowells@redhat.com>
Cc:     Miklos Szeredi <mszeredi@redhat.com>,
        Al Viro <viro@ZenIV.linux.org.uk>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags
Message-ID: <20180924094755.lkwdbrrayfrp45uu@brauner.io>
References: <20180922161406.6beo2ob6ki2otrjx@brauner.io>
 <20180922132106.wfa6xaxwbu276qka@brauner.io>
 <20180921155455.flkf5vdrm3vgn6do@brauner.io>
 <20180920151214.15484-6-mszeredi@redhat.com>
 <20180920151214.15484-1-mszeredi@redhat.com>
 <17157.1537542475@warthog.procyon.org.uk>
 <20311.1537548756@warthog.procyon.org.uk>
 <8221.1537631312@warthog.procyon.org.uk>
 <3868.1537742717@warthog.procyon.org.uk>
 <30364.1537771838@warthog.procyon.org.uk>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="agm3bumfxevf7fv5"
Content-Disposition: inline
In-Reply-To: <30364.1537771838@warthog.procyon.org.uk>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--agm3bumfxevf7fv5
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Sep 24, 2018 at 07:50:38AM +0100, David Howells wrote:
> Christian Brauner <christian@brauner.io> wrote:
>=20
> > Ok, understood. What about passing the different attrs as a struct?
> >=20
> > struct mount_attr {
> >         unsigned int attr_cmd,
> >         unsigned int attr_values,
> >         unsigned int attr_mask,
> >=20
> > };
> >=20
> > mount_setattr(int dfd, const char *path, unsigned int atflags,
> >               struct mount_attr *attr);
> >=20
> > 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.
>=20
> Yeah, we could do that - it's not like I expect mount_setattr() to have t=
o 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 =3D 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)  =20
#define MOUNT_ATTR_NOEXEC       (1<<2) =20
#define MOUNT_ATTR_NOSUID       (1<<3) =20
#define MOUNT_ATTR_NOREMOTELOCK (1<<4)
#define MOUNT_ATTR_RDONLY       (1<<5) =20
#define MOUNT_ATTR_POSIXACL     (1<<6)
#define MOUNT_ATTR_SILENT       (1<<7) =20

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 =3D 1,
        MOUNT_ATTR_SECURITY,
        MOUNT_ATTR_SECURITY_1 =3D 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

--agm3bumfxevf7fv5
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEE7btrcuORLb1XUhEwjrBW1T7ssS0FAluosssACgkQjrBW1T7s
sS3gYhAAm3KARS3C3Q4qtCsgVqikraFdr9uZTqJ38TiDZLkHr1bJDUsFR2HLdKWq
Mvv6u0ID8mg6jrYxU9Vgo6nHqd5HnX/YKIviOYYI4R5YQTCig48HmgnOgk+DJI/g
W/U3PaPowkbcPMdryXfSEV1AgRZCvouwr48H/C34lYe9ONPrIJCbI3J6AsxMiTI1
92vLM3vIkBfBtvh00vzgAeOH+gl3ik+NltEGMAgASoAkS9w784pnKcHGA+anci3s
BF85KL63OT3brUHwB1/e7CvmTsQRf0cn/KC75yK/KrGXOQYvHAEPV6ySyBDAVs54
sRzDpbIaX809TsjBEE1uknPNhlLZVvM+JqZbnPaNjt2S5mxkLF0tM/Kkfbae0YmC
joquTl++IuNpZIFKz2e2SyW+OKSHApDJ/ud4QX8PwM5mlugpuZIjEOwkJeSOP+pT
lpB71sNjYhjrxYD4upYA1cGaWWSvbl80YIWYQnNN7By7dI9X7iXMb8SIijeOm9lD
rYNCEkjn4RkCgRtikOMODAMudHcuMT8dYV3kv3/LO1iRPs3gm//pMXpok6Qbx+N9
ZoXB72P0o/0n3fegPROBVhZteH1U2wsJ0GEaHSUbLnavGt4IGPksce8EsC/3uOxb
9+fHQocGos3i11MsybQKAspwOzJfjk0teug9sImUu+t9BjhhFz4=
=yzeQ
-----END PGP SIGNATURE-----

--agm3bumfxevf7fv5--