Read-only bind mounts
Subsystems (17)
cover Read-only bind mounts
If we can't pull the entire series into -mm, can we just put the first three patches for now? They can stand on their own.
---
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.
PATCH 1/24 filesystem helpers for custom 'struct file's
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 an existing, static-scope init_file() to a less generic name.
---
lxc-dave/fs/configfs/dir.c | 5 +++-- lxc-dave/fs/file_table.c | 34 ++++++++++++++++++++++++++++++++++ lxc-dave/fs/hugetlbfs/inode.c | 22 +++++++++------------- 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 | 18 +++++++++--------- 8 files changed, 78 insertions(+), 49 deletions(-)
fs/configfs/dir.c renamed from fs/configfs/dir.c~filesystem-helpers-for-custom-struct-file-s +3 -2
@@ -142,7 +142,7 @@ static int init_dir(struct inode * inodereturn 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~filesystem-helpers-for-custom-struct-file-s +34 -0
@@ -137,6 +137,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~filesystem-helpers-for-custom-struct-file-s +9 -13
@@ -923,16 +923,11 @@ struct file *hugetlb_file_setup(const chif (!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 ))
@@ -941,17 +936,18 @@ struct file *hugetlb_file_setup(const chd_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:
include/linux/file.h renamed from include/linux/file.h~filesystem-helpers-for-custom-struct-file-s +9 -0
@@ -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~filesystem-helpers-for-custom-struct-file-s +5 -8
@@ -907,7 +907,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);
@@ -915,18 +915,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;
@@ -977,9 +975,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~filesystem-helpers-for-custom-struct-file-s +2 -5
@@ -2538,11 +2538,8 @@ struct file *shmem_file_setup(char *named_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~filesystem-helpers-for-custom-struct-file-s +7 -12
@@ -66,24 +66,19 @@ struct file *shmem_file_setup(char *nameif (!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~filesystem-helpers-for-custom-struct-file-s +9 -9
@@ -367,26 +367,26 @@ static int sock_alloc_fd(struct file **fstatic int sock_attach_fd (struct socket *sock,struct file *file) { +struct dentry *dentry;struct qstr name = { .name ="" }; - file->f_path.dentry =d_alloc (sock_mnt->mnt_sb->s_root, &name); -if (unlikely (!file->f_path.dentry)) + dentry =d_alloc (sock_mnt->mnt_sb->s_root, &name); +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)); sock->file = file; - file->f_op =SOCK_INODE (sock)->i_fop = &socket_file_ops; - file->f_mode =FMODE_READ |FMODE_WRITE ; +init_file (file, sock_mnt, dentry,FMODE_READ |FMODE_WRITE , + &socket_file_ops); +SOCK_INODE (sock)->i_fop = &socket_file_ops; file->f_flags =O_RDWR ; file->f_pos =0 ; file->private_data = sock;
_
Re: [PATCH 01/24] 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 an existing, static-scope
init_file()to a less generic name.
Looks good. But please provide a patch ontop of your patchkit to add kernel-doc comments for the two new exported functions.
Are there any direct caller of get_empty_filp left after this patch? We really should get rid of that export to stop people from shooting themselves in the foot.
PATCH 1/1 docuement filesystem helpers for custom 'struct file's
Quoted 14 lines · Christoph Hellwig
Quoted 13 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.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 5 lines
Quoted 2 lines
Also, rename an existing, static-scope
init_file()to a less generic name.Looks good. But please provide a patch ontop of your patchkit to add kernel-doc comments for the two new exported functions.
Appended. I started to write the comments to describe the function arguments, but they all looked pretty retarded.
Quoted 3 lines
Are there any direct caller of
get_empty_filpleft after this patch? We really should get rid of that export to stop people from shooting themselves in the foot.
Yeah, there are a few left. But, they're in the middle of what is sometimes tricky error handling, so I think we should trickle those in later.
-- Dave
lxc-dave/fs/file_table.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
fs/file_table.c renamed from fs/file_table.c~document +21 -1
@@ -137,6 +137,18 @@ fail:EXPORT_SYMBOL (get_empty_filp); +/** +* alloc_file - allocate and initialize a 'struct file' with +* the given arguments +* +* Use this instead of get_empty_filp() to get a new +* 'struct file'. Do so because of the same initialization +* pitfalls reasons listed for init_file(). This is a +* preferred interface to using init_file(). +* +* If all the callers of init_file() are eliminated, its +* code should be moved into this function. +*/ struct file *alloc_file (struct vfsmount *mnt,struct dentry *dentry,mode_t mode,const struct file_operations *fop) {
@@ -152,7 +164,15 @@ struct file *alloc_file(struct vfsmount }EXPORT_SYMBOL (alloc_file); -/* +/** +* init_file - initialize a 'struct file' +* @file: the already allocated 'struct file' to initialized +* @mnt: the vfsmount on which the file resides +* +* Use this instead of setting the members directly. Doing so +* avoids making mistakes like forgetting the mntget() or +* forgetting to take a write on the mnt. +* * 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
_
Re: [PATCH] docuement filesystem helpers for custom 'struct file's
Quoted 15 lines · On Wed, 19 Sep 2007 10:47:23 -0700 Dave Hansen wrote:
Quoted 14 lines · Christoph Hellwig
Quoted 13 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.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 5 lines
Quoted 2 lines
Also, rename an existing, static-scope
init_file()to a less generic name.Looks good. But please provide a patch ontop of your patchkit to add kernel-doc comments for the two new exported functions.
Appended. I started to write the comments to describe the function arguments, but they all looked pretty retarded.
Hi Dave,
They aren't quite in kernel-doc format. Holler if you need help with that, or see examples, or Documentation/kernel-doc-nano-HOWTO.txt.
Quoted 18 lines
Quoted 3 lines
Are there any direct caller of
get_empty_filpleft after this patch? We really should get rid of that export to stop people from shooting themselves in the foot.Yeah, there are a few left. But, they're in the middle of what is sometimes tricky error handling, so I think we should trickle those in later.
-- Dave
lxc-dave/fs/file_table.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)diff -puN
fs/file_table.c~documentfs/file_table.c
fs/file_table.c fail: · line 142
Quoted 6 lines
fs/file_table.c renamed from fs/file_table.c~document +3
-0@@ -137,6 +137,18 @@ fail:EXPORT_SYMBOL (get_empty_filp); +/** +* alloc_file - allocate and initialize a 'struct file' with +* the given arguments
* function_name - short description (on one line)
* @param1: descr1
* @param2: descr2
etc.
Quoted 19 lines
fs/file_table.c renamed from fs/file_table.c~document +11
-1+* +* Use this instead of get_empty_filp() to get a new +* 'struct file'. Do so because of the same initialization +* pitfalls reasons listed for init_file(). This is a +* preferred interface to using init_file(). +* +* If all the callers of init_file() are eliminated, its +* code should be moved into this function. +*/ struct file *alloc_file (struct vfsmount *mnt,struct dentry *dentry,mode_t mode,const struct file_operations *fop) {@@ -152,7 +164,15 @@ struct file *alloc_file(struct vfsmount }EXPORT_SYMBOL (alloc_file); -/* +/** +* init_file - initialize a 'struct file'
fs/file_table.c struct file *alloc_file(struct vfsmount · line 178
Quoted 10 lines
fs/file_table.c renamed from fs/file_table.c~document +7
-0+* @file: the already allocated 'struct file' to initialized +* @mnt: the vfsmount on which the file resides +* +* Use this instead of setting the members directly. Doing so +* avoids making mistakes like forgetting the mntget() or +* forgetting to take a write on the mnt. +* * 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
OK, that one looks better. :)
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
Re: [PATCH] docuement filesystem helpers for custom 'struct file's
Quoted 4 lines · Randy Dunlap
They aren't quite in kernel-doc format. Holler if you need help with that, or see examples, or
Documentation/kernel-doc-nano-HOWTO.txt.
Should we do comments for every single function argument, or is it OK to leave them out for the obvious ones?
init_file() gets really boring:
Quoted 6 lines
+ *
init_file- initialize a 'struct file'
+ * @file: the already allocated 'struct file' to initialized
+ * @mnt: the vfsmount on which the file resides
+ * @dentry: the dentry for the file
+ * @mode: the file's mode
+ * @fop: the file's operations
Seems like a waste of space to me, but I'd be happy to do them if that's the convention.
-- Dave
Re: [PATCH] docuement filesystem helpers for custom 'struct file's
Quoted 2 lines · Dave Hansen
Should we do comments for every single function argument, or is it OK to leave them out for the obvious ones?
Yes, we should keep the kerneldoc comments coherent, even if it seems useless in a few cases.
PATCH 2/24 rearrange may_open() to be r/o friendly
In reply to Dave Hansenmay_open() calls vfs_permission() before it does checks for IS_RDONLY(inode). It checks _again_ inside of vfs_permission().
The check inside of vfs_permission() is going away eventually. With the mnt_want/drop_write() functions, all of the r/o checks (except for this one) are consistently done before calling permission(). Because of this, I'd like to use permission() to hold a debugging check to make sure that the mnt_want/drop_write() calls are actually being made.
So, to do this:
1. remove the IS_RDONLY() check from permission()
2. enforce that you must mnt_want_write() before
even calling permission()
3. enable a debugging in permission()
We need to rearrange may_open(). Here's the patch.
---
lxc-dave/fs/namei.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
fs/namei.c renamed from fs/namei.c~rearrange-permission-and-ro-checks-in-may_open +9 -5
@@ -228,6 +228,10 @@ int generic_permission(struct inode *inoint permission (struct inode *inode,int mask,struct nameidata *nd) {int retval, submask; +struct vfsmount *mnt =NULL ; + +if (nd) + mnt = nd->mnt;if (mask &MAY_WRITE ) {umode_t mode = inode->i_mode;
@@ -251,7 +255,7 @@ int permission(struct inode *inode, int* MAY_EXEC on regular files is denied if the fs is mounted * with the "noexec" flag. */ -if (nd && nd->mnt && (nd->mnt->mnt_flags &MNT_NOEXEC )) +if (mnt && (mnt->mnt_flags &MNT_NOEXEC ))return -EACCES ; }
@@ -1604,10 +1608,6 @@ int may_open(struct nameidata *nd, int aif (S_ISDIR (inode->i_mode) && (flag &FMODE_WRITE ))return -EISDIR ; - error =vfs_permission (nd, acc_mode); -if (error) -return error; -/* * FIFO's, sockets and device files are special: they don't * actually live on the filesystem itself, and as such you
@@ -1622,6 +1622,10 @@ int may_open(struct nameidata *nd, int a flag &= ~O_TRUNC ; }else if (IS_RDONLY (inode) && (flag &FMODE_WRITE ))return -EROFS ; + + error =vfs_permission (nd, acc_mode); +if (error) +return error;/* * An append-only file must be opened in append mode for writing. */
_
Re: [PATCH 02/24] rearrange may_open() to be r/o friendly
Quoted 30 lines · Dave Hansen
may_open()callsvfs_permission()before it does checks forIS_RDONLY(inode). It checks _again_ inside ofvfs_permission().The check inside of
vfs_permission()is going away eventually. With the mnt_want/drop_write() functions, all of the r/o checks (except for this one) are consistently done before callingpermission(). Because of this, I'd like to usepermission()to hold a debugging check to make sure that the mnt_want/drop_write() calls are actually being made.So, to do this:
1. remove theIS_RDONLY()check frompermission()
2. enforce that you mustmnt_want_write()before
even callingpermission()
3. enable a debugging inpermission()We need to rearrange
may_open(). Here's the patch.int
permission(struct inode *inode, int mask, struct nameidata *nd){fs/namei.c renamed from fs/namei.c~rearrange-permission-and-ro-checks-in-may_open +4
-0int retval, submask; +struct vfsmount *mnt =NULL ; + +if (nd) + mnt = nd->mnt;if (mask &MAY_WRITE ) {umode_t mode = inode->i_mode;
fs/namei.c int permission(struct inode *inode, int · line 258
Quoted 5 lines
fs/namei.c renamed from fs/namei.c~rearrange-permission-and-ro-checks-in-may_open +1
-1@@ -251,7 +255,7 @@ int permission(struct inode *inode, int* MAY_EXEC on regular files is denied if the fs is mounted * with the "noexec" flag. */ -if (nd && nd->mnt && (nd->mnt->mnt_flags &MNT_NOEXEC )) +if (mnt && (mnt->mnt_flags &MNT_NOEXEC ))
Why is this entirely unrelated cleanup in this patch?
Except for that it looks fine.
Re: [PATCH 02/24] rearrange may_open() to be r/o friendly
Quoted 19 lines · Christoph Hellwig
Quoted 11 lines
int
permission(struct inode *inode, int mask, struct nameidata *nd){fs/namei.c renamed from fs/namei.c~rearrange-permission-and-ro-checks-in-may_open +4
-0int retval, submask; +struct vfsmount *mnt =NULL ; + +if (nd) + mnt = nd->mnt;if (mask &MAY_WRITE ) {umode_t mode = inode->i_mode;fs/namei.c int permission(struct inode *inode, int · line 258
Quoted 5 lines
fs/namei.c renamed from fs/namei.c~rearrange-permission-and-ro-checks-in-may_open +1
-1@@ -251,7 +255,7 @@ int permission(struct inode *inode, int* MAY_EXEC on regular files is denied if the fs is mounted * with the "noexec" flag. */ -if (nd && nd->mnt && (nd->mnt->mnt_flags &MNT_NOEXEC )) +if (mnt && (mnt->mnt_flags &MNT_NOEXEC ))Why is this entirely unrelated cleanup in this patch?
Quoted 1 line
Except for that it looks fine.
You're right. In this context, it looks completely unrelated. I'll separate it into another patch.
-- Dave
PATCH 3/24 create cleanup helper svc_msnfs()
In reply to Dave HansenI'm going to be modifying nfsd_rename() shortly to support read-only bind mounts. This #ifdef is around the area I'm patching, and it starts to get really ugly if I just try to add my new code by itself. Using this little helper makes things a lot cleaner to use.
---
lxc-dave/fs/nfsd/vfs.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~create-svc_msnfs-helper +11 -4
@@ -865,6 +865,15 @@ static int nfsd_direct_splice_actor(strureturn __splice_from_pipe (pipe, sd, nfsd_splice_actor); } +static inline int svc_msnfs (struct svc_fh *ffhp) +{ +#ifdef MSNFS +return (ffhp->fh_export->ex_flags &NFSEXP_MSNFS ); +#else +return 0 ; +#endif +} +static __be32 nfsd_vfs_read (struct svc_rqst *rqstp,struct svc_fh *fhp,struct file *file,loff_t offset,struct kvec *vec,int vlen,unsigned long *count)
@@ -877,11 +886,9 @@ nfsd_vfs_read(struct svc_rqst *rqstp, st err = nfserr_perm; inode = file->f_path.dentry->d_inode; -#ifdef MSNFS -if ((fhp->fh_export->ex_flags &NFSEXP_MSNFS ) && - (!lock_may_read (inode, offset, *count))) + +if (svc_msnfs (fhp) && !lock_may_read (inode, offset, *count)) goto out; -#endif /* Get readahead parameters */ ra =nfsd_get_raparms (inode->i_sb->s_dev, inode->i_ino);
_
Re: [PATCH 03/24] create cleanup helper svc_msnfs()
Quoted 6 lines · Dave Hansen
I'm going to be modifying
nfsd_rename()shortly to support read-only bind mounts. This #ifdef is around the area I'm patching, and it starts to get really ugly if I just try to add my new code by itself. Using this little helper makes things a lot cleaner to use.
Ok.
Re: [PATCH 03/24] create cleanup helper svc_msnfs()
In reply to Dave HansenQuoted 9 lines · Dave Hansen
lxc-dave/fs/nfsd/vfs.c| 15 +++++++++++----1 file changed, 11 insertions(+), 4 deletions(-)diff -puN
fs/nfsd/vfs.c~create-svc_msnfs-helperfs/nfsd/vfs.cfs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~create-svc_msnfs-helper +0
-0@@ -865,6 +865,15 @@ static int nfsd_direct_splice_actor(strureturn __splice_from_pipe (pipe, sd, nfsd_splice_actor);
fs/nfsd/vfs.c static int nfsd_direct_splice_actor(stru · line 875
Quoted 10 lines
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~create-svc_msnfs-helper +8
-0} +static inline int svc_msnfs (struct svc_fh *ffhp) +{ +#ifdef MSNFS +return (ffhp->fh_export->ex_flags &NFSEXP_MSNFS ); +#else +return 0 ; +#endif +}
The name looks kinda confusing. msnfs? almost makes you think what's got microsoft to do with it..
Re: [PATCH 03/24] create cleanup helper svc_msnfs()
Quoted 12 lines · Jan Engelhardt
fs/nfsd/vfs.c static int nfsd_direct_splice_actor(stru · line 875
Quoted 9 lines
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~create-svc_msnfs-helper +8
-0+static inline int svc_msnfs (struct svc_fh *ffhp) +{ +#ifdef MSNFS +return (ffhp->fh_export->ex_flags &NFSEXP_MSNFS ); +#else +return 0 ; +#endif +}The name looks kinda confusing. msnfs? almost makes you think what's got microsoft to do with it..
I'd give it a better name if I had any idea what it means. :)
The macros and names are all preexisting. I just took the liberty of pulling the #ifdef out into a function because the code got very unreadable with the other modifications that I need.
If it were some kind of larger API, we could come up with something better. But, for a single isolated user, I think it is OK.
-- Dave
Re: [PATCH 03/24] create cleanup helper svc_msnfs()
Quoted 14 lines · Dave Hansen
Quoted 12 lines · Jan Engelhardt
fs/nfsd/vfs.c static int nfsd_direct_splice_actor(stru · line 875
Quoted 9 lines
fs/nfsd/vfs.c renamed from fs/nfsd/vfs.c~create-svc_msnfs-helper +8
-0+static inline int svc_msnfs (struct svc_fh *ffhp) +{ +#ifdef MSNFS +return (ffhp->fh_export->ex_flags &NFSEXP_MSNFS ); +#else +return 0 ; +#endif +}The name looks kinda confusing. msnfs? almost makes you think what's got microsoft to do with it..
Quoted 8 lines
I'd give it a better name if I had any idea what it means. :)
The macros and names are all preexisting. I just took the liberty of pulling the #ifdef out into a function because the code got very unreadable with the other modifications that I need.
If it were some kind of larger API, we could come up with something better. But, for a single isolated user, I think it is OK.
The msnfs option means enable the use of MS Windows share locks, so yes, the name is appropriate.
Trond
Re: [PATCH 03/24] create cleanup helper svc_msnfs()
Quoted 5 lines · Trond Myklebust
Quoted 2 lines
If it were some kind of larger API, we could come up with something better. But, for a single isolated user, I think it is OK.
The msnfs option means enable the use of MS Windows share locks, so yes, the name is appropriate.
Yepp. While we're at it, can we please kill the silly ifdefs for it (once Dave's patches are merged to avoid conflicts). The code has always been enabled since it was introduced, it's an export option anyway and to make things worse the ifdef needs to be disabled in multiple files to make it consistant..
Quoted 8 lines
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
---end quoted text---
Re: [PATCH 03/24] create cleanup helper svc_msnfs()
Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Trond Myklebust
Quoted 2 lines
If it were some kind of larger API, we could come up with something better. But, for a single isolated user, I think it is OK.
Quoted 8 lines
Quoted 2 lines
The msnfs option means enable the use of MS Windows share locks, so yes, the name is appropriate.
Yepp. While we're at it, can we please kill the silly ifdefs for it (once Dave's patches are merged to avoid conflicts). The code has always been enabled since it was introduced, it's an export option anyway and to make things worse the ifdef needs to be disabled in multiple files to make it consistant..
I'm sure that either Neil or Bruce would be happy to take such a patch.
Cheers
Trond
PATCH 4/24 r/o bind mounts: stub functions
In reply to Dave HansenThis 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.
---
lxc-dave/fs/namespace.c | 46 +++++++++++++++++++++++++++++++++++++++++ lxc-dave/include/linux/mount.h | 3 ++ 2 files changed, 49 insertions(+)
fs/namespace.c renamed from fs/namespace.c~add-vfsmount-writer-count +46 -0
@@ -80,6 +80,52 @@ struct vfsmount *alloc_vfsmnt(const charreturn mnt; } +/* +* Most r/o checks on a fs are for operations that take +* discrete amounts of time, like a write() or unlink(). +* We must keep track of when those operations start +* (for permission checks) and when they end, so that +* we can determine when writes are able to occur to +* a filesystem. +*/ +/* +* This tells the low-level filesystem that a write is +* about to be performed to it, and makes sure that +* writes are allowed before returning success. When +* the write operation is finished, mnt_drop_write() +* must be called. This is effectively a refcount. +*/ +int mnt_want_write (struct vfsmount *mnt) +{ +if (__mnt_is_readonly (mnt)) +return -EROFS ; +return 0 ; +} +EXPORT_SYMBOL_GPL (mnt_want_write); + +/* +* Tells the low-level filesystem that we are done +* performing a write to it. Must be matched with +* mnt_want_write() call above. +*/ +void mnt_drop_write (struct vfsmount *mnt) +{ +} +EXPORT_SYMBOL_GPL (mnt_drop_write); + +/* +* This shouldn't be used directly ouside of the VFS. +* It does not guarantee that the filesystem will stay +* r/w, just that it is right *now*e +* mnt_want/drop_write() will _keep_ the filesystem +* r/w. +*/ +int __mnt_is_readonly (struct vfsmount *mnt) +{ +return (mnt->mnt_sb->s_flags &MS_RDONLY ); +} +EXPORT_SYMBOL_GPL (__mnt_is_readonly); +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~add-vfsmount-writer-count +3 -0
@@ -74,9 +74,12 @@ static inline struct vfsmount *mntget(streturn mnt; } +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); +extern int __mnt_is_readonly (struct vfsmount *mnt);static inline void mntput (struct vfsmount *mnt) {
_
Re: [PATCH 04/24] r/o bind mounts: stub functions
Quoted 10 lines · Dave Hansen
This patch adds two function
mnt_want_write()andmnt_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.
kerneldoc comments, please!
Otherwise looks fine.
PATCH 5/24 elevate write count open()'d files
In reply to Dave HansenThis 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~tricky-elevate-write-count-files-are-open-ed +8 -1
@@ -167,6 +167,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);
@@ -204,8 +208,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~tricky-elevate-write-count-files-are-open-ed +16 -4
@@ -1620,8 +1620,15 @@ int may_open(struct nameidata *nd, int areturn -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; + } error =vfs_permission (nd, acc_mode);if (error)
@@ -1764,14 +1771,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 ;
@@ -1809,6 +1819,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~tricky-elevate-write-count-files-are-open-ed +3 -0
@@ -683,6 +683,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); }
_
Re: [PATCH 05/24] elevate write count open()'d files
Quoted 17 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 inmay_open()above themnt_want_write()call are, combined, equivalent tospecial_file().There is also an elevated count around the
vfs_create()call inopen_namei(). The count needs to be kept elevated all the way into themay_open()call. Otherwise, when the write is dropped, aro->rwtransisition could occur. This would lead to having rw access on the newly created file, while the vfsmount is ro. That is bad.
Quoted 4 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.
Ok.
PATCH 6/24 r/o bind mounts: elevate write count for some ioctls
In reply to Dave HansenSome 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 | 55 +++++++++++------ 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, 273 insertions(+), 157 deletions(-)
fs/ext2/ioctl.c renamed from fs/ext2/ioctl.c~ioctl-mnt-takers +30 -16
@@ -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>
@@ -23,6 +24,7 @@ int ext2_ioctl (struct inode * inode, ststruct ext2_inode_info *ei =EXT2_I (inode);unsigned int flags;unsigned short rsv_window_size; +int ret;ext2_debug ("cmd = %u, arg = %lu\n" , cmd, arg);
@@ -34,14 +36,19 @@ int ext2_ioctl (struct inode * inode, stcase EXT2_IOC_SETFLAGS : {unsigned int oldflags; -if (IS_RDONLY (inode)) -return -EROFS ; - -if (!is_owner_or_cap (inode)) -return -EACCES ; + ret =mnt_want_write (filp->f_vfsmnt); +if (ret) +return ret; + +if (!is_owner_or_cap (inode)) { + 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 ;
@@ -58,7 +65,8 @@ int ext2_ioctl (struct inode * inode, stif ((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; } }
@@ -70,20 +78,26 @@ int ext2_ioctl (struct inode * inode, stext2_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 (!is_owner_or_cap (inode))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;case EXT2_IOC_GETRSVSZ :if (test_opt (inode->i_sb,RESERVATION ) && S_ISREG(inode->i_mode)
fs/ext3/ioctl.c renamed from fs/ext3/ioctl.c~ioctl-mnt-takers +66 -34
193 lines not on this page · raw message
fs/ext4/ioctl.c renamed from fs/ext4/ioctl.c~ioctl-mnt-takers +67 -38
202 lines not on this page · raw message
fs/fat/file.c renamed from fs/fat/file.c~ioctl-mnt-takers +6 -4
@@ -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 *inodmutex_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 *inodMSDOS_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~ioctl-mnt-takers +24 -15
@@ -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 (!is_owner_or_cap (inode)) -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 (!is_owner_or_cap (inode)) { + 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~ioctl-mnt-takers +21 -12
@@ -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>
@@ -65,16 +66,20 @@ int jfs_ioctl(struct inode * inode, strureturn put_user (flags, (int __user *) arg);case JFS_IOC_SETFLAGS : {unsigned int oldflags; +int err; -if (IS_RDONLY (inode)) -return -EROFS ; - -if (!is_owner_or_cap (inode)) -return -EACCES ; - -if (get_user (flags, (int __user *) arg)) -return -EFAULT ; - + err =mnt_want_write (filp->f_vfsmnt); +if (err) +return err; + +if (!is_owner_or_cap (inode)) { + 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 ;
@@ -89,8 +94,10 @@ int jfs_ioctl(struct inode * inode, struif ((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 ;
@@ -100,7 +107,9 @@ int jfs_ioctl(struct inode * inode, strujfs_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~ioctl-mnt-takers +6 -5
@@ -58,10 +58,6 @@ static int ocfs2_set_inode_attr(struct i goto bail; } - status = -EROFS ; -if (IS_RDONLY (inode)) - goto bail_unlock; - status = -EACCES ;if (!is_owner_or_cap (inode)) goto bail_unlock;
@@ -130,8 +126,13 @@ int ocfs2_ioctl(struct inode * inode, stif (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;case OCFS2_IOC_RESVSP :case OCFS2_IOC_RESVSP64 :case OCFS2_IOC_UNRESVSP :
fs/reiserfs/ioctl.c renamed from fs/reiserfs/ioctl.c~ioctl-mnt-takers +35 -20
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~ioctl-mnt-takers +10 -5
@@ -554,8 +554,6 @@ xfs_attrmulti_attr_set(char *kbuf;int error =EFAULT ; -if (IS_RDONLY (inode)) -return -EROFS ;if (IS_IMMUTABLE (inode) ||IS_APPEND (inode))return EPERM ;if (len >XATTR_SIZE_MAX )
@@ -581,8 +579,6 @@ xfs_attrmulti_attr_remove( char *name, __uint32_t flags) { - if (IS_RDONLY(inode)) - return -EROFS; if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) return EPERM;return xfs_attr_remove (XFS_I (inode), name, flags);
@@ -592,6 +588,7 @@ STATIC intxfs_attrmulti_by_handle ( xfs_mount_t *mp, void__user *arg, + struct file *parfilp, struct inode *parinode) {int error;
@@ -646,13 +643,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 (inode, 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 (inode, attr_name, ops[i].am_flags); +mnt_drop_write (parfilp->f_vfsmnt);break ;default : ops[i].am_error =EINVAL ;
@@ -826,7 +831,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~ioctl-mnt-takers +0 -7
@@ -140,13 +140,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~ioctl-mnt-takers +8 -1
@@ -51,6 +51,7 @@#include "xfs_vnodeops.h" #include <linux/capability.h> +#include <linux/mount.h> #include <linux/writeback.h>
@@ -690,10 +691,16 @@ start:if (new_size > xip->i_size) io->io_new_size = new_size; -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); }/*
_
Re: [PATCH 06/24] 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.
Ok.
PATCH 7/24 elevate writer count for chown and friends
In reply to Dave Hansenchown/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~elevate-writer-count-for-chown-and-friends +30 -9
@@ -571,12 +571,12 @@ asmlinkage long sys_fchmod(unsigned intaudit_inode (NULL , dentry); - 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;
@@ -585,6 +585,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:
@@ -604,13 +606,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 )
@@ -620,6 +622,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:
@@ -642,9 +646,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;
@@ -675,7 +676,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;
@@ -695,7 +701,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;
@@ -709,7 +720,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;
@@ -726,9 +742,14 @@ asmlinkage long sys_fchown(unsigned intif (!file) goto out; + error =mnt_want_write (file->f_vfsmnt); +if (error) + goto out_fput; dentry = file->f_path.dentry;audit_inode (NULL , dentry); error =chown_common (dentry, user, group); +mnt_drop_write (file->f_vfsmnt); +out_fput:fput (file); out:return error;
_
Re: [PATCH 07/24] elevate writer count for chown and friends
Quoted 5 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.
Ok.
PATCH 8/24 make access() use mnt check
In reply to Dave HansenIt 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 | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
fs/open.c renamed from fs/open.c~make-access-use-helper +11 -2
@@ -457,8 +457,17 @@ asmlinkage long sys_faccessat(int dfd, cif (res || !(mode &S_IWOTH ) ||special_file (nd.dentry->d_inode->i_mode)) goto out_path_release; - -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 +* no actual write to the fs is performed here, we do +* not need to telegraph to that to anyone. +* +* By doing this, we accept that this access is +* inherently racy and know that the fs may change +* state before we even see this result. +*/ +if (__mnt_is_readonly (nd.mnt)) res = -EROFS ; out_path_release:
_
Re: [PATCH 08/24] make access() use mnt check
Quoted 7 lines · Dave Hansen
fs/open.c renamed from fs/open.c~make-access-use-helper +2
-2if (res || !(mode &S_IWOTH ) ||special_file (nd.dentry->d_inode->i_mode)) goto out_path_release; - -if (IS_RDONLY (nd.dentry->d_inode)) +/* +* This is a rare case where using __mnt_is_readonly()
fs/open.c asmlinkage long sys_faccessat(int dfd, c · line 471
Quoted 10 lines
fs/open.c renamed from fs/open.c~make-access-use-helper +9
-0+* is OK without a mnt_want/drop_write() pair. Since +* no actual write to the fs is performed here, we do +* not need to telegraph to that to anyone. +* +* By doing this, we accept that this access is +* inherently racy and know that the fs may change +* state before we even see this result. +*/ +if (__mnt_is_readonly (nd.mnt)) res = -EROFS ;
Ok.
PATCH 9/24 elevate mnt writers for callers of vfs_mkdir()
In reply to Dave HansenPretty 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~elevate-mnt-writers-for-callers-of-vfs-mkdir +5 -0
@@ -2051,7 +2051,12 @@ asmlinkage long sys_mkdirat(int dfd, conif (!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~elevate-mnt-writers-for-callers-of-vfs-mkdir +4 -0
@@ -156,7 +156,11 @@ nfsd4_create_clid_dir(struct nfs4_clientdprintk ("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:
_
Re: [PATCH 09/24] 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.
PATCH 10/24 elevate write count during entire ncp_ioctl()
In reply to Dave HansenSome 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~elevate-write-count-during-entire-ncp-ioctl +54 -1
@@ -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>
@@ -261,7 +262,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);
@@ -822,6 +823,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) {
_
Re: [PATCH 10/24] elevate write count during entire ncp_ioctl()
Quoted 3 lines · 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.
The WARN_ON is too agressive because it'll happen whenever someone tries a random, non-existant ioctl on ncpfs. Otherwise the patch looks good.
PATCH 11/24 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~elevate-write-count-for-link-and-symlink-calls +10 -0
@@ -2324,7 +2324,12 @@ asmlinkage long sys_symlinkat(const charif (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);
@@ -2419,7 +2424,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);
_
Re: [PATCH 11/24] elevate write count for link and symlink calls
Ok.
PATCH 12/24 elevate mount count for extended attributes
In reply to Dave HansenThis 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~elevate-mount-count-for-extended-attributes +6 -1
@@ -658,14 +658,19 @@ nfsd4_setattr(struct svc_rqst *rqstp, streturn 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~elevate-mount-count-for-extended-attributes +14 -2
@@ -11,6 +11,7 @@#include <linux/slab.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>
@@ -32,8 +33,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 ; }
@@ -235,7 +234,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; }
@@ -250,7 +253,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; }
@@ -266,9 +273,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); error =setxattr (dentry, name, value, size, flags); +mnt_drop_write (f->f_vfsmnt); +out_fput:fput (f);return error; }
_
Re: [PATCH 12/24] elevate mount count for extended attributes
Quoted 3 lines · Dave Hansen
This basically audits the callers of
xattr_permission(), which callspermission()and can perform writes to the filesystem.
Ok.
PATCH 13/24 elevate write count for file_update_time()
In reply to Dave Hansen---
lxc-dave/fs/inode.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
fs/inode.c renamed from fs/inode.c~elevate-write-count-for-file_update_time +12 -1
@@ -1241,10 +1241,19 @@ 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)) +/* +* 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 ; now =current_fs_time (inode->i_sb);
@@ -1260,6 +1269,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);
_
Re: [PATCH 13/24] elevate write count for file_update_time()
Ok.
fs/inode.c void file_update_time(struct file *file) · line 1253
Quoted 6 lines
fs/inode.c renamed from fs/inode.c~elevate-write-count-for-file_update_time +6
-0+/* +* 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 +*/
FYI: I have a patch to fix up nfsd, but there are more suckers around, e.g. reiserfs xattr crapola.
PATCH 14/24 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~unix-find-other-elevate-write-count-for-touch-atime +12 -4
@@ -729,21 +729,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) {
@@ -763,7 +769,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;
_
Re: [PATCH 14/24] unix_find_other() elevate write count for touch_atime()
Ok.
PATCH 15/24 elevate write count over calls to vfs_rename()
In reply to Dave HansenThis also uses the little helper in the NFS code to make an if() a little bit less ugly. We introduced the helper at the beginning of the series.
---
lxc-dave/fs/namei.c | 4 ++++ lxc-dave/fs/nfsd/vfs.c | 15 +++++++++++---- 2 files changed, 15 insertions(+), 4 deletions(-)
fs/namei.c renamed from fs/namei.c~elevate-write-count-over-calls-to-vfs-rename +4 -0
@@ -2655,8 +2655,12 @@ static int do_rename(int olddfd, const cif (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~elevate-write-count-over-calls-to-vfs-rename +11 -4
@@ -1659,13 +1659,20 @@ nfsd_rename(struct svc_rqst *rqstp, struif (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);
_
Re: [PATCH 15/24] elevate write count over calls to vfs_rename()
Ok.
PATCH 16/24 nfs: check mnt instead of superblock directly
In reply to Dave HansenIf we depend on the inodes for writeability, we will not catch the r/o mounts when implemented.
This patches uses __mnt_want_write(). It does not guarantee that the mount will stay writeable after the check. But, this is OK for one of the checks because it is just for a printk().
The other two are probably unnecessary and duplicate existing checks in the VFS. This won't make them better checks than before, but it will make them detect r/o mounts.
---
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
@@ -997,7 +997,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
@@ -1853,7 +1853,7 @@ nfsd_permission(struct svc_rqst *rqstp, inode->i_mode,IS_IMMUTABLE (inode)?" immut" :"" ,IS_APPEND (inode)?" append" :"" , - IS_RDONLY(inode)? " 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
@@ -1864,7 +1864,7 @@ nfsd_permission(struct svc_rqst *rqstp,*/ if (!(acc &MAY_LOCAL_ACCESS ))if (acc & (MAY_WRITE |MAY_SATTR |MAY_TRUNC )) { -if (exp_rdonly (rqstp, exp) ||IS_RDONLY (inode)) +if (exp_rdonly (rqstp, exp) ||__mnt_is_readonly (exp->ex_mnt))return nfserr_rofs;if (/* (acc & MAY_WRITE) && */ IS_IMMUTABLE (inode))return nfserr_perm;
_
Re: [PATCH 16/24] nfs: check mnt instead of superblock directly
Quoted 4 lines · Dave Hansen
If we depend on the inodes for writeability, we will not catch the r/o mounts when implemented.
Quoted 8 lines
This patches uses
__mnt_want_write(). It does not guarantee that the mount will stay writeable after the check. But, this is OK for one of the checks because it is just for aprintk().The other two are probably unnecessary and duplicate existing checks in the VFS. This won't make them better checks than before, but it will make them detect r/o mounts.
Ok from me, but can please get a nfs person to look over this aswell?
PATCH 17/24 elevate writer count for do_sys_truncate()
In reply to Dave Hansen---
lxc-dave/fs/open.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
fs/open.c renamed from fs/open.c~elevate-writer-count-for-do-sys-truncate +8 -6
@@ -244,21 +244,21 @@ 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; error =get_write_access (inode);if (error) - goto dput_and_out; + goto mnt_drop_write_and_out;/* * Make sure that there are no leases. get_write_access() protects
@@ -276,6 +276,8 @@ static long do_sys_truncate(const char _ put_write_and_out:put_write_access (inode); +mnt_drop_write_and_out: +mnt_drop_write (nd.mnt); dput_and_out:path_release (&nd); out:
_
Re: [PATCH 17/24] elevate writer count for do_sys_truncate()
Ok.
PATCH 18/24 elevate write count for do_utimes()
In reply to Dave Hansen---
lxc-dave/fs/utimes.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
fs/utimes.c renamed from fs/utimes.c~elevate-write-count-for-do-utimes +9 -6
@@ -2,6 +2,7 @@#include <linux/file.h> #include <linux/fs.h> #include <linux/linkage.h> +#include <linux/mount.h> #include <linux/namei.h> #include <linux/sched.h> #include <linux/stat.h>
@@ -88,8 +89,8 @@ long do_utimes(int dfd, char __user *fil inode = 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() */
@@ -97,7 +98,7 @@ long do_utimes(int dfd, char __user *filif (times) { error = -EPERM ;if (IS_APPEND (inode) ||IS_IMMUTABLE (inode)) - goto dput_and_out; + goto mnt_drop_write_and_out;if (times[0 ].tv_nsec ==UTIME_OMIT ) newattrs.ia_valid &= ~ATTR_ATIME ;
@@ -117,22 +118,24 @@ 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 (!is_owner_or_cap (inode)) {if (f) {if (!(f->f_mode &FMODE_WRITE )) - goto dput_and_out; + goto mnt_drop_write_and_out; }else { error =vfs_permission (&nd,MAY_WRITE );if (error) - goto dput_and_out; + goto mnt_drop_write_and_out; } } }mutex_lock (&inode->i_mutex); error =notify_change (dentry, &newattrs);mutex_unlock (&inode->i_mutex); +mnt_drop_write_and_out: +mnt_drop_write (nd.mnt); dput_and_out:if (f)fput (f);
_
Re: [PATCH 18/24] elevate write count for do_utimes()
Ok.
PATCH 19/24 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~elevate-write-count-for-do-sys-utime-and-touch-atime +12 -8
@@ -1185,22 +1185,23 @@ void touch_atime(struct vfsmount *mnt, sstruct 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
@@ -1211,16 +1212,19 @@ void touch_atime(struct vfsmount *mnt, s &inode->i_atime) < 0 &×pec_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);
_
Re: [PATCH 19/24] elevate write count for do_sys_utime() and touch_atime()
Ok.
PATCH 20/24 sys_mknodat(): elevate write count for vfs_mknod/create()
In reply to Dave HansenThis 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.
---
lxc-dave/fs/namei.c | 32 +++++++++++++++++++++----------- lxc-dave/fs/nfsd/vfs.c | 4 ++++ lxc-dave/net/unix/af_unix.c | 4 ++++ 3 files changed, 29 insertions(+), 11 deletions(-)
fs/namei.c renamed from fs/namei.c~sys-mknodat-elevate-write-count-for-vfs-mknod-create +21 -11
@@ -1970,12 +1970,25 @@ asmlinkage long sys_mknodat(int dfd, conif (error) goto out; dentry =lookup_create (&nd,0 ); - error =PTR_ERR (dentry); - +if (IS_ERR (dentry)) { + error =PTR_ERR (dentry); + 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 ;
@@ -1986,14 +1999,11 @@ asmlinkage long sys_mknodat(int dfd, concase 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~sys-mknodat-elevate-write-count-for-vfs-mknod-create +4 -0
@@ -1233,7 +1233,11 @@ nfsd_create(struct svc_rqst *rqstp, strucase 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);
net/unix/af_unix.c renamed from net/unix/af_unix.c~sys-mknodat-elevate-write-count-for-vfs-mknod-create +4 -0
@@ -842,7 +842,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);
_
Re: [PATCH 20/24] sys_mknodat(): elevate write count for vfs_mknod/create()
Quoted 8 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 tovfs_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.
Ok.
PATCH 21/24 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~elevate-mnt-writers-for-vfs-unlink-callers +4 -0
@@ -2253,7 +2253,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~elevate-mnt-writers-for-vfs-unlink-callers +4 -1
@@ -747,8 +747,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);
_
Re: [PATCH 21/24] elevate mnt writers for vfs_unlink() callers
Quoted 3 lines · Dave Hansen
Signed-off-by Dave Hansen <[email protected]>
Ok.
PATCH 22/24 do_rmdir(): elevate write count
In reply to Dave HansenElevate 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~do-rmdir-elevate-write-count +5 -0
@@ -2173,7 +2173,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);
_
Re: [PATCH 22/24] do_rmdir(): elevate write count
Quoted 2 lines · Dave Hansen
Elevate the write count during the
vfs_rmdir()call.
Ok.
Quoted 2 lines · Dave Hansen
If we can't pull the entire series into -mm, can we just put the first three patches for now? They can stand on their own.
FWIW this set (through 22) looks good to me.
thanks,
-serge
Quoted 2 lines · Dave Hansen
If we can't pull the entire series into -mm, can we just put the first three patches for now? They can stand on their own.
Yes, they're kinda a series of their own. But I still think we really want this in -mm. As we've seen on the kernel summit there's a pretty desparate need for it. And there's not many changes in this area in -mm, maybe the unprivilegued mounts. I'd personally prioritize the r/o bindmounts over them as they're more needed and we need more reviewing of the unprivilegued mounts (I'll try to come back to that soon).
FYI: the series you sent out are missing the last two patches, and while they compile as-is they're actually missing the interesting part :)
Andrew Morton
maintainerQuoted 10 lines · On Wed, 19 Sep 2007 18:44:18 +0100 Christoph Hellwig <[email protected]> wrote:
Quoted 2 lines · Dave Hansen
If we can't pull the entire series into -mm, can we just put the first three patches for now? They can stand on their own.
Yes, they're kinda a series of their own. But I still think we really want this in -mm. As we've seen on the kernel summit there's a pretty desparate need for it. And there's not many changes in this area in -mm, maybe the unprivilegued mounts. I'd personally prioritize the r/o bindmounts over them as they're more needed and we need more reviewing of the unprivilegued mounts (I'll try to come back to that soon).
What's the situation on unprivileged mounts? iirc, it's all a bit stuck.
If unpriv-mounts code isn't going to go into mainline ahead of r/o bind mounts then it'd make a big mess to prepare the r/o bind mount patches on top of unprivileged mounts.
It sounds like a better approach would be for me to merge the r/o bind mounts code and to drop (or maybe rework) the unprivileged mounts patches
Quoted 14 lines · Andrew Morton
Quoted 10 lines · On Wed, 19 Sep 2007 18:44:18 +0100 Christoph Hellwig <[email protected]> wrote:
Quoted 2 lines · Dave Hansen
If we can't pull the entire series into -mm, can we just put the first three patches for now? They can stand on their own.
Yes, they're kinda a series of their own. But I still think we really want this in -mm. As we've seen on the kernel summit there's a pretty desparate need for it. And there's not many changes in this area in -mm, maybe the unprivilegued mounts. I'd personally prioritize the r/o bindmounts over them as they're more needed and we need more reviewing of the unprivilegued mounts (I'll try to come back to that soon).
Quoted 8 lines
What's the situation on unprivileged mounts? iirc, it's all a bit stuck.
If unpriv-mounts code isn't going to go into mainline ahead of r/o bind mounts then it'd make a big mess to prepare the r/o bind mount patches on top of unprivileged mounts.
It sounds like a better approach would be for me to merge the r/o bind mounts code and to drop (or maybe rework) the unprivileged mounts patches
I actually don't think they collided too much. There were a couple of patches, like maybe 2 or 3 that needed any futzing at all.
I'll cook up a set straight on top of mainline if that helps.
-- Dave
Andrew Morton
maintainerQuoted 21 lines · On Wed, 19 Sep 2007 14:56:27 -0700 Dave Hansen <[email protected]> wrote:
Quoted 20 lines · Andrew Morton
Quoted 10 lines · On Wed, 19 Sep 2007 18:44:18 +0100 Christoph Hellwig <[email protected]> wrote:
Quoted 2 lines · Dave Hansen
If we can't pull the entire series into -mm, can we just put the first three patches for now? They can stand on their own.
Yes, they're kinda a series of their own. But I still think we really want this in -mm. As we've seen on the kernel summit there's a pretty desparate need for it. And there's not many changes in this area in -mm, maybe the unprivilegued mounts. I'd personally prioritize the r/o bindmounts over them as they're more needed and we need more reviewing of the unprivilegued mounts (I'll try to come back to that soon).
What's the situation on unprivileged mounts? iirc, it's all a bit stuck.
If unpriv-mounts code isn't going to go into mainline ahead of r/o bind mounts then it'd make a big mess to prepare the r/o bind mount patches on top of unprivileged mounts.
Quoted 8 lines
Quoted 2 lines
It sounds like a better approach would be for me to merge the r/o bind mounts code and to drop (or maybe rework) the unprivileged mounts patches
I actually don't think they collided too much. There were a couple of patches, like maybe 2 or 3 that needed any futzing at all.
I'll cook up a set straight on top of mainline if that helps.
That sounds good, thanks.
There may be collisions with unionfs too, but if that happens in a non-trivial way I may just drop unionfs - it doesn't look like it's going to get there in its present form.
Quoted 27 lines
Quoted 24 lines · On Wed, 19 Sep 2007 14:56:27 -0700 Dave Hansen <[email protected]> wrote:
Quoted 22 lines · Andrew Morton
Quoted 10 lines · On Wed, 19 Sep 2007 18:44:18 +0100 Christoph Hellwig <[email protected]> wrote:
Quoted 2 lines · Dave Hansen
If we can't pull the entire series into -mm, can we just put the first three patches for now? They can stand on their own.
Yes, they're kinda a series of their own. But I still think we really want this in -mm. As we've seen on the kernel summit there's a pretty desparate need for it. And there's not many changes in this area in -mm, maybe the unprivilegued mounts. I'd personally prioritize the r/o bindmounts over them as they're more needed and we need more reviewing of the unprivilegued mounts (I'll try to come back to that soon).
What's the situation on unprivileged mounts? iirc, it's all a bit stuck.
If unpriv-mounts code isn't going to go into mainline ahead of r/o bind mounts then it'd make a big mess to prepare the r/o bind mount patches on top of unprivileged mounts.
It sounds like a better approach would be for me to merge the r/o bind mounts code and to drop (or maybe rework) the unprivileged mounts patches
Quoted 7 lines
Quoted 5 lines
I actually don't think they collided too much. There were a couple of patches, like maybe 2 or 3 that needed any futzing at all.
I'll cook up a set straight on top of mainline if that helps.
That sounds good, thanks.
I agree, that per mount r/o is more important than unprivileged mounts. Especially because fixing up mount(8) and option showing in /proc/mounts is really nowhere yet, both of which are needed to make unprivileged mounting really useful.
So if that's convenient for you, just drop the unpriv mount patches, and I'll resubmit sometime later.
Miklos