Skip to contentSkip to conversation

Mount writer count and read-only bind mounts

PATCH In review Dave Hansen 7 participants 83 messages Jun 22, 2007 – Jul 11, 2007

Subsystems (18)
lore
Share & tools
lore
#0 lore raw

cover Mount writer count and read-only bind mounts

The largest change from last time is in patch 25/26. It has been reworked to use per-cpu lock instead of a per-node one. See the patch for more details.

I would also appreciate any close review of patch 08/27. It touches a lot of code, and I'm a bit worried that a buglet or two could have snuck in there.

Note that there are a few stragglers left using IS_RDONLY() in reiser4 and in some other low-level filesystem code. The reiser4 asserts should be just fine left the way they are, plus reiser4 needs quite a bit of work before merging anyway. The ntfs usage appears to be internal, and not related to user activity.

This patch survives running ltp as well as a home-grown set of filesystem operations here:

	http://sr71.net/~dave/linux/robind-test.sh

---

Why do we need r/o bind mounts?

This feature allows a read-only view into a read-write filesystem. In the process of doing that, it also provides infrastructure for keeping track of the number of writers to any given mount.

This has a number of uses. It allows chroots to have parts of filesystems writable. It will be useful for containers in the future because users may have root inside a container, but should not be allowed to write to somefilesystems. This also replaces patches that vserver has had out of the tree for several years.

It allows security enhancement by making sure that parts of your filesystem read-only (such as when you don't trust your FTP server), when you don't want to have entire new filesystems mounted, or when you want atime selectively updated. I've been using the following script to test that the feature is working as desired. It takes a directory and makes a regular bind and a r/o bind mount of it. It then performs some normal filesystem operations on the three directories, including ones that are expected to fail, like creating a file on the r/o mount.

#1 lore raw

PATCH 1/26 document nlink function

These should have been documented from the beginning. Fix it.

---

 lxc-dave/include/linux/fs.h |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
include/linux/fs.h renamed from include/linux/fs.h~document-nlink-funcs +27 -0
diff -puN include/linux/fs.h~document-nlink-funcs include/linux/fs.h
@@ -1198,6 +1198,14 @@ static inline void mark_inode_dirty_sync
 	__mark_inode_dirty(inode, I_DIRTY_SYNC);
 }
 
+/**
+ * inc_nlink - directly increment an inode's link count
+ * @inode: inode
+ *
+ * This is a low-level filesystem helper to replace any
+ * direct filesystem manipulation of i_nlink.  Currently,
+ * it is only here for parity with dec_nlink().
+ */
 static inline void inc_nlink(struct inode *inode)
 {
 	inode->i_nlink++;
@@ -1209,11 +1217,30 @@ static inline void inode_inc_link_count(
 	mark_inode_dirty(inode);
 }
 
+/**
+ * drop_nlink - directly drop an inode's link count
+ * @inode: inode
+ *
+ * This is a low-level filesystem helper to replace any
+ * direct filesystem manipulation of i_nlink.  In cases
+ * where we are attempting to track writes to the
+ * filesystem, a decrement to zero means an imminent
+ * write when the file is truncated and actually unlinked
+ * on the filesystem.
+ */
 static inline void drop_nlink(struct inode *inode)
 {
 	inode->i_nlink--;
 }
 
+/**
+ * clear_nlink - directly zero an inode's link count
+ * @inode: inode
+ *
+ * This is a low-level filesystem helper to replace any
+ * direct filesystem manipulation of i_nlink.  See
+ * drop_nlink() for why we care about i_nlink hitting zero.
+ */
 static inline void clear_nlink(struct inode *inode)
 {
 	inode->i_nlink = 0;

_

#2 lore raw

Re: [PATCH 01/26] document nlink function

Quoted 2 lines · Dave Hansen

These should have been documented from the beginning. Fix it.

Ok. This is a trivial doc fix and should go into the 2.6.22 queue IMHO.

#3 lore raw

PATCH 2/26 ext3: remove extra IS_RDONLY() check

In reply to Dave Hansen

ext3_change_inode_journal_flag() is only called from one
location: ext3_ioctl(EXT3_IOC_SETFLAGS). That ioctl case already has a IS_RDONLY() call in it so this one is superfluous.

---

 lxc-dave/fs/ext3/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext3/inode.c renamed from fs/ext3/inode.c~ext3-extra-ro-check +1 -1
diff -puN fs/ext3/inode.c~ext3-extra-ro-check fs/ext3/inode.c
@@ -3182,7 +3182,7 @@ int ext3_change_inode_journal_flag(struc
 	 */
 
 	journal = EXT3_JOURNAL(inode);
-	if (is_journal_aborted(journal) || IS_RDONLY(inode))
+	if (is_journal_aborted(journal))
 		return -EROFS;
 
 	journal_lock_updates(journal);

_

#4 lore raw

Re: [PATCH 02/26] ext3: remove extra IS_RDONLY() check

Quoted 5 lines · Dave Hansen

ext3_change_inode_journal_flag() is only called from one
location: ext3_ioctl(EXT3_IOC_SETFLAGS). That ioctl case already has a IS_RDONLY() call in it so this one is superfluous.

Ok, and stands nicely on it's own.

#5 lore raw

PATCH 3/26 ext4: remove extra IS_RDONLY() check

In reply to Dave Hansen

ext4_change_inode_journal_flag() is only called from one
location: ext4_ioctl(EXT3_IOC_SETFLAGS). That ioctl case already has a IS_RDONLY() call in it so this one is superfluous.

---

 lxc-dave/fs/ext4/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext4/inode.c renamed from fs/ext4/inode.c~ext4-extra-ro-check +1 -1
diff -puN fs/ext4/inode.c~ext4-extra-ro-check fs/ext4/inode.c
@@ -3196,7 +3196,7 @@ int ext4_change_inode_journal_flag(struc
 	 */
 
 	journal = EXT4_JOURNAL(inode);
-	if (is_journal_aborted(journal) || IS_RDONLY(inode))
+	if (is_journal_aborted(journal))
 		return -EROFS;
 
 	jbd2_journal_lock_updates(journal);

_

#6 lore raw

Re: [PATCH 03/26] ext4: remove extra IS_RDONLY() check

Quoted 5 lines · Dave Hansen

ext4_change_inode_journal_flag() is only called from one
location: ext4_ioctl(EXT3_IOC_SETFLAGS). That ioctl case already has a IS_RDONLY() call in it so this one is superfluous.

Ditto.

#7 lore raw

PATCH 4/26 filesystem helpers for custom 'struct file's

In reply to Dave Hansen

Christoph H. says this stands on its own and can go in before the rest of the r/o bind mount set.

---

Some filesystems forego the vfs and may_open() and create their own 'struct file's.

This patch creates a couple of helper functions which can be used by these filesystems, and will provide a unified place which the r/o bind mount code may patch.

Also, rename two existing, static-scope init_file() to less generic names.

---

 lxc-dave/fs/configfs/dir.c    |    5 +++--
 lxc-dave/fs/file_table.c      |   34 ++++++++++++++++++++++++++++++++++
 lxc-dave/fs/hugetlbfs/inode.c |   22 +++++++++-------------
 lxc-dave/fs/sysfs/dir.c       |    4 ++--
 lxc-dave/include/linux/file.h |    9 +++++++++
 lxc-dave/ipc/shm.c            |   13 +++++--------
 lxc-dave/mm/shmem.c           |    7 ++-----
 lxc-dave/mm/tiny-shmem.c      |   19 +++++++------------
 lxc-dave/net/socket.c         |   19 ++++++++++---------
 9 files changed, 81 insertions(+), 51 deletions(-)
fs/configfs/dir.c renamed from fs/configfs/dir.c~01-24-filesystem-helpers-for-custom-struct-file-s +3 -2
diff -puN fs/configfs/dir.c~01-24-filesystem-helpers-for-custom-struct-file-s fs/configfs/dir.c
@@ -142,7 +142,7 @@ static int init_dir(struct inode * inode
 	return 0;
 }
 
-static int init_file(struct inode * inode)
+static int configfs_init_file(struct inode * inode)
 {
 	inode->i_size = PAGE_SIZE;
 	inode->i_fop = &configfs_file_operations;
@@ -283,7 +283,8 @@ static int configfs_attach_attr(struct c
 
 	dentry->d_fsdata = configfs_get(sd);
 	sd->s_dentry = dentry;
-	error = configfs_create(dentry, (attr->ca_mode & S_IALLUGO) | S_IFREG, init_file);
+	error = configfs_create(dentry, (attr->ca_mode & S_IALLUGO) | S_IFREG,
+				configfs_init_file);
 	if (error) {
 		configfs_put(sd);
 		return error;
fs/file_table.c renamed from fs/file_table.c~01-24-filesystem-helpers-for-custom-struct-file-s +34 -0
diff -puN fs/file_table.c~01-24-filesystem-helpers-for-custom-struct-file-s fs/file_table.c
@@ -139,6 +139,40 @@ fail:
 
 EXPORT_SYMBOL(get_empty_filp);
 
+struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
+		mode_t mode, const struct file_operations *fop)
+{
+	struct file *file;
+	struct path;
+
+	file = get_empty_filp();
+	if (!file)
+		return NULL;
+
+	init_file(file, mnt, dentry, mode, fop);
+	return file;
+}
+EXPORT_SYMBOL(alloc_file);
+
+/*
+ * Note: This is a crappy interface.  It is here to make
+ * merging with the existing users of get_empty_filp()
+ * who have complex failure logic easier.  All users
+ * of this should be moving to alloc_file().
+ */
+int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
+	   mode_t mode, const struct file_operations *fop)
+{
+	int error = 0;
+	file->f_path.dentry = dentry;
+	file->f_path.mnt = mntget(mnt);
+	file->f_mapping = dentry->d_inode->i_mapping;
+	file->f_mode = mode;
+	file->f_op = fop;
+	return error;
+}
+EXPORT_SYMBOL(init_file);
+
 void fastcall fput(struct file *file)
 {
 	if (atomic_dec_and_test(&file->f_count))
fs/hugetlbfs/inode.c renamed from fs/hugetlbfs/inode.c~01-24-filesystem-helpers-for-custom-struct-file-s +9 -13
diff -puN fs/hugetlbfs/inode.c~01-24-filesystem-helpers-for-custom-struct-file-s fs/hugetlbfs/inode.c
@@ -759,16 +759,11 @@ struct file *hugetlb_zero_setup(size_t s
 	if (!dentry)
 		goto out_shm_unlock;
 
-	error = -ENFILE;
-	file = get_empty_filp();
-	if (!file)
-		goto out_dentry;
-
 	error = -ENOSPC;
 	inode = hugetlbfs_get_inode(root->d_sb, current->fsuid,
 				current->fsgid, S_IFREG | S_IRWXUGO, 0);
 	if (!inode)
-		goto out_file;
+		goto out_dentry;
 
 	error = -ENOMEM;
 	if (hugetlb_reserve_pages(inode, 0, size >> HPAGE_SHIFT))
@@ -777,17 +772,18 @@ struct file *hugetlb_zero_setup(size_t s
 	d_instantiate(dentry, inode);
 	inode->i_size = size;
 	inode->i_nlink = 0;
-	file->f_path.mnt = mntget(hugetlbfs_vfsmount);
-	file->f_path.dentry = dentry;
-	file->f_mapping = inode->i_mapping;
-	file->f_op = &hugetlbfs_file_operations;
-	file->f_mode = FMODE_WRITE | FMODE_READ;
+
+	error = -ENFILE;
+	file = alloc_file(hugetlbfs_vfsmount, dentry,
+			FMODE_WRITE | FMODE_READ,
+			&hugetlbfs_file_operations);
+	if (!file)
+		goto out_inode;
+
 	return file;
 
 out_inode:
 	iput(inode);
-out_file:
-	put_filp(file);
 out_dentry:
 	dput(dentry);
 out_shm_unlock:
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~01-24-filesystem-helpers-for-custom-struct-file-s +2 -2
diff -puN fs/sysfs/dir.c~01-24-filesystem-helpers-for-custom-struct-file-s fs/sysfs/dir.c
@@ -134,7 +134,7 @@ static int init_dir(struct inode * inode
 	return 0;
 }
 
-static int init_file(struct inode * inode)
+static int sysfs_init_file(struct inode * inode)
 {
 	inode->i_size = PAGE_SIZE;
 	inode->i_fop = &sysfs_file_operations;
@@ -234,7 +234,7 @@ static int sysfs_attach_attr(struct sysf
                 attr = &bin_attr->attr;
         } else {
                 attr = sd->s_element;
-                init = init_file;
+		init = sysfs_init_file;
         }
 
 	dentry->d_fsdata = sysfs_get(sd);
include/linux/file.h renamed from include/linux/file.h~01-24-filesystem-helpers-for-custom-struct-file-s +9 -0
diff -puN include/linux/file.h~01-24-filesystem-helpers-for-custom-struct-file-s include/linux/file.h
@@ -62,6 +62,15 @@ extern struct kmem_cache *filp_cachep;
 extern void FASTCALL(__fput(struct file *));
 extern void FASTCALL(fput(struct file *));
 
+struct file_operations;
+struct vfsmount;
+struct dentry;
+extern int init_file(struct file *, struct vfsmount *mnt,
+		struct dentry *dentry, mode_t mode,
+		const struct file_operations *fop);
+extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
+		mode_t mode, const struct file_operations *fop);
+
 static inline void fput_light(struct file *file, int fput_needed)
 {
 	if (unlikely(fput_needed))
ipc/shm.c renamed from ipc/shm.c~01-24-filesystem-helpers-for-custom-struct-file-s +5 -8
diff -puN ipc/shm.c~01-24-filesystem-helpers-for-custom-struct-file-s ipc/shm.c
@@ -899,7 +899,7 @@ long do_shmat(int shmid, char __user *sh
 		goto out_unlock;
 
 	path.dentry = dget(shp->shm_file->f_path.dentry);
-	path.mnt    = mntget(shp->shm_file->f_path.mnt);
+	path.mnt    = shp->shm_file->f_path.mnt;
 	shp->shm_nattch++;
 	size = i_size_read(path.dentry->d_inode);
 	shm_unlock(shp);
@@ -907,18 +907,16 @@ long do_shmat(int shmid, char __user *sh
 	err = -ENOMEM;
 	sfd = kzalloc(sizeof(*sfd), GFP_KERNEL);
 	if (!sfd)
-		goto out_put_path;
+		goto out_put_dentry;
 
 	err = -ENOMEM;
-	file = get_empty_filp();
+
+	file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
 	if (!file)
 		goto out_free;
 
-	file->f_op = &shm_file_operations;
 	file->private_data = sfd;
-	file->f_path = path;
 	file->f_mapping = shp->shm_file->f_mapping;
-	file->f_mode = f_mode;
 	sfd->id = shp->id;
 	sfd->ns = get_ipc_ns(ns);
 	sfd->file = shp->shm_file;
@@ -969,9 +967,8 @@ out_unlock:
 
 out_free:
 	kfree(sfd);
-out_put_path:
+out_put_dentry:
 	dput(path.dentry);
-	mntput(path.mnt);
 	goto out_nattch;
 }
 
mm/shmem.c renamed from mm/shmem.c~01-24-filesystem-helpers-for-custom-struct-file-s +2 -5
diff -puN mm/shmem.c~01-24-filesystem-helpers-for-custom-struct-file-s mm/shmem.c
@@ -2567,11 +2567,8 @@ struct file *shmem_file_setup(char *name
 	d_instantiate(dentry, inode);
 	inode->i_size = size;
 	inode->i_nlink = 0;	/* It is unlinked */
-	file->f_path.mnt = mntget(shm_mnt);
-	file->f_path.dentry = dentry;
-	file->f_mapping = inode->i_mapping;
-	file->f_op = &shmem_file_operations;
-	file->f_mode = FMODE_WRITE | FMODE_READ;
+	init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
+			&shmem_file_operations);
 	return file;
 
 close_file:
mm/tiny-shmem.c renamed from mm/tiny-shmem.c~01-24-filesystem-helpers-for-custom-struct-file-s +7 -12
diff -puN mm/tiny-shmem.c~01-24-filesystem-helpers-for-custom-struct-file-s mm/tiny-shmem.c
@@ -66,24 +66,19 @@ struct file *shmem_file_setup(char *name
 	if (!dentry)
 		goto put_memory;
 
-	error = -ENFILE;
-	file = get_empty_filp();
-	if (!file)
-		goto put_dentry;
-
 	error = -ENOSPC;
 	inode = ramfs_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0);
 	if (!inode)
-		goto close_file;
+		goto put_dentry;
 
 	d_instantiate(dentry, inode);
-	inode->i_nlink = 0;	/* It is unlinked */
+	error = -ENFILE;
+	file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
+			&ramfs_file_operations);
+	if (!file)
+		goto put_dentry;
 
-	file->f_path.mnt = mntget(shm_mnt);
-	file->f_path.dentry = dentry;
-	file->f_mapping = inode->i_mapping;
-	file->f_op = &ramfs_file_operations;
-	file->f_mode = FMODE_WRITE | FMODE_READ;
+	inode->i_nlink = 0;	/* It is unlinked */
 
 	/* notify everyone as to the change of file size */
 	error = do_truncate(dentry, size, 0, file);
net/socket.c renamed from net/socket.c~01-24-filesystem-helpers-for-custom-struct-file-s +10 -9
diff -puN net/socket.c~01-24-filesystem-helpers-for-custom-struct-file-s net/socket.c
@@ -355,31 +355,32 @@ static int sock_alloc_fd(struct file **f
 
 static int sock_attach_fd(struct socket *sock, struct file *file)
 {
+	struct dentry *dentry;
 	struct qstr this;
 	char name[32];
 
 	this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
 	this.name = name;
 	this.hash = 0;
+	struct path;
 
-	file->f_path.dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);
-	if (unlikely(!file->f_path.dentry))
+	dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);
+	if (unlikely(!dentry))
 		return -ENOMEM;
 
-	file->f_path.dentry->d_op = &sockfs_dentry_operations;
+	dentry->d_op = &sockfs_dentry_operations;
 	/*
 	 * We dont want to push this dentry into global dentry hash table.
 	 * We pretend dentry is already hashed, by unsetting DCACHE_UNHASHED
 	 * This permits a working /proc/$pid/fd/XXX on sockets
 	 */
-	file->f_path.dentry->d_flags &= ~DCACHE_UNHASHED;
-	d_instantiate(file->f_path.dentry, SOCK_INODE(sock));
-	file->f_path.mnt = mntget(sock_mnt);
-	file->f_mapping = file->f_path.dentry->d_inode->i_mapping;
-
+	dentry->d_flags &= ~DCACHE_UNHASHED;
+	d_instantiate(dentry, SOCK_INODE(sock));
+	init_file(file, sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
+		&socket_file_ops);
+	SOCK_INODE(sock)->i_fop = &socket_file_ops;
 	sock->file = file;
 	file->f_op = SOCK_INODE(sock)->i_fop = &socket_file_ops;
-	file->f_mode = FMODE_READ | FMODE_WRITE;
 	file->f_flags = O_RDWR;
 	file->f_pos = 0;
 	file->private_data = sock;

_

#8 lore raw

Re: [PATCH 04/26] filesystem helpers for custom 'struct file's

Quoted 9 lines · Dave Hansen

Christoph H. says this stands on its own and can go in before the rest of the r/o bind mount set.

---

Some filesystems forego the vfs and may_open() and create their own 'struct file's.

Quoted 6 lines

This patch creates a couple of helper functions which can be used by these filesystems, and will provide a unified place which the r/o bind mount code may patch.

Also, rename two existing, static-scope init_file() to less generic names.

Looks good. Again this should go in ASAP after 2.6.23 independent of the more complicated bits because it's a nice cleanup. In the end get_empty_filp should go away as an exported symbol.

Note that we've grown more instances of the crap you're fixing here, e.g. fs/anon_inode.c

#9 lore raw

Re: [PATCH 04/26] filesystem helpers for custom 'struct file's

Quoted 2 lines · Christoph Hellwig

Note that we've grown more instances of the crap you're fixing here, e.g. fs/anon_inode.c

Ugh. I'll go clean that one up.

-- Dave

maintainer
#10 lore raw

Re: [PATCH 04/26] filesystem helpers for custom 'struct file's

In reply to Dave Hansen
Quoted 9 lines · On Fri, 22 Jun 2007 13:03:08 -0700 Dave Hansen <[email protected]> wrote:

Christoph H. says this stands on its own and can go in before the rest of the r/o bind mount set.

---

Some filesystems forego the vfs and may_open() and create their own 'struct file's.

Quoted 6 lines

This patch creates a couple of helper functions which can be used by these filesystems, and will provide a unified place which the r/o bind mount code may patch.

Also, rename two existing, static-scope init_file() to less generic names.

the sysfs changes are no longer applicable here due to pending changes in Greg's tree.

The net/socket.c change is a mystery. My version of sock_attach_fd() doesn't look like yours.

So I snarfed the first three patches, passed on this one and shall await a quality review-and-ack from someone who is appropriately familiar with this code, thanks.

#11 lore raw

Re: [PATCH 04/26] filesystem helpers for custom 'struct file's

Quoted 13 lines · Andrew Morton
Quoted 13 lines · On Fri, 22 Jun 2007 13:03:08 -0700 Dave Hansen <[email protected]> wrote:

Christoph H. says this stands on its own and can go in before the rest of the r/o bind mount set.

---

Some filesystems forego the vfs and may_open() and create their own 'struct file's.

This patch creates a couple of helper functions which can be used by these filesystems, and will provide a unified place which the r/o bind mount code may patch.

Quoted 8 lines
Quoted 2 lines

Also, rename two existing, static-scope init_file() to less generic names.

the sysfs changes are no longer applicable here due to pending changes in Greg's tree.

The net/socket.c change is a mystery. My version of sock_attach_fd() doesn't look like yours.

Should I rebase them on top of -mm, or a just current -rc?

-- Dave

maintainer
#12 lore raw

Re: [PATCH 04/26] filesystem helpers for custom 'struct file's

Quoted 17 lines · On Mon, 25 Jun 2007 08:37:29 -0700 Dave Hansen <[email protected]> wrote:
Quoted 16 lines · Andrew Morton
Quoted 15 lines · On Fri, 22 Jun 2007 13:03:08 -0700 Dave Hansen <[email protected]> wrote:

Christoph H. says this stands on its own and can go in before the rest of the r/o bind mount set.

---

Some filesystems forego the vfs and may_open() and create their own 'struct file's.

This patch creates a couple of helper functions which can be used by these filesystems, and will provide a unified place which the r/o bind mount code may patch.

Also, rename two existing, static-scope init_file() to less generic names.

Quoted 8 lines
Quoted 5 lines

the sysfs changes are no longer applicable here due to pending changes in Greg's tree.

The net/socket.c change is a mystery. My version of sock_attach_fd() doesn't look like yours.

Should I rebase them on top of -mm, or a just current -rc?

Weren't they already against current -rc? I really am mystified about where your net/socket.c cam from, although I didn't search super-hard (I was offline at the time).

Normally against current -rc, but the pending sysfs changes do mean that it perhaps would be better if you were to runtime test it at least against Greg's tree.

#13 lore raw

Re: [PATCH 04/26] filesystem helpers for custom 'struct file's

Quoted 11 lines · Andrew Morton
Quoted 6 lines · On Mon, 25 Jun 2007 08:37:29 -0700 Dave Hansen <[email protected]> wrote:
Quoted 2 lines · Andrew Morton

The net/socket.c change is a mystery. My version of sock_attach_fd() doesn't look like yours.

Should I rebase them on top of -mm, or a just current -rc?

Weren't they already against current -rc? I really am mystified about where your net/socket.c cam from, although I didn't search super-hard (I was offline at the time).

Looks like I had them againt plain 2.6.21. That'll do it.

I'll rebase and test with Greg's tree.

#14 lore raw

Re: [PATCH 04/26] filesystem helpers for custom 'struct file's

Quoted 3 lines · Dave Hansen

Looks like I had them againt plain 2.6.21. That'll do it.

I'll rebase and test with Greg's tree.

Can you send it out ASAP? I want to do the indepth review so we can get this in ASAP and kill the get_empty_filp export eventually. We're growing far too many opencoded instance of this crap and need to stop it now.

#15 lore raw

PATCH 5/26 r/o bind mounts: stub functions

In reply to Dave Hansen

This patch adds two function mnt_want_write() and mnt_drop_write(). These are used like a lock pair around and fs operations that might cause a write to the filesystem.

Before these can become useful, we must first cover each place in the VFS where writes are performed with a want/drop pair. When that is complete, we can actually introduce code that will safely check the counts before allowing r/w<->r/o transitions to occur.

Note that we put the linux/fs.h #include as far down in mount.h as possible. This is to keep the mount.h->fs.h-> sched.h->mount.h include dependency from biting us.

---

 lxc-dave/fs/namespace.c        |   13 +++++++++++++
 lxc-dave/include/linux/mount.h |   13 +++++++++++++
 2 files changed, 26 insertions(+)
fs/namespace.c renamed from fs/namespace.c~03-24-add-vfsmount-writer-count +13 -0
diff -puN fs/namespace.c~03-24-add-vfsmount-writer-count fs/namespace.c
@@ -76,6 +76,19 @@ struct vfsmount *alloc_vfsmnt(const char
 	return mnt;
 }
 
+int mnt_want_write(struct vfsmount *mnt)
+{
+	if (__mnt_is_readonly(mnt))
+		return -EROFS;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mnt_want_write);
+
+void mnt_drop_write(struct vfsmount *mnt)
+{
+}
+EXPORT_SYMBOL_GPL(mnt_drop_write);
+
 int simple_set_mnt(struct vfsmount *mnt, struct super_block *sb)
 {
 	mnt->mnt_sb = sb;
include/linux/mount.h renamed from include/linux/mount.h~03-24-add-vfsmount-writer-count +13 -0
diff -puN include/linux/mount.h~03-24-add-vfsmount-writer-count include/linux/mount.h
@@ -70,6 +70,19 @@ static inline struct vfsmount *mntget(st
 	return mnt;
 }
 
+#include <linux/fs.h>
+
+/*
+ * This shouldn't be used directly ouside of the VFS,
+ * use mnt_want/drop_write() instead.
+ */
+static inline int __mnt_is_readonly(struct vfsmount *mnt)
+{
+	return (mnt->mnt_sb->s_flags & MS_RDONLY);
+}
+
+extern int mnt_want_write(struct vfsmount *mnt);
+extern void mnt_drop_write(struct vfsmount *mnt);
 extern void mntput_no_expire(struct vfsmount *mnt);
 extern void mnt_pin(struct vfsmount *mnt);
 extern void mnt_unpin(struct vfsmount *mnt);

_

#16 lore raw

Re: [PATCH 05/26] r/o bind mounts: stub functions

Quoted 10 lines · Dave Hansen

This patch adds two function mnt_want_write() and mnt_drop_write(). These are used like a lock pair around and fs operations that might cause a write to the filesystem.

Before these can become useful, we must first cover each place in the VFS where writes are performed with a want/drop pair. When that is complete, we can actually introduce code that will safely check the counts before allowing r/w<->r/o transitions to occur.

Ok,

Quoted 4 lines

Note that we put the linux/fs.h #include as far down in mount.h as possible. This is to keep the mount.h->fs.h-> sched.h->mount.h include dependency from biting us.

except for this. Please just move __mnt_is_readonly out of line or make it a macro, but we surely don't want to include fs.h in mount.h

maintainer
#17 lore raw

Re: [PATCH 05/26] r/o bind mounts: stub functions

In reply to Dave Hansen
Quoted 3 lines · On Fri, 22 Jun 2007 13:03:09 -0700 Dave Hansen <[email protected]> wrote:

This patch adds two function mnt_want_write() and
mnt_drop_write()

ITYM "global, exported-to-modules yet 100% undocumented" functions.

tsk.

#18 lore raw

Re: [PATCH 05/26] r/o bind mounts: stub functions

Quoted 5 lines · Andrew Morton
Quoted 3 lines · On Fri, 22 Jun 2007 13:03:09 -0700 Dave Hansen <[email protected]> wrote:

This patch adds two function mnt_want_write() and
mnt_drop_write()

ITYM "global, exported-to-modules yet 100% undocumented" functions.

Point taken. :)

I've added some big fat comments on these functions' definitions.

-- Dave

#19 lore raw

PATCH 6/26 elevate write count open()'d files

In reply to Dave Hansen

This is the first really tricky patch in the series. It elevates the writer count on a mount each time a non-special file is opened for write.

This is not completely apparent in the patch because the two if() conditions in may_open() above the mnt_want_write() call are, combined, equivalent to special_file().

There is also an elevated count around the vfs_create() call in open_namei(). The count needs to be kept elevated all the way into the may_open() call. Otherwise, when the write is dropped, a ro->rw transisition could occur. This would lead to having rw access on the newly created file, while the vfsmount is ro. That is bad.

Some filesystems forego the use of normal vfs calls to create struct files. Make sure that these users elevate the mnt writer count because they will get __fput(), and we need to make sure they're balanced.

---

 lxc-dave/fs/file_table.c |    9 ++++++++-
 lxc-dave/fs/namei.c      |   20 ++++++++++++++++----
 lxc-dave/ipc/mqueue.c    |    3 +++
 3 files changed, 27 insertions(+), 5 deletions(-)
fs/file_table.c renamed from fs/file_table.c~14-24-tricky-elevate-write-count-files-are-open-ed +8 -1
diff -puN fs/file_table.c~14-24-tricky-elevate-write-count-files-are-open-ed fs/file_table.c
@@ -169,6 +169,10 @@ int init_file(struct file *file, struct 
 	file->f_mapping = dentry->d_inode->i_mapping;
 	file->f_mode = mode;
 	file->f_op = fop;
+	if (mode & FMODE_WRITE) {
+		error = mnt_want_write(mnt);
+		WARN_ON(error);
+	}
 	return error;
 }
 EXPORT_SYMBOL(init_file);
@@ -206,8 +210,11 @@ void fastcall __fput(struct file *file)
 	if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL))
 		cdev_put(inode->i_cdev);
 	fops_put(file->f_op);
-	if (file->f_mode & FMODE_WRITE)
+	if (file->f_mode & FMODE_WRITE) {
 		put_write_access(inode);
+		if (!special_file(inode->i_mode))
+			mnt_drop_write(mnt);
+	}
 	put_pid(file->f_owner.pid);
 	file_kill(file);
 	file->f_path.dentry = NULL;
fs/namei.c renamed from fs/namei.c~14-24-tricky-elevate-write-count-files-are-open-ed +16 -4
diff -puN fs/namei.c~14-24-tricky-elevate-write-count-files-are-open-ed fs/namei.c
@@ -1544,8 +1544,15 @@ int may_open(struct nameidata *nd, int a
 			return -EACCES;
 
 		flag &= ~O_TRUNC;
-	} else if (IS_RDONLY(inode) && (flag & FMODE_WRITE))
-		return -EROFS;
+	} else if (flag & FMODE_WRITE) {
+		/*
+		 * effectively: !special_file()
+		 * balanced by __fput()
+		 */
+		error = mnt_want_write(nd->mnt);
+		if (error)
+			return error;
+	}
 	/*
 	 * An append-only file must be opened in append mode for writing.
 	 */
@@ -1684,14 +1691,17 @@ do_last:
 	}
 
 	if (IS_ERR(nd->intent.open.file)) {
-		mutex_unlock(&dir->d_inode->i_mutex);
 		error = PTR_ERR(nd->intent.open.file);
-		goto exit_dput;
+		goto exit_mutex_unlock;
 	}
 
 	/* Negative dentry, just create the file */
 	if (!path.dentry->d_inode) {
+		error = mnt_want_write(nd->mnt);
+		if (error)
+			goto exit_mutex_unlock;
 		error = open_namei_create(nd, &path, flag, mode);
+		mnt_drop_write(nd->mnt);
 		if (error)
 			goto exit;
 		return 0;
@@ -1729,6 +1739,8 @@ ok:
 		goto exit;
 	return 0;
 
+exit_mutex_unlock:
+	mutex_unlock(&dir->d_inode->i_mutex);
 exit_dput:
 	dput_path(&path, nd);
 exit:
ipc/mqueue.c renamed from ipc/mqueue.c~14-24-tricky-elevate-write-count-files-are-open-ed +3 -0
diff -puN ipc/mqueue.c~14-24-tricky-elevate-write-count-files-are-open-ed ipc/mqueue.c
@@ -687,6 +687,9 @@ asmlinkage long sys_mq_open(const char _
 				goto out;
 			filp = do_open(dentry, oflag);
 		} else {
+			error = mnt_want_write(mqueue_mnt);
+			if (error)
+				goto out;
 			filp = do_create(mqueue_mnt->mnt_root, dentry,
 						oflag, mode, u_attr);
 		}

diff -puN fs/ext2/ioctl.c~14-24-tricky-elevate-write-count-files-are-open-ed fs/ext2/ioctl.c _

#20 lore raw

Re: [PATCH 06/26] elevate write count open()'d files

Quoted 10 lines · Dave Hansen

This is the first really tricky patch in the series. It elevates the writer count on a mount each time a non-special file is opened for write.

This is not completely apparent in the patch because the two if() conditions in may_open() above the mnt_want_write() call are, combined, equivalent to special_file().

Quoted 6 lines

There is also an elevated count around the vfs_create() call in open_namei(). The count needs to be kept elevated all the way into the may_open() call. Otherwise, when the write is dropped, a ro->rw transisition could occur. This would lead to having rw access on the newly created file, while the vfsmount is ro. That is bad.

Looks good.

Quoted 5 lines

Some filesystems forego the use of normal vfs calls to create struct files. Make sure that these users elevate the mnt writer count because they will get __fput(), and we need to make sure they're balanced.

With this you mean ipc/mqueue.c? You should probably mention that single bastard child of a wannabe-filesystem explicitly :)

#21 lore raw

Re: [PATCH 06/26] elevate write count open()'d files

Quoted 8 lines · Christoph Hellwig
Quoted 5 lines

Some filesystems forego the use of normal vfs calls to create struct files. Make sure that these users elevate the mnt writer count because they will get __fput(), and we need to make sure they're balanced.

With this you mean ipc/mqueue.c? You should probably mention that single bastard child of a wannabe-filesystem explicitly :)

I was referring to all the ones that use the "filesystem helpers" from patch 04/26.

-- Dave

#22 lore raw

PATCH 7/26 r/o bind mounts: elevate write count for some ioctls

In reply to Dave Hansen

Some ioctl()s can cause writes to the filesystem. Take these, and make them use mnt_want/drop_write() instead.

We need to pass the filp one layer deeper in XFS, but somebody _just_ pulled it out in February because nobody was using it, so I don't feel guilty for adding it back.

---

 lxc-dave/fs/ext2/ioctl.c              |   46 +++++++++-----
 lxc-dave/fs/ext3/ioctl.c              |  100 +++++++++++++++++++++-----------
 lxc-dave/fs/ext4/ioctl.c              |  105 +++++++++++++++++++++-------------
 lxc-dave/fs/fat/file.c                |   10 +--
 lxc-dave/fs/hfsplus/ioctl.c           |   39 +++++++-----
 lxc-dave/fs/jfs/ioctl.c               |   33 ++++++----
 lxc-dave/fs/ocfs2/ioctl.c             |   11 +--
 lxc-dave/fs/reiserfs/ioctl.c          |   53 +++++++++++------
 lxc-dave/fs/xfs/linux-2.6/xfs_ioctl.c |   15 +++-
 lxc-dave/fs/xfs/linux-2.6/xfs_iops.c  |    7 --
 lxc-dave/fs/xfs/linux-2.6/xfs_lrw.c   |    9 ++
 11 files changed, 272 insertions(+), 156 deletions(-)
fs/ext2/ioctl.c renamed from fs/ext2/ioctl.c~05-24-ioctl-mnt-takers +30 -16
diff -puN fs/ext2/ioctl.c~05-24-ioctl-mnt-takers fs/ext2/ioctl.c
@@ -12,6 +12,7 @@
 #include <linux/time.h>
 #include <linux/sched.h>
 #include <linux/compat.h>
+#include <linux/mount.h>
 #include <linux/smp_lock.h>
 #include <asm/current.h>
 #include <asm/uaccess.h>
@@ -22,6 +23,7 @@ int ext2_ioctl (struct inode * inode, st
 {
 	struct ext2_inode_info *ei = EXT2_I(inode);
 	unsigned int flags;
+	int ret;
 
 	ext2_debug ("cmd = %u, arg = %lu\n", cmd, arg);
 
@@ -32,14 +34,19 @@ int ext2_ioctl (struct inode * inode, st
 	case EXT2_IOC_SETFLAGS: {
 		unsigned int oldflags;
 
-		if (IS_RDONLY(inode))
-			return -EROFS;
-
-		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
-			return -EACCES;
+		ret = mnt_want_write(filp->f_vfsmnt);
+		if (ret)
+			return ret;
+
+		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER)) {
+			ret = -EACCES;
+			goto setflags_out;
+		}
 
-		if (get_user(flags, (int __user *) arg))
-			return -EFAULT;
+		if (get_user(flags, (int __user *) arg)) {
+			ret = -EFAULT;
+			goto setflags_out;
+		}
 
 		if (!S_ISDIR(inode->i_mode))
 			flags &= ~EXT2_DIRSYNC_FL;
@@ -56,7 +63,8 @@ int ext2_ioctl (struct inode * inode, st
 		if ((flags ^ oldflags) & (EXT2_APPEND_FL | EXT2_IMMUTABLE_FL)) {
 			if (!capable(CAP_LINUX_IMMUTABLE)) {
 				mutex_unlock(&inode->i_mutex);
-				return -EPERM;
+				ret = -EPERM;
+				goto setflags_out;
 			}
 		}
 
@@ -68,20 +76,26 @@ int ext2_ioctl (struct inode * inode, st
 		ext2_set_inode_flags(inode);
 		inode->i_ctime = CURRENT_TIME_SEC;
 		mark_inode_dirty(inode);
-		return 0;
+setflags_out:
+		mnt_drop_write(filp->f_vfsmnt);
+		return ret;
 	}
 	case EXT2_IOC_GETVERSION:
 		return put_user(inode->i_generation, (int __user *) arg);
 	case EXT2_IOC_SETVERSION:
 		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
 			return -EPERM;
-		if (IS_RDONLY(inode))
-			return -EROFS;
-		if (get_user(inode->i_generation, (int __user *) arg))
-			return -EFAULT;	
-		inode->i_ctime = CURRENT_TIME_SEC;
-		mark_inode_dirty(inode);
-		return 0;
+		ret = mnt_want_write(filp->f_vfsmnt);
+		if (ret)
+			return ret;
+		if (get_user(inode->i_generation, (int __user *) arg)) {
+			ret = -EFAULT;
+		} else {
+			inode->i_ctime = CURRENT_TIME_SEC;
+			mark_inode_dirty(inode);
+		}
+		mnt_drop_write(filp->f_vfsmnt);
+		return ret;
 	default:
 		return -ENOTTY;
 	}
fs/ext3/ioctl.c renamed from fs/ext3/ioctl.c~05-24-ioctl-mnt-takers +66 -34

193 lines not on this page · raw message

fs/ext4/ioctl.c renamed from fs/ext4/ioctl.c~05-24-ioctl-mnt-takers +67 -38

202 lines not on this page · raw message

fs/fat/file.c renamed from fs/fat/file.c~05-24-ioctl-mnt-takers +6 -4
diff -puN fs/fat/file.c~05-24-ioctl-mnt-takers fs/fat/file.c
@@ -8,6 +8,7 @@
 
 #include <linux/capability.h>
 #include <linux/module.h>
+#include <linux/mount.h>
 #include <linux/time.h>
 #include <linux/msdos_fs.h>
 #include <linux/smp_lock.h>
@@ -46,10 +47,9 @@ int fat_generic_ioctl(struct inode *inod
 
 		mutex_lock(&inode->i_mutex);
 
-		if (IS_RDONLY(inode)) {
-			err = -EROFS;
-			goto up;
-		}
+		err = mnt_want_write(filp->f_vfsmnt);
+		if (err)
+			goto up_no_drop_write;
 
 		/*
 		 * ATTR_VOLUME and ATTR_DIR cannot be changed; this also
@@ -106,6 +106,8 @@ int fat_generic_ioctl(struct inode *inod
 		MSDOS_I(inode)->i_attrs = attr & ATTR_UNUSED;
 		mark_inode_dirty(inode);
 	up:
+		mnt_drop_write(filp->f_vfsmnt);
+	up_no_drop_write:
 		mutex_unlock(&inode->i_mutex);
 		return err;
 	}
fs/hfsplus/ioctl.c renamed from fs/hfsplus/ioctl.c~05-24-ioctl-mnt-takers +24 -15
diff -puN fs/hfsplus/ioctl.c~05-24-ioctl-mnt-takers fs/hfsplus/ioctl.c
@@ -35,25 +35,32 @@ int hfsplus_ioctl(struct inode *inode, s
 			flags |= FS_NODUMP_FL; /* EXT2_NODUMP_FL */
 		return put_user(flags, (int __user *)arg);
 	case HFSPLUS_IOC_EXT2_SETFLAGS: {
-		if (IS_RDONLY(inode))
-			return -EROFS;
-
-		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
-			return -EACCES;
-
-		if (get_user(flags, (int __user *)arg))
-			return -EFAULT;
-
+		int err = 0;
+		err = mnt_want_write(filp->f_vfsmnt);
+		if (err)
+			return err;
+
+		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER)) {
+			err = -EACCES;
+			goto setflags_out;
+		}
+		if (get_user(flags, (int __user *)arg)) {
+			err = -EFAULT;
+			goto setflags_out;
+		}
 		if (flags & (FS_IMMUTABLE_FL|FS_APPEND_FL) ||
 		    HFSPLUS_I(inode).rootflags & (HFSPLUS_FLG_IMMUTABLE|HFSPLUS_FLG_APPEND)) {
-			if (!capable(CAP_LINUX_IMMUTABLE))
-				return -EPERM;
+			if (!capable(CAP_LINUX_IMMUTABLE)) {
+				err = -EPERM;
+				goto setflags_out;
+			}
 		}
 
 		/* don't silently ignore unsupported ext2 flags */
-		if (flags & ~(FS_IMMUTABLE_FL|FS_APPEND_FL|FS_NODUMP_FL))
-			return -EOPNOTSUPP;
-
+		if (flags & ~(FS_IMMUTABLE_FL|FS_APPEND_FL|FS_NODUMP_FL)) {
+			err = -EOPNOTSUPP;
+			goto setflags_out;
+		}
 		if (flags & FS_IMMUTABLE_FL) { /* EXT2_IMMUTABLE_FL */
 			inode->i_flags |= S_IMMUTABLE;
 			HFSPLUS_I(inode).rootflags |= HFSPLUS_FLG_IMMUTABLE;
@@ -75,7 +82,9 @@ int hfsplus_ioctl(struct inode *inode, s
 
 		inode->i_ctime = CURRENT_TIME_SEC;
 		mark_inode_dirty(inode);
-		return 0;
+setflags_out:
+		mnt_drop_write(filp->f_vfsmnt);
+		return err;
 	}
 	default:
 		return -ENOTTY;
fs/jfs/ioctl.c renamed from fs/jfs/ioctl.c~05-24-ioctl-mnt-takers +21 -12
diff -puN fs/jfs/ioctl.c~05-24-ioctl-mnt-takers fs/jfs/ioctl.c
@@ -8,6 +8,7 @@
 #include <linux/fs.h>
 #include <linux/ctype.h>
 #include <linux/capability.h>
+#include <linux/mount.h>
 #include <linux/time.h>
 #include <linux/sched.h>
 #include <asm/current.h>
@@ -64,16 +65,20 @@ int jfs_ioctl(struct inode * inode, stru
 		return put_user(flags, (int __user *) arg);
 	case JFS_IOC_SETFLAGS: {
 		unsigned int oldflags;
+		int err;
 
-		if (IS_RDONLY(inode))
-			return -EROFS;
-
-		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
-			return -EACCES;
-
-		if (get_user(flags, (int __user *) arg))
-			return -EFAULT;
-
+		err = mnt_want_write(filp->f_vfsmnt);
+		if (err)
+			return err;
+
+		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER)) {
+			err = -EACCES;
+			goto setflags_out;
+		}
+		if (get_user(flags, (int __user *) arg)) {
+			err = -EFAULT;
+			goto setflags_out;
+		}
 		flags = jfs_map_ext2(flags, 1);
 		if (!S_ISDIR(inode->i_mode))
 			flags &= ~JFS_DIRSYNC_FL;
@@ -87,8 +92,10 @@ int jfs_ioctl(struct inode * inode, stru
 		if ((oldflags & JFS_IMMUTABLE_FL) ||
 			((flags ^ oldflags) &
 			(JFS_APPEND_FL | JFS_IMMUTABLE_FL))) {
-			if (!capable(CAP_LINUX_IMMUTABLE))
-				return -EPERM;
+			if (!capable(CAP_LINUX_IMMUTABLE)) {
+				err = -EPERM;
+				goto setflags_out;
+			}
 		}
 
 		flags = flags & JFS_FL_USER_MODIFIABLE;
@@ -98,7 +105,9 @@ int jfs_ioctl(struct inode * inode, stru
 		jfs_set_inode_flags(inode);
 		inode->i_ctime = CURRENT_TIME_SEC;
 		mark_inode_dirty(inode);
-		return 0;
+setflags_out:
+		mnt_drop_write(filp->f_vfsmnt);
+		return err;
 	}
 	default:
 		return -ENOTTY;
fs/ocfs2/ioctl.c renamed from fs/ocfs2/ioctl.c~05-24-ioctl-mnt-takers +6 -5
diff -puN fs/ocfs2/ioctl.c~05-24-ioctl-mnt-takers fs/ocfs2/ioctl.c
@@ -56,10 +56,6 @@ static int ocfs2_set_inode_attr(struct i
 		goto bail;
 	}
 
-	status = -EROFS;
-	if (IS_RDONLY(inode))
-		goto bail_unlock;
-
 	status = -EACCES;
 	if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
 		goto bail_unlock;
@@ -127,8 +123,13 @@ int ocfs2_ioctl(struct inode * inode, st
 		if (get_user(flags, (int __user *) arg))
 			return -EFAULT;
 
-		return ocfs2_set_inode_attr(inode, flags,
+		status = mnt_want_write(filp->f_vfsmnt);
+		if (status)
+			return status;
+		status = ocfs2_set_inode_attr(inode, flags,
 			OCFS2_FL_MODIFIABLE);
+		mnt_drop_write(filp->f_vfsmnt);
+		return status;
 	default:
 		return -ENOTTY;
 	}
fs/reiserfs/ioctl.c renamed from fs/reiserfs/ioctl.c~05-24-ioctl-mnt-takers +34 -19

94 lines not on this page · raw message

fs/xfs/linux-2.6/xfs_ioctl.c renamed from fs/xfs/linux-2.6/xfs_ioctl.c~05-24-ioctl-mnt-takers +10 -5
diff -puN fs/xfs/linux-2.6/xfs_ioctl.c~05-24-ioctl-mnt-takers fs/xfs/linux-2.6/xfs_ioctl.c
@@ -526,8 +526,6 @@ xfs_attrmulti_attr_set(
 	char			*kbuf;
 	int			error = EFAULT;
 
-	if (IS_RDONLY(&vp->v_inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(&vp->v_inode) || IS_APPEND(&vp->v_inode))
 		return EPERM;
 	if (len > XATTR_SIZE_MAX)
@@ -553,8 +551,6 @@ xfs_attrmulti_attr_remove(
 	char			*name,
 	__uint32_t		flags)
 {
-	if (IS_RDONLY(&vp->v_inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(&vp->v_inode) || IS_APPEND(&vp->v_inode))
 		return EPERM;
 	return bhv_vop_attr_remove(vp, name, flags, NULL);
@@ -564,6 +560,7 @@ STATIC int
 xfs_attrmulti_by_handle(
 	xfs_mount_t		*mp,
 	void			__user *arg,
+	struct file		*parfilp,
 	struct inode		*parinode)
 {
 	int			error;
@@ -618,13 +615,21 @@ xfs_attrmulti_by_handle(
 					&ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
+			ops[i].am_error = mnt_want_write(parfilp->f_vfsmnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_set(vp,
 					attr_name, ops[i].am_attrvalue,
 					ops[i].am_length, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_vfsmnt);
 			break;
 		case ATTR_OP_REMOVE:
+			ops[i].am_error = mnt_want_write(parfilp->f_vfsmnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_remove(vp,
 					attr_name, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_vfsmnt);
 			break;
 		default:
 			ops[i].am_error = EINVAL;
@@ -804,7 +809,7 @@ xfs_ioctl(
 		return xfs_attrlist_by_handle(mp, arg, inode);
 
 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
-		return xfs_attrmulti_by_handle(mp, arg, inode);
+		return xfs_attrmulti_by_handle(mp, arg, filp, inode);
 
 	case XFS_IOC_SWAPEXT: {
 		error = xfs_swapext((struct xfs_swapext __user *)arg);
fs/xfs/linux-2.6/xfs_iops.c renamed from fs/xfs/linux-2.6/xfs_iops.c~05-24-ioctl-mnt-takers +0 -7
diff -puN fs/xfs/linux-2.6/xfs_iops.c~05-24-ioctl-mnt-takers fs/xfs/linux-2.6/xfs_iops.c
@@ -156,13 +156,6 @@ xfs_ichgtime_fast(
 	 */
 	ASSERT((flags & XFS_ICHGTIME_ACC) == 0);
 
-	/*
-	 * We're not supposed to change timestamps in readonly-mounted
-	 * filesystems.  Throw it away if anyone asks us.
-	 */
-	if (unlikely(IS_RDONLY(inode)))
-		return;
-
 	if (flags & XFS_ICHGTIME_MOD) {
 		tvp = &inode->i_mtime;
 		ip->i_d.di_mtime.t_sec = (__int32_t)tvp->tv_sec;
fs/xfs/linux-2.6/xfs_lrw.c renamed from fs/xfs/linux-2.6/xfs_lrw.c~05-24-ioctl-mnt-takers +8 -1
diff -puN fs/xfs/linux-2.6/xfs_lrw.c~05-24-ioctl-mnt-takers fs/xfs/linux-2.6/xfs_lrw.c
@@ -50,6 +50,7 @@
 #include "xfs_iomap.h"
 
 #include <linux/capability.h>
+#include <linux/mount.h>
 #include <linux/writeback.h>
 
 
@@ -762,10 +763,16 @@ start:
 		}
 	}
 
-	if (likely(!(ioflags & IO_INVIS))) {
+	/*
+	 * We're not supposed to change timestamps in readonly-mounted
+	 * filesystems.  Throw it away if anyone asks us.
+	 */
+	if (likely(!(ioflags & IO_INVIS) &&
+		   !mnt_want_write(file->f_vfsmnt))) {
 		file_update_time(file);
 		xfs_ichgtime_fast(xip, inode,
 				  XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+		mnt_drop_write(file->f_vfsmnt);
 	}
 
 	/*

_

#23 lore raw

Re: [PATCH 07/26] r/o bind mounts: elevate write count for some ioctls

Quoted 7 lines · Dave Hansen

Some ioctl()s can cause writes to the filesystem. Take these, and make them use mnt_want/drop_write() instead.

We need to pass the filp one layer deeper in XFS, but somebody _just_ pulled it out in February because nobody was using it, so I don't feel guilty for adding it back.

Could have been me :)

Would be nicer to just pass down the vfsmount instead of the file in XFS, but it shouldn't really matter in the end.

#24 lore raw

PATCH 8/26 elevate writer count for chown and friends

In reply to Dave Hansen

chown/chmod,etc... don't call permission in the same way that the normal "open for write" calls do. They still write to the filesystem, so bump the write count during these operations.

---

 lxc-dave/fs/open.c |   39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)
fs/open.c renamed from fs/open.c~06-24-elevate-writer-count-for-chown-and-friends +30 -9
diff -puN fs/open.c~06-24-elevate-writer-count-for-chown-and-friends fs/open.c
@@ -508,12 +508,12 @@ asmlinkage long sys_fchmod(unsigned int 
 
 	audit_inode(NULL, inode);
 
-	err = -EROFS;
-	if (IS_RDONLY(inode))
+	err = mnt_want_write(file->f_vfsmnt);
+	if (err)
 		goto out_putf;
 	err = -EPERM;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		goto out_putf;
+		goto out_drop_write;
 	mutex_lock(&inode->i_mutex);
 	if (mode == (mode_t) -1)
 		mode = inode->i_mode;
@@ -522,6 +522,8 @@ asmlinkage long sys_fchmod(unsigned int 
 	err = notify_change(dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
 
+out_drop_write:
+	mnt_drop_write(file->f_vfsmnt);
 out_putf:
 	fput(file);
 out:
@@ -541,13 +543,13 @@ asmlinkage long sys_fchmodat(int dfd, co
 		goto out;
 	inode = nd.dentry->d_inode;
 
-	error = -EROFS;
-	if (IS_RDONLY(inode))
+	error = mnt_want_write(nd.mnt);
+	if (error)
 		goto dput_and_out;
 
 	error = -EPERM;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		goto dput_and_out;
+		goto out_drop_write;
 
 	mutex_lock(&inode->i_mutex);
 	if (mode == (mode_t) -1)
@@ -557,6 +559,8 @@ asmlinkage long sys_fchmodat(int dfd, co
 	error = notify_change(nd.dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
 
+out_drop_write:
+	mnt_drop_write(nd.mnt);
 dput_and_out:
 	path_release(&nd);
 out:
@@ -579,9 +583,6 @@ static int chown_common(struct dentry * 
 		printk(KERN_ERR "chown_common: NULL inode\n");
 		goto out;
 	}
-	error = -EROFS;
-	if (IS_RDONLY(inode))
-		goto out;
 	error = -EPERM;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		goto out;
@@ -611,7 +612,12 @@ asmlinkage long sys_chown(const char __u
 	error = user_path_walk(filename, &nd);
 	if (error)
 		goto out;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto out_release;
 	error = chown_common(nd.dentry, user, group);
+	mnt_drop_write(nd.mnt);
+out_release:
 	path_release(&nd);
 out:
 	return error;
@@ -631,7 +637,12 @@ asmlinkage long sys_fchownat(int dfd, co
 	error = __user_walk_fd(dfd, filename, follow, &nd);
 	if (error)
 		goto out;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto out_release;
 	error = chown_common(nd.dentry, user, group);
+	mnt_drop_write(nd.mnt);
+out_release:
 	path_release(&nd);
 out:
 	return error;
@@ -645,7 +656,12 @@ asmlinkage long sys_lchown(const char __
 	error = user_path_walk_link(filename, &nd);
 	if (error)
 		goto out;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto out_release;
 	error = chown_common(nd.dentry, user, group);
+	mnt_drop_write(nd.mnt);
+out_release:
 	path_release(&nd);
 out:
 	return error;
@@ -662,9 +678,14 @@ asmlinkage long sys_fchown(unsigned int 
 	if (!file)
 		goto out;
 
+	error = mnt_want_write(file->f_vfsmnt);
+	if (error)
+		goto out_fput;
 	dentry = file->f_path.dentry;
 	audit_inode(NULL, dentry->d_inode);
 	error = chown_common(dentry, user, group);
+	mnt_drop_write(file->f_vfsmnt);
+out_fput:
 	fput(file);
 out:
 	return error;

_

#25 lore raw

Re: [PATCH 08/26] elevate writer count for chown and friends

Quoted 6 lines · Dave Hansen

chown/chmod,etc... don't call permission in the same way that the normal "open for write" calls do. They still write to the filesystem, so bump the write count during these operations.

Looks good.

#26 lore raw

PATCH 9/26 make access() use mnt check

In reply to Dave Hansen

It is OK to let access() go without using a mnt_want/drop_write() pair because it doesn't actually do writes to the filesystem, and it is inherently racy anyway. This is a rare case when it is OK to use __mnt_is_readonly() directly.

---

 lxc-dave/fs/open.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/open.c renamed from fs/open.c~make-access-use-helper +1 -1
diff -puN fs/open.c~make-access-use-helper fs/open.c
@@ -395,7 +395,7 @@ asmlinkage long sys_faccessat(int dfd, c
 	   special_file(nd.dentry->d_inode->i_mode))
 		goto out_path_release;
 
-	if(IS_RDONLY(nd.dentry->d_inode))
+	if (__mnt_is_readonly(nd.mnt))
 		res = -EROFS;
 
 out_path_release:

_

#27 lore raw

Re: [PATCH 09/26] make access() use mnt check

Quoted 5 lines · Dave Hansen

It is OK to let access() go without using a mnt_want/drop_write() pair because it doesn't actually do writes to the filesystem, and it is inherently racy anyway. This is a rare case when it is OK to use __mnt_is_readonly() directly.

You probably want to add a big comment explaining why it's fine here.

That reminds me of something else I had in mind to debug that the writer counts are okay:

we should probably add a check in permission that we have an elevated writercount on the vfsmount/sb. Of course we'll need some way to overrid it for access(), which means passing down a flag to it or something.

#28 lore raw

Re: [PATCH 09/26] make access() use mnt check

Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · Dave Hansen

It is OK to let access() go without using a mnt_want/drop_write() pair because it doesn't actually do writes to the filesystem, and it is inherently racy anyway. This is a rare case when it is OK to use __mnt_is_readonly() directly.

You probably want to add a big comment explaining why it's fine here.

I've got this in the next set:

-
- if(IS_RDONLY(nd.dentry->d_inode))
+ /*

+        * This is a rare case where using __mnt_is_readonly()
+        * is OK without a mnt_want/drop_write() pair.  Since
+        * not actual write to the fs is performed here, we do
+        * not need to telegraph to that to anyone.  Also, we
+        * accept that access is inherently racy, and know that
+        * the fs might be remounted between this syscall, and
+        * any action taken because of its result.
+        */
+       if (__mnt_is_readonly(nd.mnt))
                res = -EROFS;
Quoted 7 lines

That reminds me of something else I had in mind to debug that the writer counts are okay:

we should probably add a check in permission that we have an elevated writercount on the vfsmount/sb. Of course we'll need some way to overrid it for access(), which means passing down a flag to it or something.

This was already in the second to last patch in the series. Good enough?

fs/namei.c 2007-06-25 11:05:50.000000000 -0700 renamed from fs/namei.c~numa_mnt_want_write 2007-06-25 11:05:50.000000000 -0700 +4 -2
diff -puN fs/namei.c~numa_mnt_want_write fs/namei.c
@@ -230,10 +230,12 @@ int permission(struct inode *inode, int
        int retval, submask;
 
        if (mask & MAY_WRITE) {
-
                /*
-                * Nobody gets write access to a read-only fs.
+                * If this WARN_ON() is hit, it likely means that
+                * there was a missed mnt_want_write() on the path
+                * leading here.
                 */
+               WARN_ON(__mnt_is_readonly(nd->mnt));
                if (IS_RDONLY(inode) &&
 
 

-- Dave

#29 lore raw

Re: [PATCH 09/26] make access() use mnt check

Review

Quoted 12 lines · Dave Hansen
Quoted 1 line · Christoph Hellwig

You probably want to add a big comment explaining why it's fine here.

I've got this in the next set:

(quoted diff) +4 -2
-
-       if(IS_RDONLY(nd.dentry->d_inode))
+       /*
+        * This is a rare case where using __mnt_is_readonly()
+        * is OK without a mnt_want/drop_write() pair.  Since
+        * not actual write to the fs is performed here, we do

s/not/no/

Quoted 8 lines
(quoted diff) +6 -0
+        * not need to telegraph to that to anyone.  Also, we
+        * accept that access is inherently racy, and know that
+        * the fs might be remounted between this syscall, and
+        * any action taken because of its result.
+        */
+       if (__mnt_is_readonly(nd.mnt))
                res = -EROFS;
Signature · 2 lines
David Kleikamp
IBM Linux Technology Center
#30 lore raw

Re: [PATCH 09/26] make access() use mnt check

In reply to Dave Hansen
Quoted 5 lines · Dave Hansen

I've got this in the next set:

(quoted diff) +1 -2
-
-       if(IS_RDONLY(nd.dentry->d_inode))
+       /*

Review

Quoted 10 lines
(quoted diff) +9 -0
+        * This is a rare case where using __mnt_is_readonly()
+        * is OK without a mnt_want/drop_write() pair.  Since
+        * not actual write to the fs is performed here, we do
+        * not need to telegraph to that to anyone.  Also, we
+        * accept that access is inherently racy, and know that
+        * the fs might be remounted between this syscall, and
+        * any action taken because of its result.
+        */
+       if (__mnt_is_readonly(nd.mnt))
                res = -EROFS;

Looks good.

Quoted 6 lines

diff -puN fs/namei.c~numa_mnt_want_write fs/namei.c

fs/namei.c 2007-06-25 11:05:50.000000000 -0700 renamed from fs/namei.c~numa_mnt_want_write 2007-06-25 11:05:50.000000000 -0700 +0 -0
@@ -230,10 +230,12 @@ int permission(struct inode *inode, int
        int retval, submask;
 

fs/namei.c 2007-06-25 11:05:50.000000000 -0700 int permission(struct inode *inode, int · line 239

Quoted 10 lines
fs/namei.c 2007-06-25 11:05:50.000000000 -0700 renamed from fs/namei.c~numa_mnt_want_write 2007-06-25 11:05:50.000000000 -0700 +4 -2
        if (mask & MAY_WRITE) {
-
                /*
-                * Nobody gets write access to a read-only fs.
+                * If this WARN_ON() is hit, it likely means that
+                * there was a missed mnt_want_write() on the path
+                * leading here.
                 */
+               WARN_ON(__mnt_is_readonly(nd->mnt));
                if (IS_RDONLY(inode) &&

I might be missing something, but wouldn't this trip an

	access("/foo/bar", W_OK);

On a readonly filesystem?

#31 lore raw

Re: [PATCH 09/26] make access() use mnt check

Quoted 18 lines · Christoph Hellwig
Quoted 5 lines
fs/namei.c 2007-06-25 11:05:50.000000000 -0700 renamed from fs/namei.c~numa_mnt_want_write 2007-06-25 11:05:50.000000000 -0700 +0 -0
@@ -230,10 +230,12 @@ int permission(struct inode *inode, int
        int retval, submask;
 

fs/namei.c 2007-06-25 11:05:50.000000000 -0700 int permission(struct inode *inode, int · line 239

Quoted 10 lines
fs/namei.c 2007-06-25 11:05:50.000000000 -0700 renamed from fs/namei.c~numa_mnt_want_write 2007-06-25 11:05:50.000000000 -0700 +4 -2
        if (mask & MAY_WRITE) {
-
                /*
-                * Nobody gets write access to a read-only fs.
+                * If this WARN_ON() is hit, it likely means that
+                * there was a missed mnt_want_write() on the path
+                * leading here.
                 */
+               WARN_ON(__mnt_is_readonly(nd->mnt));
                if (IS_RDONLY(inode) &&

I might be missing something, but wouldn't this trip an

Quoted 3 lines
	access("/foo/bar", W_OK);

On a readonly filesystem?

Yes, it will. I will re-work it a bit.

-- Dave

#32 lore raw

PATCH 10/26 elevate mnt writers for callers of vfs_mkdir()

In reply to Dave Hansen

Pretty self-explanatory. Fits in with the rest of the series.

---

 lxc-dave/fs/namei.c            |    5 +++++
 lxc-dave/fs/nfsd/nfs4recover.c |    4 ++++
 2 files changed, 9 insertions(+)
fs/namei.c renamed from fs/namei.c~07-24-elevate-mnt-writers-for-callers-of-vfs-mkdir +5 -0
diff -puN fs/namei.c~07-24-elevate-mnt-writers-for-callers-of-vfs-mkdir fs/namei.c
@@ -1971,7 +1971,12 @@ asmlinkage long sys_mkdirat(int dfd, con
 
 	if (!IS_POSIXACL(nd.dentry->d_inode))
 		mode &= ~current->fs->umask;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto out_dput;
 	error = vfs_mkdir(nd.dentry->d_inode, dentry, mode);
+	mnt_drop_write(nd.mnt);
+out_dput:
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&nd.dentry->d_inode->i_mutex);
fs/nfsd/nfs4recover.c renamed from fs/nfsd/nfs4recover.c~07-24-elevate-mnt-writers-for-callers-of-vfs-mkdir +4 -0
diff -puN fs/nfsd/nfs4recover.c~07-24-elevate-mnt-writers-for-callers-of-vfs-mkdir fs/nfsd/nfs4recover.c
@@ -156,7 +156,11 @@ nfsd4_create_clid_dir(struct nfs4_client
 		dprintk("NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
 		goto out_put;
 	}
+	status = mnt_want_write(rec_dir.mnt);
+	if (status)
+		goto out_put;
 	status = vfs_mkdir(rec_dir.dentry->d_inode, dentry, S_IRWXU);
+	mnt_drop_write(rec_dir.mnt);
 out_put:
 	dput(dentry);
 out_unlock:

_

#33 lore raw

Re: [PATCH 10/26] elevate mnt writers for callers of vfs_mkdir()

Quoted 2 lines · Dave Hansen

Pretty self-explanatory. Fits in with the rest of the series.

Ok for this and similar patch 11-13.

#34 lore raw

PATCH 11/26 elevate write count during entire ncp_ioctl()

In reply to Dave Hansen

Some ioctls need write access, but others don't. Make a helper function to decide when write access is needed, and take it.

---

 lxc-dave/fs/ncpfs/ioctl.c |   55 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)
fs/ncpfs/ioctl.c renamed from fs/ncpfs/ioctl.c~08-24-elevate-write-count-during-entire-ncp-ioctl +54 -1
diff -puN fs/ncpfs/ioctl.c~08-24-elevate-write-count-during-entire-ncp-ioctl fs/ncpfs/ioctl.c
@@ -14,6 +14,7 @@
 #include <linux/ioctl.h>
 #include <linux/time.h>
 #include <linux/mm.h>
+#include <linux/mount.h>
 #include <linux/highuid.h>
 #include <linux/smp_lock.h>
 #include <linux/vmalloc.h>
@@ -260,7 +261,7 @@ ncp_get_charsets(struct ncp_server* serv
 }
 #endif /* CONFIG_NCPFS_NLS */
 
-int ncp_ioctl(struct inode *inode, struct file *filp,
+static int __ncp_ioctl(struct inode *inode, struct file *filp,
 	      unsigned int cmd, unsigned long arg)
 {
 	struct ncp_server *server = NCP_SERVER(inode);
@@ -821,6 +822,58 @@ outrel:			
 	return -EINVAL;
 }
 
+static int ncp_ioctl_need_write(unsigned int cmd)
+{
+	switch (cmd) {
+	case NCP_IOC_GET_FS_INFO:
+	case NCP_IOC_GET_FS_INFO_V2:
+	case NCP_IOC_NCPREQUEST:
+	case NCP_IOC_SETDENTRYTTL:
+	case NCP_IOC_SIGN_INIT:
+	case NCP_IOC_LOCKUNLOCK:
+	case NCP_IOC_SET_SIGN_WANTED:
+		return 1;
+	case NCP_IOC_GETOBJECTNAME:
+	case NCP_IOC_SETOBJECTNAME:
+	case NCP_IOC_GETPRIVATEDATA:
+	case NCP_IOC_SETPRIVATEDATA:
+	case NCP_IOC_SETCHARSETS:
+	case NCP_IOC_GETCHARSETS:
+	case NCP_IOC_CONN_LOGGED_IN:
+	case NCP_IOC_GETDENTRYTTL:
+	case NCP_IOC_GETMOUNTUID2:
+	case NCP_IOC_SIGN_WANTED:
+	case NCP_IOC_GETROOT:
+	case NCP_IOC_SETROOT:
+		return 0;
+	default:
+		/* unkown IOCTL command, assume write */
+		WARN_ON(1);
+	}
+	return 1;
+}
+
+int ncp_ioctl(struct inode *inode, struct file *filp,
+	      unsigned int cmd, unsigned long arg)
+{
+	int ret;
+
+	if (ncp_ioctl_need_write(cmd)) {
+		/*
+		 * inside the ioctl(), any failures which
+		 * are because of file_permission() are
+		 * -EACCESS, so it seems consistent to keep
+		 *  that here.
+		 */
+		if (mnt_want_write(filp->f_vfsmnt))
+			return -EACCES;
+	}
+	ret = __ncp_ioctl(inode, filp, cmd, arg);
+	if (ncp_ioctl_need_write(cmd))
+		mnt_drop_write(filp->f_vfsmnt);
+	return ret;
+}
+
 #ifdef CONFIG_COMPAT
 long ncp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {

_

#35 lore raw

PATCH 12/26 elevate write count for link and symlink calls

In reply to Dave Hansen

---

 lxc-dave/fs/namei.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
fs/namei.c renamed from fs/namei.c~09-24-elevate-write-count-for-link-and-symlink-calls +10 -0
diff -puN fs/namei.c~09-24-elevate-write-count-for-link-and-symlink-calls fs/namei.c
@@ -2244,7 +2244,12 @@ asmlinkage long sys_symlinkat(const char
 	if (IS_ERR(dentry))
 		goto out_unlock;
 
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto out_dput;
 	error = vfs_symlink(nd.dentry->d_inode, dentry, from, S_IALLUGO);
+	mnt_drop_write(nd.mnt);
+out_dput:
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&nd.dentry->d_inode->i_mutex);
@@ -2339,7 +2344,12 @@ asmlinkage long sys_linkat(int olddfd, c
 	error = PTR_ERR(new_dentry);
 	if (IS_ERR(new_dentry))
 		goto out_unlock;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto out_dput;
 	error = vfs_link(old_nd.dentry, nd.dentry->d_inode, new_dentry);
+	mnt_drop_write(nd.mnt);
+out_dput:
 	dput(new_dentry);
 out_unlock:
 	mutex_unlock(&nd.dentry->d_inode->i_mutex);

_

#36 lore raw

PATCH 13/26 elevate mount count for extended attributes

In reply to Dave Hansen

This basically audits the callers of xattr_permission(), which calls permission() and can perform writes to the filesystem.

---

 lxc-dave/fs/nfsd/nfs4proc.c |    7 ++++++-
 lxc-dave/fs/xattr.c         |   16 ++++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)
fs/nfsd/nfs4proc.c renamed from fs/nfsd/nfs4proc.c~10-24-elevate-mount-count-for-extended-attributes +6 -1
diff -puN fs/nfsd/nfs4proc.c~10-24-elevate-mount-count-for-extended-attributes fs/nfsd/nfs4proc.c
@@ -626,14 +626,19 @@ nfsd4_setattr(struct svc_rqst *rqstp, st
 			return status;
 		}
 	}
+	status = mnt_want_write(cstate->current_fh.fh_export->ex_mnt);
+	if (status)
+		return status;
 	status = nfs_ok;
 	if (setattr->sa_acl != NULL)
 		status = nfsd4_set_nfs4_acl(rqstp, &cstate->current_fh,
 					    setattr->sa_acl);
 	if (status)
-		return status;
+		goto out;
 	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
 				0, (time_t)0);
+out:
+	mnt_drop_write(cstate->current_fh.fh_export->ex_mnt);
 	return status;
 }
 
fs/xattr.c renamed from fs/xattr.c~10-24-elevate-mount-count-for-extended-attributes +14 -2
diff -puN fs/xattr.c~10-24-elevate-mount-count-for-extended-attributes fs/xattr.c
@@ -12,6 +12,7 @@
 #include <linux/smp_lock.h>
 #include <linux/file.h>
 #include <linux/xattr.h>
+#include <linux/mount.h>
 #include <linux/namei.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
@@ -33,8 +34,6 @@ xattr_permission(struct inode *inode, co
 	 * filesystem  or on an immutable / append-only inode.
 	 */
 	if (mask & MAY_WRITE) {
-		if (IS_RDONLY(inode))
-			return -EROFS;
 		if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 			return -EPERM;
 	}
@@ -237,7 +236,11 @@ sys_setxattr(char __user *path, char __u
 	error = user_path_walk(path, &nd);
 	if (error)
 		return error;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		return error;
 	error = setxattr(nd.dentry, name, value, size, flags);
+	mnt_drop_write(nd.mnt);
 	path_release(&nd);
 	return error;
 }
@@ -252,7 +255,11 @@ sys_lsetxattr(char __user *path, char __
 	error = user_path_walk_link(path, &nd);
 	if (error)
 		return error;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		return error;
 	error = setxattr(nd.dentry, name, value, size, flags);
+	mnt_drop_write(nd.mnt);
 	path_release(&nd);
 	return error;
 }
@@ -268,9 +275,14 @@ sys_fsetxattr(int fd, char __user *name,
 	f = fget(fd);
 	if (!f)
 		return error;
+	error = mnt_want_write(f->f_vfsmnt);
+	if (error)
+		goto out_fput;
 	dentry = f->f_path.dentry;
 	audit_inode(NULL, dentry->d_inode);
 	error = setxattr(dentry, name, value, size, flags);
+	mnt_drop_write(f->f_vfsmnt);
+out_fput:
 	fput(f);
 	return error;
 }

_

#37 lore raw

PATCH 14/26 elevate write count for file_update_time()

In reply to Dave Hansen

---

 lxc-dave/fs/inode.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/inode.c renamed from fs/inode.c~elevate-write-count-for-file_update_time +6 -1
diff -puN fs/inode.c~elevate-write-count-for-file_update_time fs/inode.c
@@ -1217,10 +1217,13 @@ void file_update_time(struct file *file)
 	struct inode *inode = file->f_path.dentry->d_inode;
 	struct timespec now;
 	int sync_it = 0;
+	int err = 0;
 
 	if (IS_NOCMTIME(inode))
 		return;
-	if (IS_RDONLY(inode))
+	if (file->f_vfsmnt)
+		err = mnt_want_write(file->f_vfsmnt);
+	if (err)
 		return;
 
 	now = current_fs_time(inode->i_sb);
@@ -1236,6 +1239,8 @@ void file_update_time(struct file *file)
 
 	if (sync_it)
 		mark_inode_dirty_sync(inode);
+	if (file->f_vfsmnt)
+		mnt_drop_write(file->f_vfsmnt);
 }
 
 EXPORT_SYMBOL(file_update_time);

_

#38 lore raw

Re: [PATCH 14/26] elevate write count for file_update_time()

Quoted 3 lines · Dave Hansen

We really want a guaranteed non-NULL file here, but I don't want to put this on your plate also. Please add a comment about bloody NFS exports for now.

#39 lore raw

Re: [PATCH 14/26] elevate write count for file_update_time()

Quoted 6 lines · Christoph Hellwig
Quoted 1 line · Dave Hansen

We really want a guaranteed non-NULL file here, but I don't want to put this on your plate also. Please add a comment about bloody NFS exports for now.

How does this look?

- if (IS_RDONLY(inode))
+ /*

+        * Ideally, we want to guarantee that 'f_vfsmnt'
+        * is non-NULL here.  But, NFS exports need to
+        * be fixed up before we can do that.  So, check
+        * it for now. - Dave Hansen
+        */
+       if (file->f_vfsmnt)
+               err = mnt_want_write(file->f_vfsmnt);
+       if (err)
                return;

-- Dave

#40 lore raw

Re: [PATCH 14/26] elevate write count for file_update_time()

Review

Quoted 13 lines · Dave Hansen

How does this look?

(quoted diff) +9 -1
-       if (IS_RDONLY(inode))
+       /*
+        * Ideally, we want to guarantee that 'f_vfsmnt'
+        * is non-NULL here.  But, NFS exports need to
+        * be fixed up before we can do that.  So, check
+        * it for now. - Dave Hansen
+        */
+       if (file->f_vfsmnt)
+               err = mnt_want_write(file->f_vfsmnt);
+       if (err)
                return;

Looks okay. Unfortunately we've grown some more callers of dentry_open with a NULL vfsmount in the meantime (*grasp* reiserfs xattr mess *grasp*)

#41 lore raw

Re: [PATCH 14/26] elevate write count for file_update_time()

In reply to Christoph Hellwig
Quoted 6 lines · Christoph Hellwig
Quoted 1 line · Dave Hansen

We really want a guaranteed non-NULL file here, but I don't want to put this on your plate also. Please add a comment about bloody NFS exports for now.

What's the deal with these? Just that the checks are only for NFS and we shouldn't have to be doing them?

-- Dave

#42 lore raw

PATCH 15/26 mount_is_safe(): add comment

In reply to Dave Hansen

This area of code is currently #ifdef'd out, so add a comment for the time when it is actually used.

---

 lxc-dave/fs/namespace.c |    4 ++++
 1 file changed, 4 insertions(+)
fs/namespace.c renamed from fs/namespace.c~11-24-mount-is-safe-add-comment +4 -0
diff -puN fs/namespace.c~11-24-mount-is-safe-add-comment fs/namespace.c
@@ -691,6 +691,10 @@ static int mount_is_safe(struct nameidat
 		if (current->uid != nd->dentry->d_inode->i_uid)
 			return -EPERM;
 	}
+	/*
+	 * We will eventually check for the mnt->writer_count here,
+	 * but since the code is not used now, skip it - Dave Hansen
+	 */
 	if (vfs_permission(nd, MAY_WRITE))
 		return -EPERM;
 	return 0;

_

#43 lore raw

Re: [PATCH 15/26] mount_is_safe(): add comment

Quoted 4 lines · Dave Hansen

This area of code is currently #ifdef'd out, so add a comment for the time when it is actually used.

Ok. Does this clash with the user mount patches? Even if it does I think we want this patch first in the series and fix the user mounts up ontop of it.

#44 lore raw

Re: [PATCH 15/26] mount_is_safe(): add comment

Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Dave Hansen

This area of code is currently #ifdef'd out, so add a comment for the time when it is actually used.

Ok. Does this clash with the user mount patches? Even if it does I think we want this patch first in the series and fix the user mounts up ontop of it.

It will clash with those, but they completely drops the call to vfs_permission() in here. As I think about this again, we really don't care about r/o bind mounts here, _only_ the vfs permissions.

If it is a r/o bind mount we want to be able to mount something writable on top of it because we're never actually writing to the mount *itself*.

I think this patch can just go away now.

-- Dave

#45 lore raw

PATCH 16/26 unix_find_other() elevate write count for touch_atime()

In reply to Dave Hansen

---

 lxc-dave/net/unix/af_unix.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
net/unix/af_unix.c renamed from net/unix/af_unix.c~12-24-unix-find-other-elevate-write-count-for-touch-atime +12 -4
diff -puN net/unix/af_unix.c~12-24-unix-find-other-elevate-write-count-for-touch-atime net/unix/af_unix.c
@@ -703,21 +703,27 @@ static struct sock *unix_find_other(stru
 		err = path_lookup(sunname->sun_path, LOOKUP_FOLLOW, &nd);
 		if (err)
 			goto fail;
+
+		err = mnt_want_write(nd.mnt);
+		if (err)
+			goto put_path_fail;
+
 		err = vfs_permission(&nd, MAY_WRITE);
 		if (err)
-			goto put_fail;
+			goto mnt_drop_write_fail;
 
 		err = -ECONNREFUSED;
 		if (!S_ISSOCK(nd.dentry->d_inode->i_mode))
-			goto put_fail;
+			goto mnt_drop_write_fail;
 		u=unix_find_socket_byinode(nd.dentry->d_inode);
 		if (!u)
-			goto put_fail;
+			goto mnt_drop_write_fail;
 
 		if (u->sk_type == type)
 			touch_atime(nd.mnt, nd.dentry);
 
 		path_release(&nd);
+		mnt_drop_write(nd.mnt);
 
 		err=-EPROTOTYPE;
 		if (u->sk_type != type) {
@@ -737,7 +743,9 @@ static struct sock *unix_find_other(stru
 	}
 	return u;
 
-put_fail:
+mnt_drop_write_fail:
+	mnt_drop_write(nd.mnt);
+put_path_fail:
 	path_release(&nd);
 fail:
 	*error=err;

_

#47 lore raw

PATCH 17/26 elevate write count over calls to vfs_rename()

In reply to Dave Hansen

This also creates a little helper in the NFS code to make an if() a little bit less ugly.

---

 lxc-dave/fs/namei.c    |    4 ++++
 lxc-dave/fs/nfsd/vfs.c |   23 +++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)
fs/namei.c renamed from fs/namei.c~13-24-elevate-write-count-over-calls-to-vfs-rename +4 -0
diff -puN fs/namei.c~13-24-elevate-write-count-over-calls-to-vfs-rename fs/namei.c
@@ -2575,8 +2575,12 @@ static int do_rename(int olddfd, const c
 	if (new_dentry == trap)
 		goto exit5;
 
+	error = mnt_want_write(oldnd.mnt);
+	if (error)
+		goto exit5;
 	error = vfs_rename(old_dir->d_inode, old_dentry,
 				   new_dir->d_inode, new_dentry);
+	mnt_drop_write(oldnd.mnt);
 exit5:
 	dput(new_dentry);
 exit4:
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~13-24-elevate-write-count-over-calls-to-vfs-rename +19 -4
diff -puN fs/nfsd/vfs.c~13-24-elevate-write-count-over-calls-to-vfs-rename fs/nfsd/vfs.c
@@ -1554,6 +1554,14 @@ out_nfserr:
 	goto out_unlock;
 }
 
+static inline int svc_msnfs(struct svc_fh *ffhp)
+{
+#ifdef MSNFS
+	return (ffhp->fh_export->ex_flags & NFSEXP_MSNFS);
+#else
+	return 0;
+#endif
+}
 /*
  * Rename a file
  * N.B. After this call _both_ ffhp and tfhp need an fh_put
@@ -1615,13 +1623,20 @@ nfsd_rename(struct svc_rqst *rqstp, stru
 	if (ndentry == trap)
 		goto out_dput_new;
 
-#ifdef MSNFS
-	if ((ffhp->fh_export->ex_flags & NFSEXP_MSNFS) &&
+	if (svc_msnfs(ffhp) &&
 		((atomic_read(&odentry->d_count) > 1)
 		 || (atomic_read(&ndentry->d_count) > 1))) {
 			host_err = -EPERM;
-	} else
-#endif
+			goto out_dput_new;
+	}
+
+	host_err = -EXDEV;
+	if (ffhp->fh_export->ex_mnt != tfhp->fh_export->ex_mnt)
+		goto out_dput_new;
+	host_err = mnt_want_write(ffhp->fh_export->ex_mnt);
+	if (host_err)
+		goto out_dput_new;
+
 	host_err = vfs_rename(fdir, odentry, tdir, ndentry);
 	if (!host_err && EX_ISSYNC(tfhp->fh_export)) {
 		host_err = nfsd_sync_dir(tdentry);

_

#48 lore raw

Re: [PATCH 17/26] elevate write count over calls to vfs_rename()

Quoted 3 lines · Dave Hansen

This also creates a little helper in the NFS code to make an if() a little bit less ugly.

That should probably be a separate patch. Or better one to just rip out the MSNFS ifdefs completely, they've always been true for about the last ten years.

#49 lore raw

PATCH 18/26 nfs: check mnt instead of superblock directly

In reply to Dave Hansen

---

 lxc-dave/fs/nfs/dir.c  |    3 ++-
 lxc-dave/fs/nfsd/vfs.c |    4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)
fs/nfs/dir.c renamed from fs/nfs/dir.c~nfs-check-mnt-instead-of-sb +2 -1
diff -puN fs/nfs/dir.c~nfs-check-mnt-instead-of-sb fs/nfs/dir.c
@@ -981,7 +981,8 @@ static int is_atomic_open(struct inode *
 	if (nd->flags & LOOKUP_DIRECTORY)
 		return 0;
 	/* Are we trying to write to a read only partition? */
-	if (IS_RDONLY(dir) && (nd->intent.open.flags & (O_CREAT|O_TRUNC|FMODE_WRITE)))
+	if (__mnt_is_readonly(nd->mnt) &&
+	    (nd->intent.open.flags & (O_CREAT|O_TRUNC|FMODE_WRITE)))
 		return 0;
 	return 1;
 }
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~nfs-check-mnt-instead-of-sb +2 -2
diff -puN fs/nfsd/vfs.c~nfs-check-mnt-instead-of-sb fs/nfsd/vfs.c
@@ -1811,7 +1811,7 @@ nfsd_permission(struct svc_export *exp, 
 		inode->i_mode,
 		IS_IMMUTABLE(inode)?	" immut" : "",
 		IS_APPEND(inode)?	" append" : "",
-		IS_RDONLY(inode)?	" ro" : "");
+		__mnt_is_readonly(exp->mnt)?	" ro" : "");
 	dprintk("      owner %d/%d user %d/%d\n",
 		inode->i_uid, inode->i_gid, current->fsuid, current->fsgid);
 #endif
@@ -1822,7 +1822,7 @@ nfsd_permission(struct svc_export *exp, 
 	 */
 	if (!(acc & MAY_LOCAL_ACCESS))
 		if (acc & (MAY_WRITE | MAY_SATTR | MAY_TRUNC)) {
-			if (EX_RDONLY(exp) || IS_RDONLY(inode))
+			if (EX_RDONLY(exp) || __mnt_is_readonly(exp->mnt))
 				return nfserr_rofs;
 			if (/* (acc & MAY_WRITE) && */ IS_IMMUTABLE(inode))
 				return nfserr_perm;

_

#51 lore raw

PATCH 19/26 elevate writer count for do_sys_truncate()

In reply to Dave Hansen

---

 lxc-dave/fs/open.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
fs/open.c renamed from fs/open.c~15-24-elevate-writer-count-for-do-sys-truncate +9 -7
diff -puN fs/open.c~15-24-elevate-writer-count-for-do-sys-truncate fs/open.c
@@ -241,28 +241,28 @@ static long do_sys_truncate(const char _
 	if (!S_ISREG(inode->i_mode))
 		goto dput_and_out;
 
-	error = vfs_permission(&nd, MAY_WRITE);
+	error = mnt_want_write(nd.mnt);
 	if (error)
 		goto dput_and_out;
 
-	error = -EROFS;
-	if (IS_RDONLY(inode))
-		goto dput_and_out;
+	error = vfs_permission(&nd, MAY_WRITE);
+	if (error)
+		goto mnt_drop_write_and_out;
 
 	error = -EPERM;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-		goto dput_and_out;
+		goto mnt_drop_write_and_out;
 
 	/*
 	 * Make sure that there are no leases.
 	 */
 	error = break_lease(inode, FMODE_WRITE);
 	if (error)
-		goto dput_and_out;
+		goto mnt_drop_write_and_out;
 
 	error = get_write_access(inode);
 	if (error)
-		goto dput_and_out;
+		goto mnt_drop_write_and_out;
 
 	error = locks_verify_truncate(inode, NULL, length);
 	if (!error) {
@@ -271,6 +271,8 @@ static long do_sys_truncate(const char _
 	}
 	put_write_access(inode);
 
+mnt_drop_write_and_out:
+	mnt_drop_write(nd.mnt);
 dput_and_out:
 	path_release(&nd);
 out:

_

#53 lore raw

PATCH 20/26 elevate write count for do_utimes()

In reply to Dave Hansen

---

 lxc-dave/fs/utimes.c |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)
fs/utimes.c renamed from fs/utimes.c~16-24-elevate-write-count-for-do-utimes +16 -11
diff -puN fs/utimes.c~16-24-elevate-write-count-for-do-utimes fs/utimes.c
@@ -1,6 +1,7 @@
 #include <linux/compiler.h>
 #include <linux/fs.h>
 #include <linux/linkage.h>
+#include <linux/mount.h>
 #include <linux/namei.h>
 #include <linux/sched.h>
 #include <linux/utime.h>
@@ -32,8 +33,8 @@ asmlinkage long sys_utime(char __user * 
 		goto out;
 	inode = nd.dentry->d_inode;
 
-	error = -EROFS;
-	if (IS_RDONLY(inode))
+	error = mnt_want_write(nd.mnt);
+	if (error)
 		goto dput_and_out;
 
 	/* Don't worry, the checks are done in inode_change_ok() */
@@ -41,7 +42,7 @@ asmlinkage long sys_utime(char __user * 
 	if (times) {
 		error = -EPERM;
 		if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
-			goto dput_and_out;
+			goto mnt_drop_write_and_out;
 
 		error = get_user(newattrs.ia_atime.tv_sec, &times->actime);
 		newattrs.ia_atime.tv_nsec = 0;
@@ -49,21 +50,23 @@ asmlinkage long sys_utime(char __user * 
 			error = get_user(newattrs.ia_mtime.tv_sec, &times->modtime);
 		newattrs.ia_mtime.tv_nsec = 0;
 		if (error)
-			goto dput_and_out;
+			goto mnt_drop_write_and_out;
 
 		newattrs.ia_valid |= ATTR_ATIME_SET | ATTR_MTIME_SET;
 	} else {
                 error = -EACCES;
                 if (IS_IMMUTABLE(inode))
-                        goto dput_and_out;
+			goto mnt_drop_write_and_out;
 
 		if (current->fsuid != inode->i_uid &&
 		    (error = vfs_permission(&nd, MAY_WRITE)) != 0)
-			goto dput_and_out;
+			goto mnt_drop_write_and_out;
 	}
 	mutex_lock(&inode->i_mutex);
 	error = notify_change(nd.dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
+mnt_drop_write_and_out:
+	mnt_drop_write(nd.mnt);
 dput_and_out:
 	path_release(&nd);
 out:
@@ -89,8 +92,8 @@ long do_utimes(int dfd, char __user *fil
 		goto out;
 	inode = nd.dentry->d_inode;
 
-	error = -EROFS;
-	if (IS_RDONLY(inode))
+	error = mnt_want_write(nd.mnt);
+	if (error)
 		goto dput_and_out;
 
 	/* Don't worry, the checks are done in inode_change_ok() */
@@ -98,7 +101,7 @@ long do_utimes(int dfd, char __user *fil
 	if (times) {
 		error = -EPERM;
                 if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
-                        goto dput_and_out;
+			goto mnt_drop_write_and_out;
 
 		newattrs.ia_atime.tv_sec = times[0].tv_sec;
 		newattrs.ia_atime.tv_nsec = times[0].tv_usec * 1000;
@@ -108,15 +111,17 @@ long do_utimes(int dfd, char __user *fil
 	} else {
 		error = -EACCES;
                 if (IS_IMMUTABLE(inode))
-                        goto dput_and_out;
+			goto mnt_drop_write_and_out;
 
 		if (current->fsuid != inode->i_uid &&
 		    (error = vfs_permission(&nd, MAY_WRITE)) != 0)
-			goto dput_and_out;
+			goto mnt_drop_write_and_out;
 	}
 	mutex_lock(&inode->i_mutex);
 	error = notify_change(nd.dentry, &newattrs);
 	mutex_unlock(&inode->i_mutex);
+mnt_drop_write_and_out:
+	mnt_drop_write(nd.mnt);
 dput_and_out:
 	path_release(&nd);
 out:

_

#55 lore raw

PATCH 21/26 elevate write count for do_sys_utime() and touch_atime()

In reply to Dave Hansen

---

 lxc-dave/fs/inode.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
fs/inode.c renamed from fs/inode.c~17-24-elevate-write-count-for-do-sys-utime-and-touch-atime +12 -8
diff -puN fs/inode.c~17-24-elevate-write-count-for-do-sys-utime-and-touch-atime fs/inode.c
@@ -1161,22 +1161,23 @@ void touch_atime(struct vfsmount *mnt, s
 	struct inode *inode = dentry->d_inode;
 	struct timespec now;
 
-	if (inode->i_flags & S_NOATIME)
+	if (mnt && mnt_want_write(mnt))
 		return;
+	if (inode->i_flags & S_NOATIME)
+		goto out;
 	if (IS_NOATIME(inode))
-		return;
+		goto out;
 	if ((inode->i_sb->s_flags & MS_NODIRATIME) && S_ISDIR(inode->i_mode))
-		return;
+		goto out;
 
 	/*
 	 * We may have a NULL vfsmount when coming from NFSD
 	 */
 	if (mnt) {
 		if (mnt->mnt_flags & MNT_NOATIME)
-			return;
+			goto out;
 		if ((mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode))
-			return;
-
+			goto out;
 		if (mnt->mnt_flags & MNT_RELATIME) {
 			/*
 			 * With relative atime, only update atime if the
@@ -1187,16 +1188,19 @@ void touch_atime(struct vfsmount *mnt, s
 						&inode->i_atime) < 0 &&
 			    timespec_compare(&inode->i_ctime,
 						&inode->i_atime) < 0)
-				return;
+				goto out;
 		}
 	}
 
 	now = current_fs_time(inode->i_sb);
 	if (timespec_equal(&inode->i_atime, &now))
-		return;
+		goto out;
 
 	inode->i_atime = now;
 	mark_inode_dirty_sync(inode);
+out:
+	if (mnt)
+		mnt_drop_write(mnt);
 }
 EXPORT_SYMBOL(touch_atime);
 

_

#57 lore raw

PATCH 22/26 sys_mknodat(): elevate write count for vfs_mknod/create()

In reply to Dave Hansen

This takes care of all of the direct callers of vfs_mknod(). Since a few of these cases also handle normal file creation as well, this also covers some calls to vfs_create().

---

 lxc-dave/fs/namei.c         |   12 ++++++++++++
 lxc-dave/fs/nfsd/vfs.c      |    8 ++++++--
 lxc-dave/net/unix/af_unix.c |    4 ++++
 3 files changed, 22 insertions(+), 2 deletions(-)
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +12 -0
diff -puN fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create fs/namei.c
@@ -1897,14 +1897,26 @@ asmlinkage long sys_mknodat(int dfd, con
 	if (!IS_ERR(dentry)) {
 		switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
+			error = mnt_want_write(nd.mnt);
+			if (error)
+				break;
 			error = vfs_create(nd.dentry->d_inode,dentry,mode,&nd);
+			mnt_drop_write(nd.mnt);
 			break;
 		case S_IFCHR: case S_IFBLK:
+			error = mnt_want_write(nd.mnt);
+			if (error)
+				break;
 			error = vfs_mknod(nd.dentry->d_inode,dentry,mode,
 					new_decode_dev(dev));
+			mnt_drop_write(nd.mnt);
 			break;
 		case S_IFIFO: case S_IFSOCK:
+			error = mnt_want_write(nd.mnt);
+			if (error)
+				break;
 			error = vfs_mknod(nd.dentry->d_inode,dentry,mode,0);
+			mnt_drop_write(nd.mnt);
 			break;
 		case S_IFDIR:
 			error = -EPERM;
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +6 -2
diff -puN fs/nfsd/vfs.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create fs/nfsd/vfs.c
@@ -1192,7 +1192,11 @@ nfsd_create(struct svc_rqst *rqstp, stru
 	case S_IFBLK:
 	case S_IFIFO:
 	case S_IFSOCK:
+		host_err = mnt_want_write(fhp->fh_export->ex_mnt);
+		if (host_err)
+			break;
 		host_err = vfs_mknod(dirp, dchild, iap->ia_mode, rdev);
+		mnt_drop_write(fhp->fh_export->ex_mnt);
 		break;
 	default:
 	        printk("nfsd: bad file type %o in nfsd_create\n", type);
@@ -1811,7 +1815,7 @@ nfsd_permission(struct svc_export *exp, 
 		inode->i_mode,
 		IS_IMMUTABLE(inode)?	" immut" : "",
 		IS_APPEND(inode)?	" append" : "",
-		__mnt_is_readonly(exp->mnt)?	" ro" : "");
+		__mnt_is_readonly(exp->ex_mnt)?	" ro" : "");
 	dprintk("      owner %d/%d user %d/%d\n",
 		inode->i_uid, inode->i_gid, current->fsuid, current->fsgid);
 #endif
@@ -1822,7 +1826,7 @@ nfsd_permission(struct svc_export *exp, 
 	 */
 	if (!(acc & MAY_LOCAL_ACCESS))
 		if (acc & (MAY_WRITE | MAY_SATTR | MAY_TRUNC)) {
-			if (EX_RDONLY(exp) || __mnt_is_readonly(exp->mnt))
+			if (EX_RDONLY(exp) || __mnt_is_readonly(exp->ex_mnt))
 				return nfserr_rofs;
 			if (/* (acc & MAY_WRITE) && */ IS_IMMUTABLE(inode))
 				return nfserr_perm;
net/unix/af_unix.c renamed from net/unix/af_unix.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +4 -0
diff -puN net/unix/af_unix.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create net/unix/af_unix.c
@@ -816,7 +816,11 @@ static int unix_bind(struct socket *sock
 		 */
 		mode = S_IFSOCK |
 		       (SOCK_INODE(sock)->i_mode & ~current->fs->umask);
+		err = mnt_want_write(nd.mnt);
+		if (err)
+			goto out_mknod_dput;
 		err = vfs_mknod(nd.dentry->d_inode, dentry, mode, 0);
+		mnt_drop_write(nd.mnt);
 		if (err)
 			goto out_mknod_dput;
 		mutex_unlock(&nd.dentry->d_inode->i_mutex);

_

#58 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

Quoted 5 lines · Dave Hansen

This takes care of all of the direct callers of vfs_mknod(). Since a few of these cases also handle normal file creation as well, this also covers some calls to vfs_create().

Ok.

Quoted 20 lines

diff -puN fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create fs/namei.c

fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +8 -0
@@ -1897,14 +1897,26 @@ asmlinkage long sys_mknodat(int dfd, con
 	if (!IS_ERR(dentry)) {
 		switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
+			error = mnt_want_write(nd.mnt);
+			if (error)
+				break;
 			error = vfs_create(nd.dentry->d_inode,dentry,mode,&nd);
+			mnt_drop_write(nd.mnt);
 			break;
 		case S_IFCHR: case S_IFBLK:
+			error = mnt_want_write(nd.mnt);
+			if (error)
+				break;
 			error = vfs_mknod(nd.dentry->d_inode,dentry,mode,
 					new_decode_dev(dev));
+			mnt_drop_write(nd.mnt);

fs/namei.c asmlinkage long sys_mknodat(int dfd, con · line 1922

Quoted 10 lines
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +4 -0
 			break;
 		case S_IFIFO: case S_IFSOCK:
+			error = mnt_want_write(nd.mnt);
+			if (error)
+				break;
 			error = vfs_mknod(nd.dentry->d_inode,dentry,mode,0);
+			mnt_drop_write(nd.mnt);
 			break;
 		case S_IFDIR:
 			error = -EPERM;

Should we just take the calls outside the switch statement?

#59 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

Quoted 24 lines · Christoph Hellwig
Quoted 24 lines

diff -puN fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create fs/namei.c

fs/namei.c 2007-06-21 23:23:25.000000000 -0700 renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create 2007-06-21 23:23:25.000000000 -0700 +10 -0
@@ -1897,14 +1897,26 @@ asmlinkage long sys_mknodat(int dfd, con
      if (!IS_ERR(dentry)) {
              switch (mode & S_IFMT) {
              case 0: case S_IFREG:
+                     error = mnt_want_write(nd.mnt);
+                     if (error)
+                             break;
                      error = vfs_create(nd.dentry->d_inode,dentry,mode,&nd);
+                     mnt_drop_write(nd.mnt);
                      break;
              case S_IFCHR: case S_IFBLK:
+                     error = mnt_want_write(nd.mnt);
+                     if (error)
+                             break;
                      error = vfs_mknod(nd.dentry->d_inode,dentry,mode,
                                      new_decode_dev(dev));
+                     mnt_drop_write(nd.mnt);
                      break;
              case S_IFIFO: case S_IFSOCK:
+                     error = mnt_want_write(nd.mnt);
+                     if (error)
Quoted 8 lines

fs/namei.c 2007-06-21 23:23:25.000000000 -0700 asmlinkage long sys_mknodat(int dfd, con · line 1922

Quoted 6 lines
fs/namei.c 2007-06-21 23:23:25.000000000 -0700 renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create 2007-06-21 23:23:25.000000000 -0700 +2 -0
+                             break;
                      error = vfs_mknod(nd.dentry->d_inode,dentry,mode,0);
+                     mnt_drop_write(nd.mnt);
                      break;
              case S_IFDIR:
                      error = -EPERM;

Should we just take the calls outside the switch statement?

Yeah, that's much better. I assume we don't care whether we're getting -EROFS or -EPERM/-EINVAL for the S_IFDIR and default cases?

-- Dave

#60 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

Quoted 4 lines · Dave Hansen
Quoted 1 line

Should we just take the calls outside the switch statement?

Yeah, that's much better. I assume we don't care whether we're getting -EROFS or -EPERM/-EINVAL for the S_IFDIR and default cases?

We need to keep the exact error returns, so you'll have to add some special case checks before the r/o check. It's probably still cleaner, though.

#61 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Dave Hansen
Quoted 1 line

Should we just take the calls outside the switch statement?

Yeah, that's much better. I assume we don't care whether we're getting -EROFS or -EPERM/-EINVAL for the S_IFDIR and default cases?

We need to keep the exact error returns, so you'll have to add some special case checks before the r/o check. It's probably still cleaner, though.

I'm looking through this and having a really hard time seeing any nice ways to make this cleaner.

Were you thinking of basically a duplicate case statement before the r/w check?

-- Dave

#62 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

In reply to Christoph Hellwig
Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Dave Hansen
Quoted 1 line

Should we just take the calls outside the switch statement?

Yeah, that's much better. I assume we don't care whether we're getting -EROFS or -EPERM/-EINVAL for the S_IFDIR and default cases?

We need to keep the exact error returns, so you'll have to add some special case checks before the r/o check. It's probably still cleaner, though.

How does this (untested) patch look?

--

This takes care of all of the direct callers of vfs_mknod(). Since a few of these cases also handle normal file creation as well, this also covers some calls to vfs_create().

So that we don't have to make three mnt_want/drop_write() calls inside of the switch statement, we move some of its logic outside of the switch.

One thing I noticed: do we actually _need_ the first S_ISDIR() check at the top of the function?

---

 lxc-dave/fs/namei.c         |   29 ++++++++++++++++++++---------
 lxc-dave/fs/nfsd/vfs.c      |    8 ++++++--
 lxc-dave/net/unix/af_unix.c |    4 ++++
 3 files changed, 30 insertions(+), 11 deletions(-)
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +20 -9
diff -puN fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create fs/namei.c
@@ -1911,13 +1911,27 @@ asmlinkage long sys_mknodat(int dfd, con
 	error = do_path_lookup(dfd, tmp, LOOKUP_PARENT, &nd);
 	if (error)
 		goto out;
+
 	dentry = lookup_create(&nd, 0);
 	error = PTR_ERR(dentry);
+	if (error)
+		goto out_unlock;
 
 	if (!IS_POSIXACL(nd.dentry->d_inode))
 		mode &= ~current->fs->umask;
-	if (!IS_ERR(dentry)) {
-		switch (mode & S_IFMT) {
+	if (S_ISDIR(mode)) {
+		error = -EPERM;
+		goto out_dput;
+	}
+	if (!S_ISREG(mode)  && !S_ISCHR(mode) && !S_ISBLK(mode) &&
+	    !S_ISFIFO(mode) && !S_ISSOCK(mode) && (mode != 0)) {
+		error = -EINVAL;
+		goto out_dput;
+	}
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto out_dput;
+	switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
 			error = vfs_create(nd.dentry->d_inode,dentry,mode,&nd);
 			break;
@@ -1928,14 +1942,11 @@ asmlinkage long sys_mknodat(int dfd, con
 		case S_IFIFO: case S_IFSOCK:
 			error = vfs_mknod(nd.dentry->d_inode,dentry,mode,0);
 			break;
-		case S_IFDIR:
-			error = -EPERM;
-			break;
-		default:
-			error = -EINVAL;
-		}
-		dput(dentry);
 	}
+	mnt_drop_write(nd.mnt);
+out_dput:
+	dput(dentry);
+out_unlock:
 	mutex_unlock(&nd.dentry->d_inode->i_mutex);
 	path_release(&nd);
 out:
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +6 -2
diff -puN fs/nfsd/vfs.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create fs/nfsd/vfs.c
@@ -1199,7 +1199,11 @@ nfsd_create(struct svc_rqst *rqstp, stru
 	case S_IFBLK:
 	case S_IFIFO:
 	case S_IFSOCK:
+		host_err = mnt_want_write(fhp->fh_export->ex_mnt);
+		if (host_err)
+			break;
 		host_err = vfs_mknod(dirp, dchild, iap->ia_mode, rdev);
+		mnt_drop_write(fhp->fh_export->ex_mnt);
 		break;
 	default:
 	        printk("nfsd: bad file type %o in nfsd_create\n", type);
@@ -1810,7 +1814,7 @@ nfsd_permission(struct svc_export *exp, 
 		inode->i_mode,
 		IS_IMMUTABLE(inode)?	" immut" : "",
 		IS_APPEND(inode)?	" append" : "",
-		__mnt_is_readonly(exp->mnt)?	" ro" : "");
+		__mnt_is_readonly(exp->ex_mnt)?	" ro" : "");
 	dprintk("      owner %d/%d user %d/%d\n",
 		inode->i_uid, inode->i_gid, current->fsuid, current->fsgid);
 #endif
@@ -1821,7 +1825,7 @@ nfsd_permission(struct svc_export *exp, 
 	 */
 	if (!(acc & MAY_LOCAL_ACCESS))
 		if (acc & (MAY_WRITE | MAY_SATTR | MAY_TRUNC)) {
-			if (EX_RDONLY(exp) || __mnt_is_readonly(exp->mnt))
+			if (EX_RDONLY(exp) || __mnt_is_readonly(exp->ex_mnt))
 				return nfserr_rofs;
 			if (/* (acc & MAY_WRITE) && */ IS_IMMUTABLE(inode))
 				return nfserr_perm;
net/unix/af_unix.c renamed from net/unix/af_unix.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +4 -0
diff -puN net/unix/af_unix.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create net/unix/af_unix.c
@@ -815,7 +815,11 @@ static int unix_bind(struct socket *sock
 		 */
 		mode = S_IFSOCK |
 		       (SOCK_INODE(sock)->i_mode & ~current->fs->umask);
+		err = mnt_want_write(nd.mnt);
+		if (err)
+			goto out_mknod_dput;
 		err = vfs_mknod(nd.dentry->d_inode, dentry, mode, 0);
+		mnt_drop_write(nd.mnt);
 		if (err)
 			goto out_mknod_dput;
 		mutex_unlock(&nd.dentry->d_inode->i_mutex);

_

#63 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

Quoted 7 lines · Dave Hansen
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +1 -0
@@ -1911,13 +1911,27 @@ asmlinkage long sys_mknodat(int dfd, con
 	error = do_path_lookup(dfd, tmp, LOOKUP_PARENT, &nd);
 	if (error)
 		goto out;
+
 	dentry = lookup_create(&nd, 0);
 	error = PTR_ERR(dentry);

fs/namei.c asmlinkage long sys_mknodat(int dfd, con · line 1924

Quoted 10 lines
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +5 -2
+	if (error)
+		goto out_unlock;
 
 	if (!IS_POSIXACL(nd.dentry->d_inode))
 		mode &= ~current->fs->umask;
-	if (!IS_ERR(dentry)) {
-		switch (mode & S_IFMT) {
+	if (S_ISDIR(mode)) {
+		error = -EPERM;
+		goto out_dput;

Hm, I wonder why mknod(,S_IFDIR,) returns -EPERM rather than -EINVAL like the others? It is a bit misleading; EPERM usually means root could do it somehow (unless the kernel is really pedantic).
IMHO this if(S_ISDIR) should go.

fs/namei.c asmlinkage long sys_mknodat(int dfd, con · line 1927

Quoted 2 lines
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +2 -0
+	if (!S_ISREG(mode)  && !S_ISCHR(mode) && !S_ISBLK(mode) &&
+	    !S_ISFIFO(mode) && !S_ISSOCK(mode) && (mode != 0)) {

If you align the two &&s (^) you can also align ^ the others :)

fs/namei.c asmlinkage long sys_mknodat(int dfd, con · line 1930

Quoted 3 lines
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +3 -0
+		error = -EINVAL;
+		goto out_dput;
+	}

Jan

#64 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

Quoted 12 lines · Jan Engelhardt
Quoted 10 lines · Dave Hansen
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +3 -0
@@ -1911,13 +1911,27 @@ asmlinkage long sys_mknodat(int dfd, con
 	error = do_path_lookup(dfd, tmp, LOOKUP_PARENT, &nd);
 	if (error)
 		goto out;
+
 	dentry = lookup_create(&nd, 0);
 	error = PTR_ERR(dentry);
+	if (error)
+		goto out_unlock;
 
Quoted 12 lines

fs/namei.c asmlinkage long sys_mknodat(int dfd, con · line 1924

Quoted 7 lines
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +3 -2
 	if (!IS_POSIXACL(nd.dentry->d_inode))
 		mode &= ~current->fs->umask;
-	if (!IS_ERR(dentry)) {
-		switch (mode & S_IFMT) {
+	if (S_ISDIR(mode)) {
+		error = -EPERM;
+		goto out_dput;

Hm, I wonder why mknod(,S_IFDIR,) returns -EPERM rather than -EINVAL like the others? It is a bit misleading; EPERM usually means root could do it somehow (unless the kernel is really pedantic).
IMHO this if(S_ISDIR) should go.

I'd prefer to leave that fight for another day. :) It'd be easy to fix up later.

Quoted 4 lines

fs/namei.c asmlinkage long sys_mknodat(int dfd, con · line 1927

Quoted 2 lines
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +2 -0
+	if (!S_ISREG(mode)  && !S_ISCHR(mode) && !S_ISBLK(mode) &&
+	    !S_ISFIFO(mode) && !S_ISSOCK(mode) && (mode != 0)) {

If you align the two &&s (^) you can also align ^ the others :)

Good point. I'll fix it up.

#65 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

In reply to Jan Engelhardt
Quoted 2 lines · Jan Engelhardt

Hm, I wonder why mknod(,S_IFDIR,) returns -EPERM rather than -EINVAL like the others?

Posix.

#66 lore raw

Re: [PATCH 22/26] sys_mknodat(): elevate write count for vfs_mknod/create()

In reply to Dave Hansen
Quoted 7 lines · Dave Hansen

This takes care of all of the direct callers of vfs_mknod(). Since a few of these cases also handle normal file creation as well, this also covers some calls to vfs_create().

So that we don't have to make three mnt_want/drop_write() calls inside of the switch statement, we move some of its logic outside of the switch.

Looks good to me.

Quoted 2 lines

One thing I noticed: do we actually _need_ the first S_ISDIR() check at the top of the function?

The EPERM is required by Posix.

fs/namei.c asmlinkage long sys_mknodat(int dfd, con · line 1927

Quoted 2 lines
fs/namei.c renamed from fs/namei.c~18-24-sys-mknodat-elevate-write-count-for-vfs-mknod-create +2 -0
+	if (!S_ISREG(mode)  && !S_ISCHR(mode) && !S_ISBLK(mode) &&
+	    !S_ISFIFO(mode) && !S_ISSOCK(mode) && (mode != 0)) {

no need for braces around the mode != 0

#67 lore raw

PATCH 23/26 elevate mnt writers for vfs_unlink() callers

In reply to Dave Hansen

---

 lxc-dave/fs/namei.c   |    4 ++++
 lxc-dave/ipc/mqueue.c |    5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)
fs/namei.c renamed from fs/namei.c~19-24-elevate-mnt-writers-for-vfs-unlink-callers +4 -0
diff -puN fs/namei.c~19-24-elevate-mnt-writers-for-vfs-unlink-callers fs/namei.c
@@ -2175,7 +2175,11 @@ static long do_unlinkat(int dfd, const c
 		inode = dentry->d_inode;
 		if (inode)
 			atomic_inc(&inode->i_count);
+		error = mnt_want_write(nd.mnt);
+		if (error)
+			goto exit2;
 		error = vfs_unlink(nd.dentry->d_inode, dentry);
+		mnt_drop_write(nd.mnt);
 	exit2:
 		dput(dentry);
 	}
ipc/mqueue.c renamed from ipc/mqueue.c~19-24-elevate-mnt-writers-for-vfs-unlink-callers +4 -1
diff -puN ipc/mqueue.c~19-24-elevate-mnt-writers-for-vfs-unlink-callers ipc/mqueue.c
@@ -750,8 +750,11 @@ asmlinkage long sys_mq_unlink(const char
 	inode = dentry->d_inode;
 	if (inode)
 		atomic_inc(&inode->i_count);
-
+	err = mnt_want_write(mqueue_mnt);
+	if (err)
+		goto out_err;
 	err = vfs_unlink(dentry->d_parent->d_inode, dentry);
+	mnt_drop_write(mqueue_mnt);
 out_err:
 	dput(dentry);
 

_

#69 lore raw

PATCH 24/26 do_rmdir(): elevate write count

In reply to Dave Hansen

Elevate the write count during the vfs_rmdir() call.

---

 lxc-dave/fs/namei.c |    5 +++++
 1 file changed, 5 insertions(+)
fs/namei.c renamed from fs/namei.c~20-24-do-rmdir-elevate-write-count +5 -0
diff -puN fs/namei.c~20-24-do-rmdir-elevate-write-count fs/namei.c
@@ -2095,7 +2095,12 @@ static long do_rmdir(int dfd, const char
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto exit2;
+	error = mnt_want_write(nd.mnt);
+	if (error)
+		goto exit3;
 	error = vfs_rmdir(nd.dentry->d_inode, dentry);
+	mnt_drop_write(nd.mnt);
+exit3:
 	dput(dentry);
 exit2:
 	mutex_unlock(&nd.dentry->d_inode->i_mutex);

_

#71 lore raw

PATCH 25/26 r/o bind mounts: scalable writer count

In reply to Dave Hansen

This uses a statically-allocated percpu variable. All operations are local to a cpu as long that cpu operates on the same mount, and there are no writer count imbalances. Writer count imbalances happen when a write is taken on one cpu, and released on another, like when an open/close pair is performed on two different cpus because the task moved.

I've written a little benchmark to sit in a loop for a couple of seconds in several cpus in parallel doing open/write/close loops.

http://sr71.net/~dave/linux/openbench.c

The code in here is a a worst-possible case for this patch. It does opens on a _pair_ of files in two different mounts in parallel. This should cause my code to lose its "operate on the same mount" optimization completely. This worst-case scenario causes a 3% degredation in the benchmark.

I could probably get rid of even this 3%, but it would be more complex than what I have here, and I think this is getting into acceptable territory. In practice, I expect writing more than 3 bytes to a file, as well as disk I/O to mask any effects that this has.

---

A little history:

These patches used to use a single atomic_t in each vfsmount to track the number of writers to the mount at any given time. Open a file for write anywhere on the mount, and you increment the atomic_t.

This didn't scale on NUMA or SMP. It caused something like a 4x slowdown in open/write/close operations. It bounced cachelines around like mad. I tried out a new system with a per-node spinlock and atomic in each vfsmount to replace the old single atomic. It worked _much_ better on NUMA, but still caused a ~6% regression on the same open/write/close operation set on a normal 4-way SMP machine, because it was still contended inside of a node.

We could generalize this lock, but this is an awfully specialized situation, and I'd be worried that people would try to use it when it isn't absolutely necessary.

---

 lxc-dave/fs/namei.c            |    6 +
 lxc-dave/fs/namespace.c        |  140 ++++++++++++++++++++++++++++++++++++++++-
 lxc-dave/include/linux/mount.h |    9 ++
 3 files changed, 150 insertions(+), 5 deletions(-)
fs/namei.c renamed from fs/namei.c~numa_mnt_want_write +4 -2
diff -puN fs/namei.c~numa_mnt_want_write fs/namei.c
@@ -231,10 +231,12 @@ int permission(struct inode *inode, int 
 	int retval, submask;
 
 	if (mask & MAY_WRITE) {
-
 		/*
-		 * Nobody gets write access to a read-only fs.
+		 * If this WARN_ON() is hit, it likely means that
+		 * there was a missed mnt_want_write() on the path
+		 * leading here.
 		 */
+		WARN_ON(__mnt_is_readonly(nd->mnt));
 		if (IS_RDONLY(inode) &&
 		    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
 			return -EROFS;
fs/namespace.c renamed from fs/namespace.c~numa_mnt_want_write +137 -3

174 lines not on this page · raw message

include/linux/mount.h renamed from include/linux/mount.h~numa_mnt_want_write +9 -0
diff -puN include/linux/mount.h~numa_mnt_want_write include/linux/mount.h
@@ -14,6 +14,7 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/nodemask.h>
 #include <linux/spinlock.h>
 #include <asm/atomic.h>
 
@@ -61,6 +62,14 @@ struct vfsmount {
 	atomic_t mnt_count;
 	int mnt_expiry_mark;		/* true if marked for expiry */
 	int mnt_pinned;
+
+	/*
+	 * This value is not stable unless all of the
+	 * mnt_writers[] spinlocks are held, and all
+	 * mnt_writer[]s on this mount have 0 as
+	 * their ->count
+	 */
+	atomic_t __mnt_writers;
 };
 
 static inline struct vfsmount *mntget(struct vfsmount *mnt)

_

#72 lore raw

Re: [PATCH 25/26] r/o bind mounts: scalable writer count

Quoted 7 lines

This uses a statically-allocated percpu variable. All operations are local to a cpu as long that cpu operates on the same mount, and there are no writer count imbalances. Writer count imbalances happen when a write is taken on one cpu, and released on another, like when an open/close pair is performed on two different cpus because the task moved.

I'd suggest something along these lines in final mntput:

	lock_and_coalesce_cpu_mnt_writer_counts();
	mnt_unlock_cpus();
	BUG_ON(atomic_read(&mnt->__mnt_writers));

since there's basically no other we'll notice if there _is_ an imbalance.

Miklos

#73 lore raw

Re: [PATCH 25/26] r/o bind mounts: scalable writer count

Quoted 5 lines

I'd suggest something along these lines in final mntput:

	lock_and_coalesce_cpu_mnt_writer_counts();
	mnt_unlock_cpus();
	BUG_ON(atomic_read(&mnt->__mnt_writers));

Or rather a WARN_ON(), since it's not a fatal condition.

Miklos

#74 lore raw

Re: [PATCH 25/26] r/o bind mounts: scalable writer count

In reply to Miklos Szeredi
Quoted 9 lines · Miklos Szeredi

I'd suggest something along these lines in final mntput:

        lock_and_coalesce_cpu_mnt_writer_counts();
        mnt_unlock_cpus();
        BUG_ON(atomic_read(&mnt->__mnt_writers));

since there's basically no other we'll notice if there _is_ an imbalance.

That's a very good idea. I've added that check, but a WARN_ON() instead of a BUG_ON().

Does __mntput() seem like the right place to you?

-- Dave

#75 lore raw

Re: [PATCH 25/26] r/o bind mounts: scalable writer count

Quoted 6 lines
Quoted 6 lines

I'd suggest something along these lines in final mntput:

        lock_and_coalesce_cpu_mnt_writer_counts();
        mnt_unlock_cpus();
        BUG_ON(atomic_read(&mnt->__mnt_writers));
Quoted 7 lines
Quoted 2 lines

since there's basically no other we'll notice if there _is_ an imbalance.

That's a very good idea. I've added that check, but a WARN_ON() instead of a BUG_ON().

Does __mntput() seem like the right place to you?

Yes, it does.

Miklos

#77 lore raw

Re: [PATCH 25/26] r/o bind mounts: scalable writer count

Quoted 5 lines · Andrew Morton
Quoted 1 line

On Fri, 22 Jun 2007 13:03:36 -0700 Dave Hansen <[email protected]>

Quoted 1 line · wrote:

+DEFINE_PER_CPU(struct mnt_writer, mnt_writers);

this can have static scope.

Fixed. That'll be in my update.

-- Dave

#78 lore raw

PATCH 26/26 honor r/w changes at do_remount() time

In reply to Dave Hansen

Originally from: Herbert Poetzl <[email protected]>

This is the core of the read-only bind mount patch set.

Note that this does _not_ add a "ro" option directly to the bind mount operation. If you require such a mount, you must first do the bind, then follow it up with a 'mount -o remount,ro' operation.

---

 lxc-dave/fs/namespace.c        |   40 ++++++++++++++++++++++++++++++++++------
 lxc-dave/include/linux/mount.h |    7 ++++++-
 2 files changed, 40 insertions(+), 7 deletions(-)
fs/namespace.c renamed from fs/namespace.c~23-24-honor-r-w-changes-at-do-remount-time +34 -6
diff -puN fs/namespace.c~23-24-honor-r-w-changes-at-do-remount-time fs/namespace.c
@@ -201,7 +201,7 @@ void mnt_drop_write(struct vfsmount *mnt
 }
 EXPORT_SYMBOL_GPL(mnt_drop_write);
 
-int mnt_make_readonly(struct vfsmount *mnt)
+static int mnt_make_readonly(struct vfsmount *mnt)
 {
 	int ret = 0;
 
@@ -214,15 +214,21 @@ int mnt_make_readonly(struct vfsmount *m
 		goto out;
 	}
 	/*
-	 * actually set mount's r/o flag here to make
-	 * __mnt_is_readonly() true, which keeps anyone
-	 * from doing a successful mnt_want_write().
+	 * nobody can do a successful mnt_want_write() with all
+	 * of the counts in MNT_DENIED_WRITE and the locks held.
 	 */
+	if (!ret)
+		mnt->mnt_flags |= MNT_READONLY;
 out:
 	mnt_unlock_cpus();
 	return ret;
 }
 
+static void __mnt_unmake_readonly(struct vfsmount *mnt)
+{
+	mnt->mnt_flags &= ~MNT_READONLY;
+}
+
 int simple_set_mnt(struct vfsmount *mnt, struct super_block *sb)
 {
 	mnt->mnt_sb = sb;
@@ -524,7 +530,7 @@ static int show_vfsmnt(struct seq_file *
 	seq_path(m, mnt, mnt->mnt_root, " \t\n\\");
 	seq_putc(m, ' ');
 	mangle(m, mnt->mnt_sb->s_type->name);
-	seq_puts(m, mnt->mnt_sb->s_flags & MS_RDONLY ? " ro" : " rw");
+	seq_puts(m, __mnt_is_readonly(mnt) ? " ro" : " rw");
 	for (fs_infop = fs_info; fs_infop->flag; fs_infop++) {
 		if (mnt->mnt_sb->s_flags & fs_infop->flag)
 			seq_puts(m, fs_infop->str);
@@ -1093,6 +1099,23 @@ out:
 	return err;
 }
 
+static int change_mount_flags(struct vfsmount *mnt, int ms_flags)
+{
+	int error = 0;
+	int readonly_request = 0;
+
+	if (ms_flags & MS_RDONLY)
+		readonly_request = 1;
+	if (readonly_request == __mnt_is_readonly(mnt))
+		return 0;
+
+	if (readonly_request)
+		error = mnt_make_readonly(mnt);
+	else
+		__mnt_unmake_readonly(mnt);
+	return error;
+}
+
 /*
  * change filesystem flags. dir should be a physical root of filesystem.
  * If you've mounted a non-root directory somewhere and want to do remount
@@ -1114,7 +1137,10 @@ static int do_remount(struct nameidata *
 		return -EINVAL;
 
 	down_write(&sb->s_umount);
-	err = do_remount_sb(sb, flags, data, 0);
+	if (flags & MS_BIND)
+		err = change_mount_flags(nd->mnt, flags);
+	else
+		err = do_remount_sb(sb, flags, data, 0);
 	if (!err)
 		nd->mnt->mnt_flags = mnt_flags;
 	up_write(&sb->s_umount);
@@ -1558,6 +1584,8 @@ long do_mount(char *dev_name, char *dir_
 		mnt_flags |= MNT_NODIRATIME;
 	if (flags & MS_RELATIME)
 		mnt_flags |= MNT_RELATIME;
+	if (flags & MS_RDONLY)
+		mnt_flags |= MNT_READONLY;
 
 	flags &= ~(MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_ACTIVE |
 		   MS_NOATIME | MS_NODIRATIME | MS_RELATIME);
include/linux/mount.h renamed from include/linux/mount.h~23-24-honor-r-w-changes-at-do-remount-time +6 -1
diff -puN include/linux/mount.h~23-24-honor-r-w-changes-at-do-remount-time include/linux/mount.h
@@ -29,6 +29,7 @@ struct mnt_namespace;
 #define MNT_NOATIME	0x08
 #define MNT_NODIRATIME	0x10
 #define MNT_RELATIME	0x20
+#define MNT_READONLY	0x40 /* does the user want this to be r/o? */
 
 #define MNT_SHRINKABLE	0x100
 
@@ -87,7 +88,11 @@ static inline struct vfsmount *mntget(st
  */
 static inline int __mnt_is_readonly(struct vfsmount *mnt)
 {
-	return (mnt->mnt_sb->s_flags & MS_RDONLY);
+	if (mnt->mnt_flags & MNT_READONLY)
+		return 1;
+	if (mnt->mnt_sb->s_flags & MS_RDONLY)
+		return 1;
+	return 0;
 }
 
 extern int mnt_want_write(struct vfsmount *mnt);

_

#79 lore raw

Re: [PATCH 26/26] honor r/w changes at do_remount() time

Quoted 4 lines · Dave Hansen

Originally from: Herbert Poetzl <[email protected]>

Quoted 8 lines

This is the core of the read-only bind mount patch set.

Note that this does _not_ add a "ro" option directly to the bind mount operation. If you require such a mount, you must first do the bind, then follow it up with a 'mount -o remount,ro' operation.

Ok.

maintainer
#80 lore raw
In reply to Dave Hansen
Quoted 13 lines · On Fri, 22 Jun 2007 13:03:03 -0700 Dave Hansen <[email protected]> wrote:

Why do we need r/o bind mounts?

This feature allows a read-only view into a read-write filesystem. In the process of doing that, it also provides infrastructure for keeping track of the number of writers to any given mount.

This has a number of uses. It allows chroots to have parts of filesystems writable. It will be useful for containers in the future because users may have root inside a container, but should not be allowed to write to somefilesystems. This also replaces patches that vserver has had out of the tree for several years.

Quoted 10 lines

It allows security enhancement by making sure that parts of your filesystem read-only (such as when you don't trust your FTP server), when you don't want to have entire new filesystems mounted, or when you want atime selectively updated. I've been using the following script to test that the feature is working as desired. It takes a directory and makes a regular bind and a r/o bind mount of it. It then performs some normal filesystem operations on the three directories, including ones that are expected to fail, like creating a file on the r/o mount.

Doesn't selinux do some of this?

My overall reaction: owch. There's a ton of tricksy code here and great potential for us to accidentally break it in the future by forgetting a mnt_may_write() as the kernel evolves. And then there's the added complexity and the added runtime overhead.

Balance that against some pretty obscure-looking benefits and I'm struggling to see how a merge is justifiable?

#81 lore raw
Quoted 24 lines · Andrew Morton
Quoted 13 lines · On Fri, 22 Jun 2007 13:03:03 -0700 Dave Hansen <[email protected]> wrote:

Why do we need r/o bind mounts?

This feature allows a read-only view into a read-write filesystem. In the process of doing that, it also provides infrastructure for keeping track of the number of writers to any given mount.

This has a number of uses. It allows chroots to have parts of filesystems writable. It will be useful for containers in the future because users may have root inside a container, but should not be allowed to write to somefilesystems. This also replaces patches that vserver has had out of the tree for several years.

Quoted 10 lines

It allows security enhancement by making sure that parts of your filesystem read-only (such as when you don't trust your FTP server), when you don't want to have entire new filesystems mounted, or when you want atime selectively updated. I've been using the following script to test that the feature is working as desired. It takes a directory and makes a regular bind and a r/o bind mount of it. It then performs some normal filesystem operations on the three directories, including ones that are expected to fail, like creating a file on the r/o mount.

Quoted 5 lines

Doesn't selinux do some of this?

My overall reaction: owch. There's a ton of tricksy code here and great potential for us to accidentally break it in the future by forgetting a mnt_may_write() as the kernel evolves.

This is definitely a tricky thing. It takes a static, single check and replaces it with a matched set of operations. But, it's not much different that adding a mutex to something. People can always miss one side of the lock pair.

People won't miss the mnt_may_write() because it will become the only way that it is valid to check a mounted fs for the ability to write to it. IS_RDONLY() will not be available for these kinds of checks.

Quoted 4 lines

And then there's the added complexity and the added runtime overhead.

Balance that against some pretty obscure-looking benefits and I'm struggling to see how a merge is justifiable?

One reason Al had me go through using these paired operations instead of just passing the mount all over the vfs is that this fixes some existing, fundamental problems: we do not properly track when writers are _finished_ to our filesystems, and may allow a remount-r/o operation to success when writes are still occurring. We needed to separate out the logical "users can write to this fs" from the physical "this fs is on r/o media" or "this fs is dying and writes will only kill it more". That's what these patches do in the end.

One set of things that I'm going to tack on here once these go in is the ability to increment the writer count upon a decrement of i_nlink to zero. We'll drop the write count when the file is actually truncated. As it stands right now, since there is never an open filp on those files, you might unlink a file, do a r/o mount of the fs, then still write to it when the truncate occurs. I think fixing that was one of Al's long-term goals with this strategy.

-- Dave

#82 lore raw
In reply to Andrew Morton
Quoted 1 line · Andrew Morton

Doesn't selinux do some of this?

No.

Quoted 4 lines

My overall reaction: owch. There's a ton of tricksy code here and great potential for us to accidentally break it in the future by forgetting a mnt_may_write() as the kernel evolves. And then there's the added complexity and the added runtime overhead.

Most of the code is not actually implementing per-mountpoint r/o but fixing the way we deal with mount -o remount,ro and the general writeability state of a filesystem. Theres been a huge racy window before which is fixed. And it lays the groundwork for interesting new things like the posibility for the filesystem to revert to r/o after it's not been written to for a while.

There's been a plain r/o bindmounts patch from the vserver folks that's a lot smaller because it doesn't fix the problems (and actually widens them a little)

Quoted 3 lines

Balance that against some pretty obscure-looking benefits and I'm struggling to see how a merge is justifiable?

It's a feature people have been asking for a long time, just about a month ago there has been a lenghty thread on the nfs list about the requirement.

And it only gets more important (not to say absolutely essential) for full containers support, where you really don't want your untrusted container to able to write into the shared /usr.