Read-only bind mounts (-mm resend)
Subsystems (18)
PATCH 2/27 make open_namei() return a filp
In reply to Dave Hansenopen_namei() will, in the future, need to take mount write counts over its creation and truncation (via may_open()) operations. It needs to keep these write counts until any potential filp that is created gets __fput()'d.
This gets complicated in the error handling and becomes very murky as to how far open_namei() actually got, and whether or not that mount write count was taken.
Creating the filps inside of open_namei() lets us shift the write count to be taken and released along with the filp. We can hold a temporary write count during those creation and truncation operations, then release them once the write for the filp has been established.
Any caller who gets a 'struct file' back must consider that filp instantiated and fput() it normally. The callers no longer have to worry about ever manually releasing a mnt write count.
---
linux-2.6.git-dave/fs/namei.c | 16 ++++++++-------- linux-2.6.git-dave/fs/open.c | 7 +------ linux-2.6.git-dave/include/linux/fs.h | 2 +- 3 files changed, 10 insertions(+), 15 deletions(-)
fs/namei.c renamed from fs/namei.c~make-open_namei-return-a-filp +8 -8
@@ -1730,8 +1730,8 @@ static inline int sys_open_flags_to_name* system call. See sys_open_flags_to_namei_flags(). * SMP-safe */ -int open_namei (int dfd,const char *pathname,int sys_open_flag, -int mode,struct nameidata *nd) +struct file *open_namei (int dfd,const char *pathname,int sys_open_flag, +int mode,struct nameidata *nd) {int acc_mode, error;struct path path;
@@ -1757,7 +1757,7 @@ int open_namei(int dfd, const char *path error =path_lookup_open (dfd, pathname,lookup_flags (flag), nd, flag);if (error) -return error; +return ERR_PTR (error); goto ok; }
@@ -1766,7 +1766,7 @@ int open_namei(int dfd, const char *path*/ error =path_lookup_create (dfd,pathname,LOOKUP_PARENT ,nd,flag,mode);if (error) -return error; +return ERR_PTR (error);/* * We have the parent and last component. First of all, check
@@ -1801,7 +1801,7 @@ do_last: error =__open_namei_create (nd, &path, flag, mode);if (error) goto exit; -return 0 ; +return nameidata_to_filp (nd, sys_open_flag); }/*
@@ -1834,7 +1834,7 @@ ok: error =may_open (nd, acc_mode, flag);if (error) goto exit; -return 0 ; +return nameidata_to_filp (nd, sys_open_flag); exit_dput:dput_path (&path, nd);
@@ -1842,7 +1842,7 @@ exit:if (!IS_ERR (nd->intent.open.file))release_open_intent (nd);path_release (nd); -return error; +return ERR_PTR (error); do_link: error = -ELOOP ;
@@ -1869,7 +1869,7 @@ do_link:* with "intent.open". */ release_open_intent (nd); -return error; +return ERR_PTR (error); } nd->flags &= ~LOOKUP_PARENT ;if (nd->last_type ==LAST_BIND )
fs/open.c renamed from fs/open.c~make-open_namei-return-a-filp +1 -6
@@ -803,14 +803,9 @@ cleanup_file:static struct file *do_filp_open (int dfd,const char *filename,int flags,int mode) { -int error;struct nameidata nd; - error =open_namei (dfd, filename, flags, mode, &nd); -if (!error) -return nameidata_to_filp (&nd, flags); - -return ERR_PTR (error); +return open_namei (dfd, filename, flags, mode, &nd); }struct file *filp_open (const char *filename,int flags,int mode)
include/linux/fs.h renamed from include/linux/fs.h~make-open_namei-return-a-filp +1 -1
@@ -1714,7 +1714,7 @@ extern struct file *create_read_pipe(strextern struct file *create_write_pipe (void );extern void free_write_pipe (struct file *); -extern int open_namei (int dfd,const char *,int ,int ,struct nameidata *); +extern struct file *open_namei (int dfd,const char *,int ,int ,struct nameidata *);extern int may_open (struct nameidata *,int ,int );extern int kernel_read (struct file *,unsigned long ,char *,unsigned long );
_
PATCH 3/27 kill do_filp_open()
In reply to Dave HansenThis kills off the almost empty do_filp_open(). The indenting change in do_sys_open() is because we would have gone over our 80 characters otherwise.
---
linux-2.6.git-dave/fs/open.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-)
fs/open.c renamed from fs/open.c~kill-do_filp_open +18 -21
@@ -800,17 +800,10 @@ cleanup_file:return ERR_PTR (error); } -static struct file *do_filp_open (int dfd,const char *filename,int flags, -int mode) -{ -struct nameidata nd; - -return open_namei (dfd, filename, flags, mode, &nd); -} -struct file *filp_open (const char *filename,int flags,int mode) { -return do_filp_open (AT_FDCWD , filename, flags, mode); +struct nameidata nd; +return open_namei (AT_FDCWD , filename, flags, mode, &nd); }EXPORT_SYMBOL (filp_open);
@@ -1009,20 +1002,24 @@ long do_sys_open(int dfd, const char __uchar *tmp =getname (filename);int fd =PTR_ERR (tmp); -if (!IS_ERR (tmp)) { - fd =get_unused_fd_flags (flags); -if (fd >=0 ) { -struct file *f =do_filp_open (dfd, tmp, flags, mode); -if (IS_ERR (f)) { -put_unused_fd (fd); - fd =PTR_ERR (f); - }else { -fsnotify_open (f->f_path.dentry); -fd_install (fd, f); - } +if (IS_ERR (tmp)) + goto out; + + fd =get_unused_fd_flags (flags); +if (fd >=0 ) { +struct nameidata nd; +struct file *f =open_namei (dfd, tmp, flags, mode, &nd); + +if (IS_ERR (f)) { +put_unused_fd (fd); + fd =PTR_ERR (f); + }else { +fsnotify_open (f->f_path.dentry); +fd_install (fd, f); } -putname (tmp); } +putname (tmp); +out:return fd; }
_
PATCH 4/27 kill filp_open()
In reply to Dave HansenReplace all callers with open_namei() directly, and move the nameidata stack allocation into open_namei().
---
linux-2.6.git-dave/drivers/usb/gadget/file_storage.c | 5 - linux-2.6.git-dave/fs/exec.c | 2 linux-2.6.git-dave/fs/namei.c | 71 +++++++++---------- linux-2.6.git-dave/fs/nfsctl.c | 5 + linux-2.6.git-dave/fs/open.c | 6 - linux-2.6.git-dave/fs/reiserfs/journal.c | 2 linux-2.6.git-dave/include/linux/fs.h | 3 linux-2.6.git-dave/kernel/acct.c | 2 linux-2.6.git-dave/mm/swapfile.c | 4 - linux-2.6.git-dave/sound/sound_firmware.c | 2 10 files changed, 53 insertions(+), 49 deletions(-)
drivers/usb/gadget/file_storage.c renamed from drivers/usb/gadget/file_storage.c~kill-filp_open +3 -2
@@ -3468,16 +3468,17 @@ static int open_backing_file(struct lunstruct inode *inode =NULL ;loff_t size;loff_t num_sectors; +int mode =O_LARGEFILE ;/* R/W if we can, R/O if we must */ ro = curlun->ro;if (!ro) { - filp =filp_open (filename,O_RDWR |O_LARGEFILE ,0 ); + filp =open_namei (AT_FDCWD , filename,O_RDWR | mode,0 );if (-EROFS ==PTR_ERR (filp)) ro =1 ; }if (ro) - filp =filp_open (filename,O_RDONLY |O_LARGEFILE ,0 ); + filp =open_namei (AT_FDCWD , filename,O_RDONLY | mode,0 );if (IS_ERR (filp)) {LINFO (curlun,"unable to open backing file: %s\n" , filename);return PTR_ERR (filp);
fs/exec.c renamed from fs/exec.c~kill-filp_open +1 -1
@@ -1763,7 +1763,7 @@ int do_coredump(long signr, int exit_cod goto fail_unlock; } }else - file =filp_open (corename, + file =open_namei (AT_FDCWD , corename,O_CREAT |2 |O_NOFOLLOW |O_LARGEFILE | flag,0600 );if (IS_ERR (file))
fs/namei.c renamed from fs/namei.c~kill-filp_open +36 -35
155 lines not on this page · raw message
fs/nfsctl.c renamed from fs/nfsctl.c~kill-filp_open +5 -0
@@ -35,6 +35,11 @@ static struct file *do_open(char *name,if (error)return ERR_PTR (error); +/* +* If O_TRUNC is ever passed into may_open() +* it will be necessary to take and release +* mnt writer count around these calls. +*/ if (flags ==O_RDWR ) error =may_open (&nd,MAY_READ |MAY_WRITE ,FMODE_READ |FMODE_WRITE );else
fs/open.c renamed from fs/open.c~kill-filp_open +2 -4
@@ -802,8 +802,7 @@ cleanup_file:struct file *filp_open (const char *filename,int flags,int mode) { -struct nameidata nd; -return open_namei (AT_FDCWD , filename, flags, mode, &nd); +return open_namei (AT_FDCWD , filename, flags, mode); }EXPORT_SYMBOL (filp_open);
@@ -1007,8 +1006,7 @@ long do_sys_open(int dfd, const char __u fd =get_unused_fd_flags (flags);if (fd >=0 ) { -struct nameidata nd; -struct file *f =open_namei (dfd, tmp, flags, mode, &nd); +struct file *f =open_pathname (dfd, tmp, flags, mode);if (IS_ERR (f)) {put_unused_fd (fd);
fs/reiserfs/journal.c renamed from fs/reiserfs/journal.c~kill-filp_open +1 -1
@@ -2625,7 +2625,7 @@ static int journal_init_dev(struct superreturn 0 ; } - journal->j_dev_file =filp_open (jdev_name,0 ,0 ); + journal->j_dev_file =open_namei (AT_FDCWD , jdev_name,0 ,0 );if (!IS_ERR (journal->j_dev_file)) {struct inode *jdev_inode = journal->j_dev_file->f_mapping->host;if (!S_ISBLK (jdev_inode->i_mode)) {
include/linux/fs.h renamed from include/linux/fs.h~kill-filp_open +1 -2
@@ -1540,7 +1540,6 @@ extern int do_truncate(struct dentry *, struct file *filp);extern long do_sys_open (int dfd,const char __user *filename,int flags,int mode); -extern struct file *filp_open (const char *,int ,int );extern struct file *dentry_open (struct dentry *,struct vfsmount *,int );extern int filp_close (struct file *,fl_owner_t id);extern char *getname (const char __user *);
@@ -1714,7 +1713,7 @@ extern struct file *create_read_pipe(strextern struct file *create_write_pipe (void );extern void free_write_pipe (struct file *); -extern struct file *open_namei (int dfd,const char *,int ,int ,struct nameidata *); +extern struct file *open_pathname (int dfd,const char *,int ,int );extern int may_open (struct nameidata *,int ,int );extern int kernel_read (struct file *,unsigned long ,char *,unsigned long );
kernel/acct.c renamed from kernel/acct.c~kill-filp_open +1 -1
@@ -208,7 +208,7 @@ static int acct_on(char *name)int error;/* Difference from BSD - they don't do O_APPEND */ - file =filp_open (name,O_WRONLY |O_APPEND |O_LARGEFILE ,0 ); + file =open_namei (AT_FDCWD , name,O_WRONLY |O_APPEND |O_LARGEFILE ,0 );if (IS_ERR (file))return PTR_ERR (file);
mm/swapfile.c renamed from mm/swapfile.c~kill-filp_open +2 -2
@@ -1191,7 +1191,7 @@ asmlinkage long sys_swapoff(const char _if (IS_ERR (pathname)) goto out; - victim =filp_open (pathname,O_RDWR |O_LARGEFILE ,0 ); + victim =open_namei (AT_FDCWD , pathname,O_RDWR |O_LARGEFILE ,0 );putname (pathname); err =PTR_ERR (victim);if (IS_ERR (victim))
@@ -1470,7 +1470,7 @@ asmlinkage long sys_swapon(const char __ name =NULL ; goto bad_swap_2; } - swap_file =filp_open (name,O_RDWR |O_LARGEFILE ,0 ); + swap_file =open_namei (AT_FDCWD , name,O_RDWR |O_LARGEFILE ,0 ); error =PTR_ERR (swap_file);if (IS_ERR (swap_file)) { swap_file =NULL ;
sound/sound_firmware.c renamed from sound/sound_firmware.c~kill-filp_open +1 -1
@@ -14,7 +14,7 @@ static int do_mod_firmware_load(const chchar *dp;loff_t pos; - filp =filp_open (fn,0 ,0 ); + filp =open_namei (AT_FDCWD , fn,0 ,0 );if (IS_ERR (filp)) {printk (KERN_INFO "Unable to load '%s'.\n" , fn);
_
PATCH 5/27 rename open_namei() to open_pathname()
In reply to Dave Hansenopen_namei() no longer touches namei's. rename it to something more appropriate: open_pathname().
---
linux-2.6.git-dave/drivers/usb/gadget/file_storage.c | 4 ++-- linux-2.6.git-dave/fs/exec.c | 2 +- linux-2.6.git-dave/fs/namei.c | 8 ++++---- linux-2.6.git-dave/fs/open.c | 6 ------ linux-2.6.git-dave/fs/reiserfs/journal.c | 2 +- linux-2.6.git-dave/kernel/acct.c | 2 +- linux-2.6.git-dave/mm/swapfile.c | 4 ++-- linux-2.6.git-dave/sound/sound_firmware.c | 2 +- 8 files changed, 12 insertions(+), 18 deletions(-)
drivers/usb/gadget/file_storage.c renamed from drivers/usb/gadget/file_storage.c~rename-open_namei +2 -2
@@ -3473,12 +3473,12 @@ static int open_backing_file(struct lun/* R/W if we can, R/O if we must */ ro = curlun->ro;if (!ro) { - filp =open_namei (AT_FDCWD , filename,O_RDWR | mode,0 ); + filp =open_pathname (AT_FDCWD , filename,O_RDWR | mode,0 );if (-EROFS ==PTR_ERR (filp)) ro =1 ; }if (ro) - filp =open_namei (AT_FDCWD , filename,O_RDONLY | mode,0 ); + filp =open_pathname (AT_FDCWD , filename,O_RDONLY | mode,0 );if (IS_ERR (filp)) {LINFO (curlun,"unable to open backing file: %s\n" , filename);return PTR_ERR (filp);
fs/exec.c renamed from fs/exec.c~rename-open_namei +1 -1
@@ -1763,7 +1763,7 @@ int do_coredump(long signr, int exit_cod goto fail_unlock; } }else - file =open_namei (AT_FDCWD , corename, + file =open_pathname (AT_FDCWD , corename,O_CREAT |2 |O_NOFOLLOW |O_LARGEFILE | flag,0600 );if (IS_ERR (file))
fs/namei.c renamed from fs/namei.c~rename-open_namei +4 -4
@@ -81,7 +81,7 @@*/ /* [16-Dec-97 Kevin Buhr] For security reasons, we change some symlink -* semantics. See the comments in "open_namei" and "do_link" below. +* semantics. See the comments in "open_pathname" and "do_link" below. * * [10-Sep-98 Alan Modra] Another symlink change. */
@@ -571,7 +571,7 @@ out:if (nd->depth || res || nd->last_type!=LAST_NORM )return res;/* -* If it is an iterative symlinks resolution in open_namei() we +* If it is an iterative symlinks resolution in open_pathname() we * have to copy the last component. And all that crap because of * bloody create() on broken symlinks. Furrfu... */
@@ -1708,7 +1708,7 @@ static int __open_namei_create(struct na* 01 - read-permission * 10 - write-permission * 11 - read-write -* for the internal routines (ie open_namei()/follow_link() etc) +* for the internal routines (ie open_pathname()/follow_link() etc) * This is more logical, and also allows the 00 "no perm needed" * to be used for symlinks (where the permissions are checked * later).
@@ -1722,7 +1722,7 @@ static inline int sys_open_flags_to_name }/* -* open_namei() +* open_pathname() * * namei for open - this is in fact almost the whole open-routine. *
fs/open.c renamed from fs/open.c~rename-open_namei +0 -6
@@ -800,12 +800,6 @@ cleanup_file:return ERR_PTR (error); } -struct file *filp_open (const char *filename,int flags,int mode) -{ -return open_namei (AT_FDCWD , filename, flags, mode); -} -EXPORT_SYMBOL (filp_open); -/** * lookup_instantiate_filp - instantiates the open intent filp * @nd: pointer to nameidata
fs/reiserfs/journal.c renamed from fs/reiserfs/journal.c~rename-open_namei +1 -1
@@ -2625,7 +2625,7 @@ static int journal_init_dev(struct superreturn 0 ; } - journal->j_dev_file =open_namei (AT_FDCWD , jdev_name,0 ,0 ); + journal->j_dev_file =open_pathname (AT_FDCWD , jdev_name,0 ,0 );if (!IS_ERR (journal->j_dev_file)) {struct inode *jdev_inode = journal->j_dev_file->f_mapping->host;if (!S_ISBLK (jdev_inode->i_mode)) {
kernel/acct.c renamed from kernel/acct.c~rename-open_namei +1 -1
@@ -208,7 +208,7 @@ static int acct_on(char *name)int error;/* Difference from BSD - they don't do O_APPEND */ - file =open_namei (AT_FDCWD , name,O_WRONLY |O_APPEND |O_LARGEFILE ,0 ); + file =open_pathname (AT_FDCWD , name,O_WRONLY |O_APPEND |O_LARGEFILE ,0 );if (IS_ERR (file))return PTR_ERR (file);
mm/swapfile.c renamed from mm/swapfile.c~rename-open_namei +2 -2
@@ -1191,7 +1191,7 @@ asmlinkage long sys_swapoff(const char _if (IS_ERR (pathname)) goto out; - victim =open_namei (AT_FDCWD , pathname,O_RDWR |O_LARGEFILE ,0 ); + victim =open_pathname (AT_FDCWD , pathname,O_RDWR |O_LARGEFILE ,0 );putname (pathname); err =PTR_ERR (victim);if (IS_ERR (victim))
@@ -1470,7 +1470,7 @@ asmlinkage long sys_swapon(const char __ name =NULL ; goto bad_swap_2; } - swap_file =open_namei (AT_FDCWD , name,O_RDWR |O_LARGEFILE ,0 ); + swap_file =open_pathname (AT_FDCWD , name,O_RDWR |O_LARGEFILE ,0 ); error =PTR_ERR (swap_file);if (IS_ERR (swap_file)) { swap_file =NULL ;
sound/sound_firmware.c renamed from sound/sound_firmware.c~rename-open_namei +1 -1
@@ -14,7 +14,7 @@ static int do_mod_firmware_load(const chchar *dp;loff_t pos; - filp =open_namei (AT_FDCWD , fn,0 ,0 ); + filp =open_pathname (AT_FDCWD , fn,0 ,0 );if (IS_ERR (filp)) {printk (KERN_INFO "Unable to load '%s'.\n" , fn);
_
PATCH 8/27 r-o-bind-mounts-elevate-mnt-writers-for-callers-of-vfs_mkdir
In reply to Dave HansenPretty self-explanatory. Fits in with the rest of the series.
---
linux-2.6.git-dave/fs/namei.c | 5 +++++ linux-2.6.git-dave/fs/nfsd/nfs4recover.c | 5 +++++ 2 files changed, 10 insertions(+)
fs/namei.c renamed from fs/namei.c~r-o-bind-mounts-elevate-mnt-writers-for-callers-of-vfs_mkdir +5 -0
@@ -2067,7 +2067,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~r-o-bind-mounts-elevate-mnt-writers-for-callers-of-vfs_mkdir +5 -0
@@ -41,6 +41,7 @@#include <linux/nfsd/xdr4.h> #include <linux/param.h> #include <linux/file.h> +#include <linux/mount.h> #include <linux/namei.h> #include <asm/uaccess.h> #include <asm/scatterlist.h>
@@ -154,7 +155,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:
_
cover Read-only bind mounts (-mm resend)
This is against 2.6.24-rc1 + recent git.
I've integrated all of the fixes from mm, and included cleanups in a different order. This also includes some extra fput-time checking to ensure that we have balanced mount writer counts.
These replace the patches in -mm mostly because the new fixes require some cleanups that are functionally independent from the r/o bind mount code itself. These fixes precent the rest of the set and require some by hand merging.
If you're going to review one and only one patch, 17/27 (the one for "opend" files) is the most critical.
---
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 are 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 this script:
http://sr71.net/~dave/linux/robind-test.sh
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 6/27 r-o-bind-mounts-stub-functions
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.
---
linux-2.6.git-dave/fs/namespace.c | 54 +++++++++++++++++++++++++++++++ linux-2.6.git-dave/include/linux/mount.h | 3 + 2 files changed, 57 insertions(+)
fs/namespace.c renamed from fs/namespace.c~r-o-bind-mounts-stub-functions +54 -0
@@ -77,6 +77,60 @@ 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. +*/ +/** +* mnt_want_write - get write access to a mount +* @mnt: the mount on which to take a write +* +* 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); + +/** +* mnt_drop_write - give up write access to a mount +* @mnt: the mount on which to give up write access +* +* Tells the low-level filesystem that we are done +* performing writes to it. Must be matched with +* mnt_want_write() call above. +*/ +void mnt_drop_write (struct vfsmount *mnt) +{ +} +EXPORT_SYMBOL_GPL (mnt_drop_write); + +/* +* __mnt_is_readonly: check whether a mount is read-only +* @mnt: the mount to check for its write status +* +* 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*. This can not and +* should not be used in place of IS_RDONLY(inode). +*/ +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~r-o-bind-mounts-stub-functions +3 -0
@@ -70,9 +70,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) {
_
PATCH 9/27 r-o-bind-mounts-elevate-mnt-writers-for-vfs_unlink-callers
In reply to Dave Hansen---
linux-2.6.git-dave/fs/namei.c | 4 ++++ linux-2.6.git-dave/ipc/mqueue.c | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-)
fs/namei.c renamed from fs/namei.c~r-o-bind-mounts-elevate-mnt-writers-for-vfs_unlink-callers +4 -0
@@ -2264,7 +2264,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~r-o-bind-mounts-elevate-mnt-writers-for-vfs_unlink-callers +4 -1
@@ -743,8 +743,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);
_
PATCH 10/27 r-o-bind-mounts-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.
---
linux-2.6.git-dave/fs/nfsd/nfs4proc.c | 7 ++++++- linux-2.6.git-dave/fs/xattr.c | 16 ++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-)
fs/nfsd/nfs4proc.c renamed from fs/nfsd/nfs4proc.c~r-o-bind-mounts-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~r-o-bind-mounts-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; }
_
PATCH 1/27 do namei_flags calculation inside open_namei()
In reply to Dave HansenMy end goal here is to make sure all users of may_open() return filps. This will ensure that we properly release mount write counts which were taken for the filp in may_open().
This patch moves the sys_open flags to namei flags calculation into fs/namei.c. We'll shortly be moving the nameidata_to_filp() calls into namei.c, and this gets the sys_open flags to a place where we can get at them when we need them.
---
linux-2.6.git-dave/fs/namei.c | 43 +++++++++++++++++++++++++++++++++--------- linux-2.6.git-dave/fs/open.c | 22 +-------------------- 2 files changed, 36 insertions(+), 29 deletions(-)
fs/namei.c renamed from fs/namei.c~do-namei_flags-calculation-inside-open_namei +34 -9
@@ -1674,7 +1674,12 @@ int may_open(struct nameidata *nd, int areturn 0 ; } -static int open_namei_create (struct nameidata *nd,struct path *path, +/* +* Be careful about ever adding any more callers of this +* function. Its flags must be in the namei format, not +* what get passed to sys_open(). +*/ +static int __open_namei_create (struct nameidata *nd,struct path *path,int flag,int mode) {int error;
@@ -1693,26 +1698,46 @@ static int open_namei_create(struct name }/* +* Note that while the flag value (low two bits) for sys_open means: +* 00 - read-only +* 01 - write-only +* 10 - read-write +* 11 - special +* it is changed into +* 00 - no permissions needed +* 01 - read-permission +* 10 - write-permission +* 11 - read-write +* for the internal routines (ie open_namei()/follow_link() etc) +* This is more logical, and also allows the 00 "no perm needed" +* to be used for symlinks (where the permissions are checked +* later). +* +*/ +static inline int sys_open_flags_to_namei_flags (int flag) +{ +if ((flag+1 ) &O_ACCMODE ) + flag++; +return flag; +} + +/* * open_namei() * * namei for open - this is in fact almost the whole open-routine. * * Note that the low bits of "flag" aren't the same as in the open -* system call - they are 00 - no permissions needed -* 01 - read permission needed -* 10 - write permission needed -* 11 - read/write permissions needed -* which is a lot more logical, and also allows the "no perm" needed -* for symlinks (where the permissions are checked later). +* system call. See sys_open_flags_to_namei_flags(). * SMP-safe */ -int open_namei (int dfd,const char *pathname,int flag, +int open_namei (int dfd,const char *pathname,int sys_open_flag,int mode,struct nameidata *nd) {int acc_mode, error;struct path path;struct dentry *dir;int count =0 ; +int flag =sys_open_flags_to_namei_flags (sys_open_flag); acc_mode =ACC_MODE (flag);
@@ -1773,7 +1798,7 @@ do_last:/* Negative dentry, just create the file */ if (!path.dentry->d_inode) { - error =open_namei_create (nd, &path, flag, mode); + error =__open_namei_create (nd, &path, flag, mode);if (error) goto exit;return 0 ;
fs/open.c renamed from fs/open.c~do-namei_flags-calculation-inside-open_namei +2 -20
@@ -800,31 +800,13 @@ cleanup_file:return ERR_PTR (error); } -/* -* Note that while the flag value (low two bits) for sys_open means: -* 00 - read-only -* 01 - write-only -* 10 - read-write -* 11 - special -* it is changed into -* 00 - no permissions needed -* 01 - read-permission -* 10 - write-permission -* 11 - read-write -* for the internal routines (ie open_namei()/follow_link() etc). 00 is -* used by symlinks. -*/ static struct file *do_filp_open (int dfd,const char *filename,int flags,int mode) { -int namei_flags, error; +int error;struct nameidata nd; - namei_flags = flags; -if ((namei_flags+1 ) &O_ACCMODE ) - namei_flags++; - - error =open_namei (dfd, filename, namei_flags, mode, &nd); + error =open_namei (dfd, filename, flags, mode, &nd);if (!error)return nameidata_to_filp (&nd, flags);
_
PATCH 7/27 r-o-bind-mounts-do_rmdir-elevate-write-count
In reply to Dave HansenElevate the write count during the vfs_rmdir() call.
---
linux-2.6.git-dave/fs/namei.c | 5 +++++ 1 file changed, 5 insertions(+)
fs/namei.c renamed from fs/namei.c~r-o-bind-mounts-do_rmdir-elevate-write-count +5 -0
@@ -2174,7 +2174,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);
_
PATCH 13/27 r-o-bind-mounts-elevate-write-count-for-do_utimes
In reply to Dave HansenNow includes fix for oops seen by akpm.
"never let a libc developer write your kernel code" - hch
"nor, apparently, a kernel developer" - akpm
---
linux-2.6.git-dave/fs/utimes.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
fs/utimes.c renamed from fs/utimes.c~r-o-bind-mounts-elevate-write-count-for-do_utimes +12 -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>
@@ -58,6 +59,7 @@ long do_utimes(int dfd, char __user *filstruct inode *inode;struct iattr newattrs;struct file *f =NULL ; +struct vfsmount *mnt; error = -EINVAL ; if (times && (!nsec_valid(times[0].tv_nsec) ||
@@ -78,18 +80,20 @@ long do_utimes(int dfd, char __user *filif (!f) goto out; dentry = f->f_path.dentry; + mnt = f->f_path.mnt; }else { error =__user_walk_fd (dfd, filename, (flags &AT_SYMLINK_NOFOLLOW ) ?0 :LOOKUP_FOLLOW , &nd);if (error) goto out; dentry = nd.dentry; + mnt = nd.mnt; } inode = dentry->d_inode; - error = -EROFS ; -if (IS_RDONLY (inode)) + error =mnt_want_write (mnt); +if (error) goto dput_and_out;/* Don't worry, the checks are done in inode_change_ok() */
@@ -97,7 +101,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 +121,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 (mnt); dput_and_out:if (f)fput (f);
_
PATCH 14/27 r-o-bind-mounts-elevate-write-count-for-file_update_time
In reply to Dave Hansen---
linux-2.6.git-dave/fs/inode.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
fs/inode.c renamed from fs/inode.c~r-o-bind-mounts-elevate-write-count-for-file_update_time +12 -1
@@ -1263,10 +1263,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);
@@ -1282,6 +1291,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);
_
PATCH 11/27 r-o-bind-mounts-elevate-write-count-during-entire-ncp_ioctl
In reply to Dave Hansenfs/ncpfs/ioctl.c: In function 'ncp_ioctl_need_write':fs/ncpfs/ioctl.c:852: error: label at end of compound statement
---
linux-2.6.git-dave/fs/ncpfs/ioctl.c | 57 ++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 2 deletions(-)
fs/ncpfs/ioctl.c renamed from fs/ncpfs/ioctl.c~r-o-bind-mounts-elevate-write-count-during-entire-ncp_ioctl +55 -2
@@ -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);
@@ -817,11 +818,63 @@ outrel:return -EFAULT ;return 0 ; } - +default :/* unkown IOCTL command, assume write */ + ; }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 */ + } +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) {
_
PATCH 15/27 r-o-bind-mounts-elevate-write-count-for-link-and-symlink-calls
In reply to Dave Hansen---
linux-2.6.git-dave/fs/namei.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
fs/namei.c renamed from fs/namei.c~r-o-bind-mounts-elevate-write-count-for-link-and-symlink-calls +10 -0
@@ -2349,7 +2349,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);
@@ -2444,7 +2449,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);
_
PATCH 12/27 r-o-bind-mounts-elevate-write-count-for-do_sys_utime-and-touch_atime
In reply to Dave Hansen---
linux-2.6.git-dave/fs/inode.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
fs/inode.c renamed from fs/inode.c~r-o-bind-mounts-elevate-write-count-for-do_sys_utime-and-touch_atime +12 -8
@@ -1203,22 +1203,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
@@ -1229,16 +1230,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);
_
PATCH 16/27 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.
---
linux-2.6.git-dave/fs/ext2/ioctl.c | 46 ++++++---- linux-2.6.git-dave/fs/ext3/ioctl.c | 100 +++++++++++++++------- linux-2.6.git-dave/fs/ext4/ioctl.c | 105 +++++++++++++++--------- linux-2.6.git-dave/fs/fat/file.c | 10 +- linux-2.6.git-dave/fs/hfsplus/ioctl.c | 40 +++++---- linux-2.6.git-dave/fs/jfs/ioctl.c | 33 ++++--- linux-2.6.git-dave/fs/ocfs2/ioctl.c | 11 +- linux-2.6.git-dave/fs/reiserfs/ioctl.c | 55 ++++++++---- linux-2.6.git-dave/fs/xfs/linux-2.6/xfs_ioctl.c | 15 ++- linux-2.6.git-dave/fs/xfs/linux-2.6/xfs_iops.c | 7 - linux-2.6.git-dave/fs/xfs/linux-2.6/xfs_lrw.c | 9 +- 11 files changed, 274 insertions(+), 157 deletions(-)
fs/ext2/ioctl.c renamed from fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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>
@@ -20,6 +21,7 @@int ext2_ioctl (struct inode * inode,struct file * filp,unsigned int cmd,unsigned long arg) { +int ret;struct ext2_inode_info *ei =EXT2_I (inode);unsigned int flags;unsigned short rsv_window_size;
@@ -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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +66 -34
193 lines not on this page · raw message
fs/ext4/ioctl.c renamed from fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls +67 -38
202 lines not on this page · raw message
fs/fat/file.c renamed from fs/fat/file.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +25 -15
@@ -14,6 +14,7 @@#include <linux/capability.h> #include <linux/fs.h> +#include <linux/mount.h> #include <linux/sched.h> #include <linux/xattr.h> #include <asm/uaccess.h>
@@ -35,25 +36,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 +83,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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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 ;
@@ -834,7 +839,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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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~r-o-bind-mounts-elevate-write-count-for-some-ioctls +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); }/*
_
PATCH 17/27 r-o-bind-mounts-elevate-write-count-opend-files
In reply to Dave HansenThis is an old patch combined with a couple other ones I've been working on. It should the issues that Miklos has spotted.
--
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.
We used to do this in may_open(), but Miklos pointed out that __dentry_open() is used as well to create filps. This will cover even those cases, while a call in may_open() would not have.
There is also an elevated count around the vfs_create() call in open_namei(). See the comments for more details, but we need this to fix a 'create, remount, fail r/w open()' race.
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.
---
linux-2.6.git-dave/fs/file_table.c | 16 +++++++- linux-2.6.git-dave/fs/namei.c | 74 ++++++++++++++++++++++++++++++++----- linux-2.6.git-dave/fs/open.c | 36 +++++++++++++++++- linux-2.6.git-dave/ipc/mqueue.c | 3 + 4 files changed, 116 insertions(+), 13 deletions(-)
fs/file_table.c renamed from fs/file_table.c~r-o-bind-mounts-elevate-write-count-opend-files +15 -1
@@ -193,6 +193,17 @@ int init_file(struct file *file, struct file->f_mapping = dentry->d_inode->i_mapping; file->f_mode = mode; file->f_op = fop; + +/* +* These mounts don't really matter in practice +* for r/o bind mounts. They aren't userspace- +* visible. We do this for consistency, and so +* that we can do debugging checks at __fput()e +*/ +if ((mode &FMODE_WRITE ) && !special_file (dentry->d_inode->i_mode)) { + error =mnt_want_write (mnt); +WARN_ON (error); + }return error; }EXPORT_SYMBOL (init_file);
@@ -230,8 +241,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~r-o-bind-mounts-elevate-write-count-opend-files +64 -10
122 lines not on this page · raw message
fs/open.c renamed from fs/open.c~r-o-bind-mounts-elevate-write-count-opend-files +34 -2
@@ -734,6 +734,35 @@ out:return error; } +/* +* You have to be very careful that these write +* counts get cleaned up in error cases and +* upon __fput(). This should probably never +* be called outside of __dentry_open(). +*/ +static inline int __get_file_write_access (struct inode *inode, +struct vfsmount *mnt) +{ +int error; + error =get_write_access (inode); +if (error) +return error; +/* +* Do not take mount writer counts on +* special files since no writes to +* the mount itself will occur. +*/ +if (!special_file (inode->i_mode)) { +/* +* Balanced in __fput() +*/ + error =mnt_want_write (mnt); +if (error) +put_write_access (inode); + } +return error; +} +static struct file *__dentry_open (struct dentry *dentry,struct vfsmount *mnt,int flags,struct file *f,int (*open)(struct inode *,struct file *))
@@ -746,7 +775,7 @@ static struct file *__dentry_open(structFMODE_PREAD |FMODE_PWRITE ; inode = dentry->d_inode;if (f->f_mode &FMODE_WRITE ) { - error =get_write_access (inode); + error =__get_file_write_access (inode, mnt);if (error) goto cleanup_file; }
@@ -788,8 +817,11 @@ static struct file *__dentry_open(struct cleanup_all:fops_put (f->f_op); -if (f->f_mode &FMODE_WRITE ) +if (f->f_mode &FMODE_WRITE ) {put_write_access (inode); +if (!special_file (inode->i_mode)) +mnt_drop_write (mnt); + }file_kill (f); f->f_path.dentry =NULL ; f->f_path.mnt =NULL ;
ipc/mqueue.c renamed from ipc/mqueue.c~r-o-bind-mounts-elevate-write-count-opend-files +3 -0
@@ -682,6 +682,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); }
_
PATCH 18/27 r-o-bind-mounts-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.
---
linux-2.6.git-dave/fs/namei.c | 4 ++++ linux-2.6.git-dave/fs/nfsd/vfs.c | 15 +++++++++++---- 2 files changed, 15 insertions(+), 4 deletions(-)
fs/namei.c renamed from fs/namei.c~r-o-bind-mounts-elevate-write-count-over-calls-to-vfs_rename +4 -0
@@ -2734,8 +2734,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~r-o-bind-mounts-elevate-write-count-over-calls-to-vfs_rename +11 -4
@@ -1668,13 +1668,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);
_
PATCH 20/27 r-o-bind-mounts-elevate-writer-count-for-do_sys_truncate
In reply to Dave Hansen---
linux-2.6.git-dave/fs/open.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
fs/open.c renamed from fs/open.c~r-o-bind-mounts-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:
_
PATCH 21/27 r-o-bind-mounts-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.
---
linux-2.6.git-dave/fs/open.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
fs/open.c renamed from fs/open.c~r-o-bind-mounts-make-access-use-mnt-check +11 -2
@@ -459,8 +459,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:
_
PATCH 19/27 r-o-bind-mounts-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.
This conflicts with the current (~2.6.23-rc7) audit git tree in -mm. wiggle'ing the patch merges it.
---
linux-2.6.git-dave/fs/open.c | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-)
fs/open.c renamed from fs/open.c~r-o-bind-mounts-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;
_
PATCH 23/27 r-o-bind-mounts-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 and into a helper function suggested by Christoph.
This also encapsulates a fix for mknod(S_IFREG) that Miklos found.
---
linux-2.6.git-dave/fs/namei.c | 43 +++++++++++++++++++++++++--------- linux-2.6.git-dave/fs/nfsd/vfs.c | 4 +++ linux-2.6.git-dave/net/unix/af_unix.c | 4 +++ 3 files changed, 40 insertions(+), 11 deletions(-)
fs/namei.c renamed from fs/namei.c~r-o-bind-mounts-sys_mknodat-elevate-write-count-for-vfs_mknod-create +32 -11
@@ -2022,6 +2022,23 @@ int vfs_mknod(struct inode *dir, structreturn error; } +static int may_mknod (mode_t mode) +{ +switch (mode &S_IFMT ) { +case S_IFREG : +case S_IFCHR : +case S_IFBLK : +case S_IFIFO : +case S_IFSOCK : +case 0 :/* zero mode translates to S_IFREG */ +return 0 ; +case S_IFDIR : +return -EPERM ; +default : +return -EINVAL ; + } +} +asmlinkage long sys_mknodat (int dfd,const char __user *filename,int mode,unsigned dev) {
@@ -2040,12 +2057,19 @@ 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 ) { + error =may_mknod (mode); +if (error) + 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 ;
@@ -2056,14 +2080,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~r-o-bind-mounts-sys_mknodat-elevate-write-count-for-vfs_mknod-create +4 -0
@@ -1242,7 +1242,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~r-o-bind-mounts-sys_mknodat-elevate-write-count-for-vfs_mknod-create +4 -0
@@ -838,7 +838,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);
_
PATCH 24/27 r-o-bind-mounts-track-number-of-mount-writers
In reply to Dave HansenThis is the real meat of the entire series. It actually implements the tracking of the number of writers to a mount. However, it causes scalability problems because there can be hundreds of cpus doing open()/close() on files on the same mnt at the same time. Even an atomic_t in the mnt has massive scalaing problems because the cacheline gets so terribly contended.
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
Upon a remount,ro request, all of the data from the percpu variables is collected (expensive, but very rare) and we determine if there are any outstanding writers to the mount.
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.
(To get rid of that 3%, we could have an #defined number of mounts in the percpu variable. So, instead of a CPU getting operate only on percpu data when it accesses only one mount, it could stay on percpu data when it only accesses N or fewer mounts.)
---
linux-2.6.git-dave/fs/namespace.c | 205 ++++++++++++++++++++++++++++--- linux-2.6.git-dave/include/linux/mount.h | 8 + 2 files changed, 198 insertions(+), 15 deletions(-)
fs/namespace.c renamed from fs/namespace.c~r-o-bind-mounts-track-number-of-mount-writers +190 -15
@@ -17,6 +17,7 @@#include <linux/quotaops.h> #include <linux/acct.h> #include <linux/capability.h> +#include <linux/cpumask.h> #include <linux/module.h> #include <linux/sysfs.h> #include <linux/seq_file.h>
@@ -52,6 +53,8 @@ static inline unsigned long hash(structreturn tmp & hash_mask; } +#define MNT_WRITER_UNDERFLOW_LIMIT -(1<<16) +struct vfsmount *alloc_vfsmnt (const char *name) {struct vfsmount *mnt =kmem_cache_zalloc (mnt_cache,GFP_KERNEL );
@@ -65,6 +68,7 @@ struct vfsmount *alloc_vfsmnt(const charINIT_LIST_HEAD (&mnt->mnt_share);INIT_LIST_HEAD (&mnt->mnt_slave_list);INIT_LIST_HEAD (&mnt->mnt_slave); +atomic_set (&mnt->__mnt_writers,0 );if (name) {int size =strlen (name) +1 ;char *newname =kmalloc (size,GFP_KERNEL );
@@ -85,6 +89,84 @@ struct vfsmount *alloc_vfsmnt(const char* we can determine when writes are able to occur to * a filesystem. */ +/* +* __mnt_is_readonly: check whether a mount is read-only +* @mnt: the mount to check for its write status +* +* 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*. This can not and +* should not be used in place of IS_RDONLY(inode). +* 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); + +struct mnt_writer { +/* +* If holding multiple instances of this lock, they +* must be ordered by cpu number. +*/ +spinlock_t lock; +unsigned long count; +struct vfsmount *mnt; +}____cacheline_aligned_in_smp ; +static DEFINE_PER_CPU(struct mnt_writer, mnt_writers); + +static int __init init_mnt_writers (void ) +{ +int cpu; +for_each_possible_cpu (cpu) { +struct mnt_writer *writer = &per_cpu (mnt_writers, cpu); +spin_lock_init (&writer->lock); + writer->count =0 ; + } +return 0 ; +} +fs_initcall (init_mnt_writers); + +static void mnt_unlock_cpus (void ) +{ +int cpu; +struct mnt_writer *cpu_writer; + +for_each_possible_cpu (cpu) { + cpu_writer = &per_cpu (mnt_writers, cpu); +spin_unlock (&cpu_writer->lock); + } +} + +static inline void __clear_mnt_count (struct mnt_writer *cpu_writer) +{ +if (!cpu_writer->mnt) +return ; +atomic_add (cpu_writer->count, &cpu_writer->mnt->__mnt_writers); + cpu_writer->count =0 ; +} +/* +* must hold cpu_writer->lock +*/ +static inline void use_cpu_writer_for_mount (struct mnt_writer *cpu_writer, +struct vfsmount *mnt) +{ +if (cpu_writer->mnt == mnt) +return ; +__clear_mnt_count (cpu_writer); + cpu_writer->mnt = 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. +*/ /** * mnt_want_write - get write access to a mount * @mnt: the mount on which to take a write
@@ -97,12 +179,58 @@ struct vfsmount *alloc_vfsmnt(const char*/ int mnt_want_write (struct vfsmount *mnt) { -if (__mnt_is_readonly (mnt)) -return -EROFS ; -return 0 ; +int ret =0 ; +struct mnt_writer *cpu_writer; + + cpu_writer = &get_cpu_var (mnt_writers); +spin_lock (&cpu_writer->lock); +if (__mnt_is_readonly (mnt)) { + ret = -EROFS ; + goto out; + } +use_cpu_writer_for_mount (cpu_writer, mnt); + cpu_writer->count++; +out: +spin_unlock (&cpu_writer->lock); +put_cpu_var (mnt_writers); +return ret; }EXPORT_SYMBOL_GPL (mnt_want_write); +static void lock_and_coalesce_cpu_mnt_writer_counts (void ) +{ +int cpu; +struct mnt_writer *cpu_writer; + +for_each_possible_cpu (cpu) { + cpu_writer = &per_cpu (mnt_writers, cpu); +spin_lock (&cpu_writer->lock); +__clear_mnt_count (cpu_writer); + cpu_writer->mnt =NULL ; + } +} + +/* +* These per-cpu write counts are not guaranteed to have +* matched increments and decrements on any given cpu. +* A file open()ed for write on one cpu and close()d on +* another cpu will imbalance this count. Make sure it +* does not get too far out of whack. +*/ +static void handle_write_count_underflow (struct vfsmount *mnt) +{ +while (atomic_read (&mnt->__mnt_writers) < +MNT_WRITER_UNDERFLOW_LIMIT ) { +/* +* It isn't necessary to hold all of the locks +* at the same time, but doing it this way makes +* us share a lot more code. +*/ +lock_and_coalesce_cpu_mnt_writer_counts (); +mnt_unlock_cpus (); + } +} +/** * mnt_drop_write - give up write access to a mount * @mnt: the mount on which to give up write access
@@ -113,23 +241,61 @@ EXPORT_SYMBOL_GPL(mnt_want_write);*/ void mnt_drop_write (struct vfsmount *mnt) { +int must_check_underflow =0 ; +struct mnt_writer *cpu_writer; + + cpu_writer = &get_cpu_var (mnt_writers); +spin_lock (&cpu_writer->lock); + +use_cpu_writer_for_mount (cpu_writer, mnt); +if (cpu_writer->count >0 ) { + cpu_writer->count--; + }else { + must_check_underflow =1 ; +atomic_dec (&mnt->__mnt_writers); + } + +spin_unlock (&cpu_writer->lock); +/* +* Logically, we could call this each time, +* but the __mnt_writers cacheline tends to +* be cold, and makes this expensive. +*/ +if (must_check_underflow) +handle_write_count_underflow (mnt); +/* +* This could be done right after the spinlock +* is taken because the spinlock keeps us on +* the cpu, and disables preemption. However, +* putting it here bounds the amount that +* __mnt_writers can underflow. Without it, +* we could theoretically wrap __mnt_writers. +*/ +put_cpu_var (mnt_writers); }EXPORT_SYMBOL_GPL (mnt_drop_write); -/* -* __mnt_is_readonly: check whether a mount is read-only -* @mnt: the mount to check for its write status -* -* 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*. This can not and -* should not be used in place of IS_RDONLY(inode). -*/ -int __mnt_is_readonly (struct vfsmount *mnt) +int mnt_make_readonly (struct vfsmount *mnt) { -return (mnt->mnt_sb->s_flags &MS_RDONLY ); +int ret =0 ; + +lock_and_coalesce_cpu_mnt_writer_counts (); +/* +* With all the locks held, this value is stable +*/ +if (atomic_read (&mnt->__mnt_writers) >0 ) { + ret = -EBUSY ; + 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(). +*/ +out: +mnt_unlock_cpus (); +return ret; } -EXPORT_SYMBOL_GPL (__mnt_is_readonly);int simple_set_mnt (struct vfsmount *mnt,struct super_block *sb) {
@@ -325,6 +491,15 @@ static struct vfsmount *clone_mnt(structstatic inline void __mntput (struct vfsmount *mnt) {struct super_block *sb = mnt->mnt_sb; +lock_and_coalesce_cpu_mnt_writer_counts (); +mnt_unlock_cpus (); +/* +* This probably indicates that somebody messed +* up a mnt_want/drop_write() pair. If this +* happens, the filesystem was probably unable +* to make r/w->r/o transitions. +*/ +WARN_ON (atomic_read (&mnt->__mnt_writers));dput (mnt->mnt_root);free_vfsmnt (mnt);deactivate_super (sb);
include/linux/mount.h renamed from include/linux/mount.h~r-o-bind-mounts-track-number-of-mount-writers +8 -0
@@ -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,13 @@ 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)
_
PATCH 22/27 r-o-bind-mounts-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.
---
linux-2.6.git-dave/fs/nfs/dir.c | 3 ++- linux-2.6.git-dave/fs/nfsd/vfs.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
fs/nfs/dir.c renamed from fs/nfs/dir.c~r-o-bind-mounts-nfs-check-mnt-instead-of-superblock-directly +2 -1
@@ -949,7 +949,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~r-o-bind-mounts-nfs-check-mnt-instead-of-superblock-directly +2 -2
@@ -1862,7 +1862,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
@@ -1873,7 +1873,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;
_
PATCH 26/27 r-o-bind-mounts-honor-r-w-changes-at-do_remount-time
In reply to Dave HansenOriginally 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.
---
linux-2.6.git-dave/fs/namespace.c | 46 ++++++++++++++++++++++++++----- linux-2.6.git-dave/include/linux/mount.h | 1 2 files changed, 40 insertions(+), 7 deletions(-)
fs/namespace.c renamed from fs/namespace.c~r-o-bind-mounts-honor-r-w-changes-at-do_remount-time +39 -7
@@ -102,7 +102,11 @@ struct vfsmount *alloc_vfsmnt(const char*/ 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 ; }EXPORT_SYMBOL_GPL (__mnt_is_readonly);
@@ -277,7 +281,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 ;
@@ -290,15 +294,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;
@@ -607,7 +617,7 @@ static int show_vfsmnt(struct seq_file *seq_putc (m,'.' );mangle (m, mnt->mnt_sb->s_subtype); } -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);
@@ -1195,6 +1205,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
@@ -1216,7 +1243,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);
@@ -1660,6 +1690,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 |MS_KERNMOUNT );
include/linux/mount.h renamed from include/linux/mount.h~r-o-bind-mounts-honor-r-w-changes-at-do_remount-time +1 -0
@@ -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
_
PATCH 25/27 r-o-bind-mounts-track-number-of-mount-writers-make-lockdep-happy-with-r-o-bind-mounts
In reply to Dave HansenWith the r/o bind mount patches, we can have as many spinlocks nested as there are CPUs on the system. Lockdep freaks out after 8.
So, create a new lockdep class of locks for the mnt_writer spinlocks, and initialize each of the cpu locks to be in a different class.
It should shut up warnings like this, while still allowing some of the lockdep goodness to remain:
=============================================
[ INFO: possible recursive locking detected ] 2.6.23-rc6 #6
---
linux-2.6.git-dave/fs/namespace.c | 2 ++ 1 file changed, 2 insertions(+)
fs/namespace.c renamed from fs/namespace.c~r-o-bind-mounts-track-number-of-mount-writers-make-lockdep-happy-with-r-o-bind-mounts +2 -0
@@ -112,6 +112,7 @@ struct mnt_writer {* must be ordered by cpu number. */ spinlock_t lock; +struct lock_class_key lock_class;/* compiles out with !lockdep */ unsigned long count;struct vfsmount *mnt; }____cacheline_aligned_in_smp ;
@@ -123,6 +124,7 @@ static int __init init_mnt_writers(void)for_each_possible_cpu (cpu) {struct mnt_writer *writer = &per_cpu (mnt_writers, cpu);spin_lock_init (&writer->lock); +lockdep_set_class (&writer->lock, &writer->lock_class); writer->count =0 ; }return 0 ;
_
PATCH 27/27 keep track of mnt_writer state of struct file
In reply to Dave HansenThere have been a few oopses caused by 'struct file's with NULL f_vfsmnts. There was also a set of potentially missed mnt_want_write()s from dentry_open() calls.
This patch provides a very simple debugging framework to catch these kinds of bugs. It will WARN_ON() them, but should stop us from having any oopses or mnt_writer count imbalances.
I'm quite convinced that this is a good thing because it found bugs in the stuff I was working on as soon as I wrote it.
---
linux-2.6.git-dave/fs/file_table.c | 21 +++++++++++++++++++-- linux-2.6.git-dave/fs/open.c | 14 +++++++++++++- linux-2.6.git-dave/include/linux/fs.h | 4 ++++ 3 files changed, 36 insertions(+), 3 deletions(-)
fs/file_table.c renamed from fs/file_table.c~keep-track-of-mnt_writer-state-of-struct-file +19 -2
@@ -42,6 +42,12 @@ static inline void file_free_rcu(structstatic inline void file_free (struct file *f) {percpu_counter_dec (&nr_files); +/* +* At this point, either both or neither of these bits +* should be set. +*/ +WARN_ON (f->f_mnt_write_state ==FILE_MNT_WRITE_TAKEN ); +WARN_ON (f->f_mnt_write_state ==FILE_MNT_WRITE_RELEASED );call_rcu (&f->f_u.fu_rcuhead, file_free_rcu); }
@@ -201,6 +207,7 @@ int init_file(struct file *file, struct* that we can do debugging checks at __fput()e */ if ((mode &FMODE_WRITE ) && !special_file (dentry->d_inode->i_mode)) { + file->f_mnt_write_state =FILE_MNT_WRITE_TAKEN ; error =mnt_want_write (mnt);WARN_ON (error); }
@@ -243,8 +250,18 @@ void fastcall __fput(struct file *file)fops_put (file->f_op);if (file->f_mode &FMODE_WRITE ) {put_write_access (inode); -if (!special_file (inode->i_mode)) -mnt_drop_write (mnt); +if (!special_file (inode->i_mode)) { +if (file->f_mnt_write_state ==FILE_MNT_WRITE_TAKEN ) { +mnt_drop_write (mnt); + file->f_mnt_write_state |= +FILE_MNT_WRITE_RELEASED ; + }else { +printk (KERN_WARNING "__fput() of writeable " +"file with no " +"mnt_want_write()\n" ); +WARN_ON (1 ); + } + } }put_pid (file->f_owner.pid);file_kill (file);
fs/open.c renamed from fs/open.c~keep-track-of-mnt_writer-state-of-struct-file +13 -1
@@ -810,6 +810,10 @@ static struct file *__dentry_open(struct error =__get_file_write_access (inode, mnt);if (error) goto cleanup_file; +if (!special_file (inode->i_mode)) { +WARN_ON (f->f_mnt_write_state !=0 ); + f->f_mnt_write_state =FILE_MNT_WRITE_TAKEN ; + } } f->f_mapping = inode->i_mapping;
@@ -851,8 +855,16 @@ cleanup_all:fops_put (f->f_op);if (f->f_mode &FMODE_WRITE ) {put_write_access (inode); -if (!special_file (inode->i_mode)) +if (!special_file (inode->i_mode)) { +/* +* We don't consider this a real +* mnt_want/drop_write() pair +* because it all happenend right +* here, so just reset the state. +*/ + f->f_mnt_write_state =0 ;mnt_drop_write (mnt); + } }file_kill (f); f->f_path.dentry =NULL ;
include/linux/fs.h renamed from include/linux/fs.h~keep-track-of-mnt_writer-state-of-struct-file +4 -0
@@ -774,6 +774,9 @@ static inline int ra_has_index(struct fi index < ra->start + ra->size); } +#define FILE_MNT_WRITE_TAKEN 1 +#define FILE_MNT_WRITE_RELEASED 2 +struct file {/* * fu_list becomes invalid after file_free is called and queued via
@@ -808,6 +811,7 @@ struct file {spinlock_t f_ep_lock;#endif /* #ifdef CONFIG_EPOLL */ struct address_space *f_mapping; +unsigned long f_mnt_write_state; };extern spinlock_t files_lock;#define file_list_lock () spin_lock(&files_lock);
_
Andrew Morton
maintainerRe: [PATCH 16/27] r-o-bind-mounts-elevate-write-count-for-some-ioctls
In reply to Dave HansenQuoted 6 lines · On Thu, 01 Nov 2007 16:08:47 -0700 Dave Hansen <[email protected]> wrote:
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.
See, when we combine this patch with Jan's forbid-user-to-change-file-flags-on-quota-files.patch we silently add bugs to five filesystems. Lessons:
- never ever ever do `return' from deep in the guts of a function. This is a *classic* instance of the maintainability risks which this practice introduces.
- this whole elevate-the-write-count-in-a-zillion-places stuff is quite fragile.
fs/ext2/ioctl.c renamed from fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +2 -1
@@ -57,7 +57,8 @@ int ext2_ioctl (struct inode * inode, st/* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA (inode)) {mutex_unlock (&inode->i_mutex); -return -EPERM ; + ret = -EPERM ; + goto setflags_out; } oldflags = ei->i_flags;
fs/ext3/ioctl.c renamed from fs/ext3/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +2 -1
@@ -60,7 +60,8 @@ int ext3_ioctl (struct inode * inode, st/* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA (inode)) {mutex_unlock (&inode->i_mutex); -return -EPERM ; + err = -EPERM ; + goto flags_out; } oldflags = ei->i_flags;
fs/ext4/ioctl.c renamed from fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +2 -1
@@ -59,7 +59,8 @@ int ext4_ioctl (struct inode * inode, st/* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA (inode)) {mutex_unlock (&inode->i_mutex); -return -EPERM ; + err = -EPERM ; + goto flags_out; } oldflags = ei->i_flags;
fs/jfs/ioctl.c renamed from fs/jfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +4 -2
@@ -85,8 +85,10 @@ int jfs_ioctl(struct inode * inode, stru flags &= ~JFS_DIRSYNC_FL ;/* Is it quota file? Do not allow user to mess with it */ -if (IS_NOQUOTA (inode)) -return -EPERM ; +if (IS_NOQUOTA (inode)) { + err = -EPERM ; + goto setflags_out; + }jfs_get_inode_flags (jfs_inode); oldflags = jfs_inode->mode2;
diff -puN fs/reiserfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files fs/reiserfs/ioctl.c _
Andrew Morton
maintainerRe: [PATCH 25/27] r-o-bind-mounts-track-number-of-mount-writers-make-lockdep-happy-with-r-o-bind-mounts
In reply to Dave HansenQuoted 8 lines · On Thu, 01 Nov 2007 16:09:00 -0700 Dave Hansen <[email protected]> wrote:
With the r/o bind mount patches, we can have as many spinlocks nested as there are CPUs on the system. Lockdep freaks out after 8.
So, create a new lockdep class of locks for the
mnt_writerspinlocks, and initialize each of the cpu locks to be in a different class.
Quoted 7 lines
It should shut up warnings like this, while still allowing some of the lockdep goodness to remain:
=============================================
[ INFO: possible recursive locking detected ] 2.6.23-rc6 #6
I added your signed-off-by: to this.
I wish you hadn't mucked up all those Subject:s
Jan Kara
reviewerRe: [PATCH 16/27] r-o-bind-mounts-elevate-write-count-for-some-ioctls
In reply to Andrew MortonQuoted 10 lines
Quoted 6 lines · On Thu, 01 Nov 2007 16:08:47 -0700 Dave Hansen <[email protected]> wrote:
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.
Quoted 7 lines
See, when we combine this patch with Jan's forbid-user-to-change-file-flags-on-quota-files.patch we silently add bugs to five filesystems. Lessons:
- never ever ever do `return' from deep in the guts of a function. This is a *classic* instance of the maintainability risks which this practice introduces.
OK, lesson learned ;) Thanks for spotting this. But if there already
was a label like:
err_out:
return err;
And I would have just added a place which does 'goto err_out', then
the same problem could arise... But I agree it's less likely than if we do just return err;.
Quoted 58 lines
- this whole elevate-the-write-count-in-a-zillion-places stuff is quite fragile.
diff -puN
fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-filesfs/ext2/ioctl.cfs/ext2/ioctl.c renamed from fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +2
-1@@ -57,7 +57,8 @@ int ext2_ioctl (struct inode * inode, st/* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA (inode)) {mutex_unlock (&inode->i_mutex); -return -EPERM ; + ret = -EPERM ; + goto setflags_out; } oldflags = ei->i_flags;diff -puN
fs/ext3/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-filesfs/ext3/ioctl.cfs/ext3/ioctl.c renamed from fs/ext3/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +2
-1@@ -60,7 +60,8 @@ int ext3_ioctl (struct inode * inode, st/* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA (inode)) {mutex_unlock (&inode->i_mutex); -return -EPERM ; + err = -EPERM ; + goto flags_out; } oldflags = ei->i_flags;diff -puN
fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-filesfs/ext4/ioctl.cfs/ext4/ioctl.c renamed from fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +2
-1@@ -59,7 +59,8 @@ int ext4_ioctl (struct inode * inode, st/* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA (inode)) {mutex_unlock (&inode->i_mutex); -return -EPERM ; + err = -EPERM ; + goto flags_out; } oldflags = ei->i_flags;diff -puN
fs/jfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-filesfs/jfs/ioctl.cfs/jfs/ioctl.c renamed from fs/jfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files +4
-2@@ -85,8 +85,10 @@ int jfs_ioctl(struct inode * inode, stru flags &= ~JFS_DIRSYNC_FL ;/* Is it quota file? Do not allow user to mess with it */ -if (IS_NOQUOTA (inode)) -return -EPERM ; +if (IS_NOQUOTA (inode)) { + err = -EPERM ; + goto setflags_out; + }jfs_get_inode_flags (jfs_inode); oldflags = jfs_inode->mode2;
Quoted 2 lines
diff -puN
fs/reiserfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-filesfs/reiserfs/ioctl.c_
Why there's no modification for reiserfs?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SuSE CR Labs
Andrew Morton
maintainerRe: [PATCH 16/27] r-o-bind-mounts-elevate-write-count-for-some-ioctls
Quoted 1 line · On Tue, 6 Nov 2007 10:01:02 +0100 Jan Kara <[email protected]> wrote:
Why there's no modification for reiserfs?
I fixed that in the base patch while repairing some rejects.
Re: [PATCH 05/27] rename open_namei() to open_pathname()
In reply to Dave HansenQuoted 3 lines · Dave Hansen
open_namei()no longer touches namei's. rename it to something more appropriate:open_pathname().
The name is quite non-descriptive. What about just leaving it as filp_open and merging this into the previous patches to avoid the spurious renaming?
And btw, exported functions that get half-rewritten should always grow a kerneldoc comment. Then again this particular one really shouldn't be exported, but that's for another series.
Andrew Morton
maintainerRe: [PATCH 04/27] kill filp_open()
In reply to Dave HansenQuoted 2 lines · On Thu, 01 Nov 2007 16:08:31 -0700 Dave Hansen <[email protected]> wrote:
Replace all callers with
open_namei()directly, and move the nameidata stack allocation intoopen_namei().
ftp://ftp.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-loop.patch is using filp_open() and hence doesn't work very well.
A shall revert dm-loop.patch and run away.
Re: [PATCH 04/27] kill filp_open()
Quoted 8 lines · Andrew Morton
Quoted 2 lines · On Thu, 01 Nov 2007 16:08:31 -0700 Dave Hansen <[email protected]> wrote:
Replace all callers with
open_namei()directly, and move the nameidata stack allocation intoopen_namei().ftp://ftp.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-loop.patch is using
filp_open()and hence doesn't work very well.A shall revert dm-loop.patch and run away.
This one's pretty easy, thank goodness. Just replace filp_open() with open_namei():
/filp_open(/open_namei(AT_FDCWD, /
BTW, why do we need this on top of the existing loopback driver? Can they really share no code?
linux-2.6.git-dave/drivers/md/dm-loop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/dm-loop.c renamed from drivers/md/dm-loop.c~fix-dm-loop +1 -1
@@ -757,7 +757,7 @@ static int loop_get_file(struct dm_targeint r =0 ; ti->error ="could not open backing file" ; - filp =filp_open (lc->path, flags,0 ); + filp =open_namei (AT_FDCWD , lc->path, flags,0 );if (IS_ERR (filp))return PTR_ERR (filp); lc->filp = filp;
_
-- Dave
Re: [PATCH 04/27] kill filp_open()
Quoted 8 lines · Dave Hansen
Quoted 3 lines
is using
filp_open()and hence doesn't work very well.A shall revert dm-loop.patch and run away.
This one's pretty easy, thank goodness. Just replace
filp_open()withopen_namei():/filp_open(/open_namei(AT_FDCWD, /
But ultimatively I think we should remove this silly renaming from the patch. It doesn't help the goal and just created churn, so please rename open_namei back to do_filp_open for now and put filp_open back.
Quoted 2 lines
BTW, why do we need this on top of the existing loopback driver? Can they really share no code?
Yeah, given how much pain loop drivers in general are I don't think it's a good idea to add a second one.
Re: [PATCH 04/27] kill filp_open()
In reply to Dave Hansen-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 9 lines · Dave Hansen
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Thu, 01 Nov 2007 16:08:31 -0700 Dave Hansen <[email protected]> wrote:
Replace all callers with
open_namei()directly, and move the nameidata stack allocation intoopen_namei().ftp://ftp.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-loop.patch is using
filp_open()and hence doesn't work very well.A shall revert dm-loop.patch and run away.
Quoted 7 lines
This one's pretty easy, thank goodness. Just replace
filp_open()withopen_namei():/filp_open(/open_namei(AT_FDCWD, /BTW, why do we need this on top of the existing loopback driver? Can they really share no code?
The current dm-loop patch was really written to demonstrate the value of block-mapping as an alternative to doing file backed I/O via the pagecache.
Personally, I would like to see a single "loopback block devices" driver in the kernel with loop.c and dm-loop as alternate interfaces to it.
There seems to be some demand for this, e.g.:
http://bugzilla.kernel.org/show_bug.cgi?id=5333
And numerous similar posts to dm-devel (although I think obsoleting may be going a bit far to start with!).
Regards,
Bryn.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org
iD8DBQFHjjsA6YSQoMYUY94RAv+fAJ90mbqDQ/17nd3j9OAIXXc8Y4SRbQCfR75I KtDxHXDnYmCmBlw/lOzGmms=
=A+Vb
-----END PGP SIGNATURE-----
Re: [PATCH 04/27] kill filp_open()
In reply to Christoph HellwigQuoted 4 lines · Christoph Hellwig
But ultimatively I think we should remove this silly renaming from the patch. It doesn't help the goal and just created churn, so please rename
open_nameiback todo_filp_openfor now and putfilp_openback.
It wasn't really a rename. More that I ended up moving everything useful the filp_open() did to other functions, so I didn't see the need to keep around a stub like this:
struct file *filp_open(const char *filename, int flags, int mode)
{
return open_namei(AT_FDCWD, filename, flags, mode);}
It would be trivial to add this stub back, though.
-- Dave
Re: [PATCH 04/27] kill filp_open()
Quoted 10 lines · Dave Hansen
Quoted 4 lines · Christoph Hellwig
But ultimatively I think we should remove this silly renaming from the patch. It doesn't help the goal and just created churn, so please rename
open_nameiback todo_filp_openfor now and putfilp_openback.It wasn't really a rename. More that I ended up moving everything useful the
filp_open()did to other functions, so I didn't see the need to keep around a stub like this:
Quoted 6 lines
struct file *filp_open(const char *filename, int flags, int mode)
{ return open_namei(AT_FDCWD, filename, flags, mode);}
It would be trivial to add this stub back, though.
Well, it was a name from the caller point of view :) You basically merged do_filp_open and open_namei. Given that open_namei was an internal helper only used by do_filp_open it seems quite sensible to keep the existing do_filp_open user interface and call the merged function do_filp_open. Especially as filp_open is a quite descriptive name of what we're doing here - open the pathname and return a file pointer for it.