lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 1 Dec 2020 11:49:07 +0100 From: Christoph Hellwig <hch@....de> To: Christian Brauner <christian.brauner@...ntu.com> Cc: Alexander Viro <viro@...iv.linux.org.uk>, Christoph Hellwig <hch@...radead.org>, linux-fsdevel@...r.kernel.org, John Johansen <john.johansen@...onical.com>, James Morris <jmorris@...ei.org>, Mimi Zohar <zohar@...ux.ibm.com>, Dmitry Kasatkin <dmitry.kasatkin@...il.com>, Stephen Smalley <stephen.smalley.work@...il.com>, Casey Schaufler <casey@...aufler-ca.com>, Arnd Bergmann <arnd@...db.de>, Andreas Dilger <adilger.kernel@...ger.ca>, OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>, Geoffrey Thomas <geofft@...reload.com>, Mrunal Patel <mpatel@...hat.com>, Josh Triplett <josh@...htriplett.org>, Andy Lutomirski <luto@...nel.org>, Theodore Tso <tytso@....edu>, Alban Crequy <alban@...volk.io>, Tycho Andersen <tycho@...ho.ws>, David Howells <dhowells@...hat.com>, James Bottomley <James.Bottomley@...senpartnership.com>, Seth Forshee <seth.forshee@...onical.com>, Stéphane Graber <stgraber@...ntu.com>, Aleksa Sarai <cyphar@...har.com>, Lennart Poettering <lennart@...ttering.net>, "Eric W. Biederman" <ebiederm@...ssion.com>, smbarber@...omium.org, Phil Estes <estesp@...il.com>, Serge Hallyn <serge@...lyn.com>, Kees Cook <keescook@...omium.org>, Todd Kjos <tkjos@...gle.com>, Paul Moore <paul@...l-moore.com>, Jonathan Corbet <corbet@....net>, containers@...ts.linux-foundation.org, fstests@...r.kernel.org, linux-security-module@...r.kernel.org, linux-api@...r.kernel.org, linux-ext4@...r.kernel.org, linux-integrity@...r.kernel.org, selinux@...r.kernel.org, Christoph Hellwig <hch@....de> Subject: Re: [PATCH v3 04/38] fs: add mount_setattr() Lots of crazy long lines in the patch. Remember that you should only go past 80 lines if it clearly improves readability, and I don't think it does anywhere in here. > index a7cd0f64faa4..a5a6c470dc07 100644 > --- a/fs/internal.h > +++ b/fs/internal.h > @@ -82,6 +82,14 @@ int may_linkat(struct path *link); > /* > * namespace.c > */ > +struct mount_kattr { > + unsigned int attr_set; > + unsigned int attr_clr; > + unsigned int propagation; > + unsigned int lookup_flags; > + bool recurse; > +}; Even with the whole series applied this structure is only used in namespace.c, so it might be worth moving there. > +static inline int mnt_hold_writers(struct mount *mnt) > { > - int ret = 0; > - > mnt->mnt.mnt_flags |= MNT_WRITE_HOLD; > /* > * After storing MNT_WRITE_HOLD, we'll read the counters. This store > @@ -497,15 +495,29 @@ static int mnt_make_readonly(struct mount *mnt) > * we're counting up here. > */ > if (mnt_get_writers(mnt) > 0) > - ret = -EBUSY; > - else > - mnt->mnt.mnt_flags |= MNT_READONLY; > + return -EBUSY; > + > + return 0; > +} > + > +static inline void mnt_unhold_writers(struct mount *mnt) > +{ > /* > * MNT_READONLY must become visible before ~MNT_WRITE_HOLD, so writers > * that become unheld will see MNT_READONLY. > */ > smp_wmb(); > mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD; > +} > + > +static int mnt_make_readonly(struct mount *mnt) > +{ > + int ret; > + > + ret = mnt_hold_writers(mnt); > + if (!ret) > + mnt->mnt.mnt_flags |= MNT_READONLY; > + mnt_unhold_writers(mnt); > return ret; > } > > @@ -3438,6 +3450,33 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name, > return ret; > } This refactoring seems worth a little prep patch. > > +static int build_attr_flags(unsigned int attr_flags, unsigned int *flags) > +{ > + unsigned int aflags = 0; > + > + if (attr_flags & ~(MOUNT_ATTR_RDONLY | > + MOUNT_ATTR_NOSUID | > + MOUNT_ATTR_NODEV | > + MOUNT_ATTR_NOEXEC | > + MOUNT_ATTR__ATIME | > + MOUNT_ATTR_NODIRATIME)) > + return -EINVAL; > + > + if (attr_flags & MOUNT_ATTR_RDONLY) > + aflags |= MNT_READONLY; > + if (attr_flags & MOUNT_ATTR_NOSUID) > + aflags |= MNT_NOSUID; > + if (attr_flags & MOUNT_ATTR_NODEV) > + aflags |= MNT_NODEV; > + if (attr_flags & MOUNT_ATTR_NOEXEC) > + aflags |= MNT_NOEXEC; > + if (attr_flags & MOUNT_ATTR_NODIRATIME) > + aflags |= MNT_NODIRATIME; > + > + *flags = aflags; > + return 0; > +} Same for adding this helper. > + *kattr = (struct mount_kattr){ Missing whitespace before the {. > + switch (attr->propagation) { > + case MAKE_PROPAGATION_UNCHANGED: > + kattr->propagation = 0; > + break; > + case MAKE_PROPAGATION_UNBINDABLE: > + kattr->propagation = MS_UNBINDABLE; > + break; > + case MAKE_PROPAGATION_PRIVATE: > + kattr->propagation = MS_PRIVATE; > + break; > + case MAKE_PROPAGATION_DEPENDENT: > + kattr->propagation = MS_SLAVE; > + break; > + case MAKE_PROPAGATION_SHARED: > + kattr->propagation = MS_SHARED; > + break; > + default: Any reason to not just reuse the MS_* flags in the new API? Yes, your new names are more descriptive, but having different names for the same thing is also rather confusing. > + if (upper_32_bits(attr->attr_set)) > + return -EINVAL; > + if (build_attr_flags(lower_32_bits(attr->attr_set), &kattr->attr_set)) > + return -EINVAL; > + > + if (upper_32_bits(attr->attr_clr)) > + return -EINVAL; > + if (build_attr_flags(lower_32_bits(attr->attr_clr), &kattr->attr_clr)) > + return -EINVAL; What is so magic about the upper and lower 32 bits? > + return -EINVAL; > + else if ((attr->attr_clr & MOUNT_ATTR__ATIME) && > + ((attr->attr_clr & MOUNT_ATTR__ATIME) != MOUNT_ATTR__ATIME)) > + return -EINVAL; No need for the else here. That being said I'd reword the thing to be a little more obvious: if (attr->attr_clr & MOUNT_ATTR__ATIME) { if ((attr->attr_clr & MOUNT_ATTR__ATIME) != MOUNT_ATTR__ATIME) return -EINVAL; ... code doing the update of the atime flags here } else { if (attr->attr_set & MOUNT_ATTR__ATIME) return -EINVAL; } > +/* Change propagation through mount_setattr(). */ > +enum propagation_type { > + MAKE_PROPAGATION_UNCHANGED = 0, /* Don't change mount propagation (default). */ > + MAKE_PROPAGATION_UNBINDABLE = 1, /* Make unbindable. */ > + MAKE_PROPAGATION_PRIVATE = 2, /* Do not receive or send mount events. */ > + MAKE_PROPAGATION_DEPENDENT = 3, /* Only receive mount events. */ > + MAKE_PROPAGATION_SHARED = 4, /* Send and receive mount events. */ > +}; FYI, in uapis using defines instead of enums is usually the better choice, as that allows userspace to probe for later added defines. But if we use MS_* here that would be void anyway. > +/* List of all mount_attr versions. */ > +#define MOUNT_ATTR_SIZE_VER0 24 /* sizeof first published struct */ > +#define MOUNT_ATTR_SIZE_LATEST MOUNT_ATTR_SIZE_VER0 The _LATEST things is pretty dangerous as there basically is no safe and correct way for userspace to use it.
Powered by blists - more mailing lists