check ATTR_SIZE contraints in inode_change_ok
Christoph Hellwig [Fri, 4 Jun 2010 09:30:04 +0000 (11:30 +0200)]
Make sure we check the truncate constraints early on in ->setattr by adding
those checks to inode_change_ok.  Also clean up and document inode_change_ok
to make this obvious.

As a fallout we don't have to call inode_newsize_ok from simple_setsize and
simplify it down to a truncate_setsize which doesn't return an error.  This
simplifies a lot of setattr implementations and means we use truncate_setsize
almost everywhere.  Get rid of fat_setsize now that it's trivial and mark
ext2_setsize static to make the calling convention obvious.

Keep the inode_newsize_ok in vmtruncate for now as all callers need an
audit for its removal anyway.

Note: setattr code in ecryptfs doesn't call inode_change_ok at all and
needs a deeper audit, but that is left for later.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

21 files changed:
fs/adfs/inode.c
fs/attr.c
fs/ecryptfs/inode.c
fs/ext2/inode.c
fs/fat/fat.h
fs/fat/file.c
fs/fuse/dir.c
fs/gfs2/aops.c
fs/gfs2/ops_inode.c
fs/jffs2/fs.c
fs/libfs.c
fs/ocfs2/file.c
fs/ramfs/file-nommu.c
fs/smbfs/inode.c
fs/ubifs/file.c
fs/ubifs/ubifs.h
fs/ufs/truncate.c
include/linux/fs.h
include/linux/mm.h
mm/shmem.c
mm/truncate.c

index b3dec19..65794b8 100644 (file)
@@ -333,10 +333,7 @@ adfs_notify_change(struct dentry *dentry, struct iattr *attr)
 
        /* XXX: this is missing some actual on-disk truncation.. */
        if (ia_valid & ATTR_SIZE)
-               error = simple_setsize(inode, attr->ia_size);
-
-       if (error)
-               goto out;
+               truncate_setsize(inode, attr->ia_size);
 
        if (ia_valid & ATTR_MTIME) {
                inode->i_mtime = attr->ia_mtime;
index ed44d8a..7ca4181 100644 (file)
--- a/fs/attr.c
+++ b/fs/attr.c
 #include <linux/fcntl.h>
 #include <linux/security.h>
 
-/* Taken over from the old code... */
-
-/* POSIX UID/GID verification for setting inode attributes. */
+/**
+ * inode_change_ok - check if attribute changes to an inode are allowed
+ * @inode:     inode to check
+ * @attr:      attributes to change
+ *
+ * Check if we are allowed to change the attributes contained in @attr
+ * in the given inode.  This includes the normal unix access permission
+ * checks, as well as checks for rlimits and others.
+ *
+ * Should be called as the first thing in ->setattr implementations,
+ * possibly after taking additional locks.
+ */
 int inode_change_ok(const struct inode *inode, struct iattr *attr)
 {
-       int retval = -EPERM;
        unsigned int ia_valid = attr->ia_valid;
 
+       /*
+        * First check size constraints.  These can't be overriden using
+        * ATTR_FORCE.
+        */
+       if (ia_valid & ATTR_SIZE) {
+               int error = inode_newsize_ok(inode, attr->ia_size);
+               if (error)
+                       return error;
+       }
+
        /* If force is set do it anyway. */
        if (ia_valid & ATTR_FORCE)
-               goto fine;
+               return 0;
 
        /* Make sure a caller can chown. */
        if ((ia_valid & ATTR_UID) &&
            (current_fsuid() != inode->i_uid ||
             attr->ia_uid != inode->i_uid) && !capable(CAP_CHOWN))
-               goto error;
+               return -EPERM;
 
        /* Make sure caller can chgrp. */
        if ((ia_valid & ATTR_GID) &&
            (current_fsuid() != inode->i_uid ||
            (!in_group_p(attr->ia_gid) && attr->ia_gid != inode->i_gid)) &&
            !capable(CAP_CHOWN))
-               goto error;
+               return -EPERM;
 
        /* Make sure a caller can chmod. */
        if (ia_valid & ATTR_MODE) {
                if (!is_owner_or_cap(inode))
-                       goto error;
+                       return -EPERM;
                /* Also check the setgid bit! */
                if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
                                inode->i_gid) && !capable(CAP_FSETID))
@@ -52,12 +70,10 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr)
        /* Check for setting the inode time. */
        if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
                if (!is_owner_or_cap(inode))
-                       goto error;
+                       return -EPERM;
        }
-fine:
-       retval = 0;
-error:
-       return retval;
+
+       return 0;
 }
 EXPORT_SYMBOL(inode_change_ok);
 
@@ -113,7 +129,7 @@ EXPORT_SYMBOL(inode_newsize_ok);
  *
  * setattr_copy updates the inode's metadata with that specified
  * in attr. Noticably missing is inode size update, which is more complex
- * as it requires pagecache updates. See simple_setsize.
+ * as it requires pagecache updates.
  *
  * The inode is not marked as dirty after this operation. The rationale is
  * that for "simple" filesystems, the struct inode is the inode storage.
index 31ef525..82900b0 100644 (file)
@@ -804,10 +804,20 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
                size_t num_zeros = (PAGE_CACHE_SIZE
                                    - (ia->ia_size & ~PAGE_CACHE_MASK));
 
+
+               /*
+                * XXX(truncate) this should really happen at the begginning
+                * of ->setattr.  But the code is too messy to that as part
+                * of a larger patch.  ecryptfs is also totally missing out
+                * on the inode_change_ok check at the beginning of
+                * ->setattr while would include this.
+                */
+               rc = inode_newsize_ok(inode, ia->ia_size);
+               if (rc)
+                       goto out;
+
                if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) {
-                       rc = simple_setsize(inode, ia->ia_size);
-                       if (rc)
-                               goto out;
+                       truncate_setsize(inode, ia->ia_size);
                        lower_ia->ia_size = ia->ia_size;
                        lower_ia->ia_valid |= ATTR_SIZE;
                        goto out;
@@ -830,7 +840,7 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
                                goto out;
                        }
                }
-               simple_setsize(inode, ia->ia_size);
+               truncate_setsize(inode, ia->ia_size);
                rc = ecryptfs_write_inode_size_to_metadata(inode);
                if (rc) {
                        printk(KERN_ERR "Problem with "
index 7dee7b3..069620b 100644 (file)
@@ -1156,15 +1156,10 @@ static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
        __ext2_truncate_blocks(inode, offset);
 }
 
-int ext2_setsize(struct inode *inode, loff_t newsize)
+static int ext2_setsize(struct inode *inode, loff_t newsize)
 {
-       loff_t oldsize;
        int error;
 
-       error = inode_newsize_ok(inode, newsize);
-       if (error)
-               return error;
-
        if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
            S_ISLNK(inode->i_mode)))
                return -EINVAL;
@@ -1184,10 +1179,7 @@ int ext2_setsize(struct inode *inode, loff_t newsize)
        if (error)
                return error;
 
-       oldsize = inode->i_size;
-       i_size_write(inode, newsize);
-       truncate_pagecache(inode, oldsize, newsize);
-
+       truncate_setsize(inode, newsize);
        __ext2_truncate_blocks(inode, newsize);
 
        inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
index 27ac257..d75a77f 100644 (file)
@@ -306,7 +306,6 @@ extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
 extern const struct file_operations fat_file_operations;
 extern const struct inode_operations fat_file_inode_operations;
 extern int fat_setattr(struct dentry * dentry, struct iattr * attr);
-extern int fat_setsize(struct inode *inode, loff_t offset);
 extern void fat_truncate_blocks(struct inode *inode, loff_t offset);
 extern int fat_getattr(struct vfsmount *mnt, struct dentry *dentry,
                       struct kstat *stat);
index b2eedce..7257752 100644 (file)
@@ -364,18 +364,6 @@ static int fat_allow_set_time(struct msdos_sb_info *sbi, struct inode *inode)
        return 0;
 }
 
-int fat_setsize(struct inode *inode, loff_t offset)
-{
-       int error;
-
-       error = simple_setsize(inode, offset);
-       if (error)
-               return error;
-       fat_truncate_blocks(inode, offset);
-
-       return error;
-}
-
 #define TIMES_SET_FLAGS        (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)
 /* valid file mode bits */
 #define FAT_VALID_MODE (S_IFREG | S_IFDIR | S_IRWXUGO)
@@ -441,9 +429,8 @@ int fat_setattr(struct dentry *dentry, struct iattr *attr)
        }
 
        if (attr->ia_valid & ATTR_SIZE) {
-               error = fat_setsize(inode, attr->ia_size);
-               if (error)
-                       goto out;
+               truncate_setsize(inode, attr->ia_size);
+               fat_truncate_blocks(inode, attr->ia_size);
        }
 
        setattr_copy(inode, attr);
index 43a9b37..3978a42 100644 (file)
@@ -1280,12 +1280,8 @@ static int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
        if ((attr->ia_valid & ATTR_OPEN) && fc->atomic_o_trunc)
                return 0;
 
-       if (attr->ia_valid & ATTR_SIZE) {
-               err = inode_newsize_ok(inode, attr->ia_size);
-               if (err)
-                       return err;
+       if (attr->ia_valid & ATTR_SIZE)
                is_truncate = true;
-       }
 
        req = fuse_get_req(fc);
        if (IS_ERR(req))
index 703000d..54fe087 100644 (file)
@@ -702,12 +702,12 @@ out:
        page_cache_release(page);
 
        /*
-        * XXX(hch): the call below should probably be replaced with
+        * XXX(truncate): the call below should probably be replaced with
         * a call to the gfs2-specific truncate blocks helper to actually
         * release disk blocks..
         */
        if (pos + len > ip->i_inode.i_size)
-               simple_setsize(&ip->i_inode, ip->i_inode.i_size);
+               truncate_setsize(&ip->i_inode, ip->i_inode.i_size);
 out_endtrans:
        gfs2_trans_end(sdp);
 out_trans_fail:
index d7d410a..1009be2 100644 (file)
@@ -1072,7 +1072,7 @@ int gfs2_permission(struct inode *inode, int mask)
 }
 
 /*
- * XXX: should be changed to have proper ordering by opencoding simple_setsize
+ * XXX(truncate): the truncate_setsize calls should be moved to the end.
  */
 static int setattr_size(struct inode *inode, struct iattr *attr)
 {
@@ -1084,10 +1084,8 @@ static int setattr_size(struct inode *inode, struct iattr *attr)
                error = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
                if (error)
                        return error;
-               error = simple_setsize(inode, attr->ia_size);
+               truncate_setsize(inode, attr->ia_size);
                gfs2_trans_end(sdp);
-               if (error) 
-                       return error;
        }
 
        error = gfs2_truncatei(ip, attr->ia_size);
index 459d39d..1b24266 100644 (file)
@@ -169,13 +169,13 @@ int jffs2_do_setattr (struct inode *inode, struct iattr *iattr)
        mutex_unlock(&f->sem);
        jffs2_complete_reservation(c);
 
-       /* We have to do the simple_setsize() without f->sem held, since
+       /* We have to do the truncate_setsize() without f->sem held, since
           some pages may be locked and waiting for it in readpage().
           We are protected from a simultaneous write() extending i_size
           back past iattr->ia_size, because do_truncate() holds the
           generic inode semaphore. */
        if (ivalid & ATTR_SIZE && inode->i_size > iattr->ia_size) {
-               simple_setsize(inode, iattr->ia_size);
+               truncate_setsize(inode, iattr->ia_size);
                inode->i_blocks = (inode->i_size + 511) >> 9;
        }       
 
index 4056222..0a9da95 100644 (file)
@@ -327,49 +327,6 @@ int simple_rename(struct inode *old_dir, struct dentry *old_dentry,
 }
 
 /**
- * simple_setsize - handle core mm and vfs requirements for file size change
- * @inode: inode
- * @newsize: new file size
- *
- * Returns 0 on success, -error on failure.
- *
- * simple_setsize must be called with inode_mutex held.
- *
- * simple_setsize will check that the requested new size is OK (see
- * inode_newsize_ok), and then will perform the necessary i_size update
- * and pagecache truncation (if necessary). It will be typically be called
- * from the filesystem's setattr function when ATTR_SIZE is passed in.
- *
- * The inode itself must have correct permissions and attributes to allow
- * i_size to be changed, this function then just checks that the new size
- * requested is valid.
- *
- * In the case of simple in-memory filesystems with inodes stored solely
- * in the inode cache, and file data in the pagecache, nothing more needs
- * to be done to satisfy a truncate request. Filesystems with on-disk
- * blocks for example will need to free them in the case of truncate, in
- * that case it may be easier not to use simple_setsize (but each of its
- * components will likely be required at some point to update pagecache
- * and inode etc).
- */
-int simple_setsize(struct inode *inode, loff_t newsize)
-{
-       loff_t oldsize;
-       int error;
-
-       error = inode_newsize_ok(inode, newsize);
-       if (error)
-               return error;
-
-       oldsize = inode->i_size;
-       i_size_write(inode, newsize);
-       truncate_pagecache(inode, oldsize, newsize);
-
-       return error;
-}
-EXPORT_SYMBOL(simple_setsize);
-
-/**
  * simple_setattr - setattr for simple filesystem
  * @dentry: dentry
  * @iattr: iattr structure
@@ -394,12 +351,8 @@ int simple_setattr(struct dentry *dentry, struct iattr *iattr)
        if (error)
                return error;
 
-       if (iattr->ia_valid & ATTR_SIZE) {
-               error = simple_setsize(inode, iattr->ia_size);
-               if (error)
-                       return error;
-       }
-
+       if (iattr->ia_valid & ATTR_SIZE)
+               truncate_setsize(inode, iattr->ia_size);
        setattr_copy(inode, iattr);
        mark_inode_dirty(inode);
        return 0;
index 584cf8a..81296b4 100644 (file)
@@ -1233,7 +1233,7 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
        }
 
        /*
-        * This will intentionally not wind up calling simple_setsize(),
+        * This will intentionally not wind up calling truncate_setsize(),
         * since all the work for a size change has been done above.
         * Otherwise, we could get into problems with truncate as
         * ip_alloc_sem is used there to protect against i_size
@@ -2308,12 +2308,12 @@ relock:
                         * blocks outside i_size. Trim these off again.
                         * Don't need i_size_read because we hold i_mutex.
                         *
-                        * XXX(hch): this looks buggy because ocfs2 did not
+                        * XXX(truncate): this looks buggy because ocfs2 did not
                         * actually implement ->truncate.  Take a look at
                         * the new truncate sequence and update this accordingly
                         */
                        if (*ppos + count > inode->i_size)
-                               simple_setsize(inode, inode->i_size);
+                               truncate_setsize(inode, inode->i_size);
                        ret = written;
                        goto out_dio;
                }
index 8d44f03..9eead2c 100644 (file)
@@ -146,9 +146,8 @@ static int ramfs_nommu_resize(struct inode *inode, loff_t newsize, loff_t size)
                        return ret;
        }
 
-       ret = simple_setsize(inode, newsize);
-
-       return ret;
+       truncate_setsize(inode, newsize);
+       return 0;
 }
 
 /*****************************************************************************/
index 9551cb6..e338f0a 100644 (file)
@@ -714,9 +714,7 @@ smb_notify_change(struct dentry *dentry, struct iattr *attr)
                error = server->ops->truncate(inode, attr->ia_size);
                if (error)
                        goto out;
-               error = simple_setsize(inode, attr->ia_size);
-               if (error)
-                       goto out;
+               truncate_setsize(inode, attr->ia_size);
                refresh = 1;
        }
 
index 12f445c..03ae894 100644 (file)
@@ -967,14 +967,15 @@ static int do_writepage(struct page *page, int len)
  * the page locked, and it locks @ui_mutex. However, write-back does take inode
  * @i_mutex, which means other VFS operations may be run on this inode at the
  * same time. And the problematic one is truncation to smaller size, from where
- * we have to call 'simple_setsize()', which first changes @inode->i_size, then
+ * we have to call 'truncate_setsize()', which first changes @inode->i_size, then
  * drops the truncated pages. And while dropping the pages, it takes the page
- * lock. This means that 'do_truncation()' cannot call 'simple_setsize()' with
+ * lock. This means that 'do_truncation()' cannot call 'truncate_setsize()' with
  * @ui_mutex locked, because it would deadlock with 'ubifs_writepage()'. This
  * means that @inode->i_size is changed while @ui_mutex is unlocked.
  *
- * XXX: with the new truncate the above is not true anymore, the simple_setsize
- * calls can be replaced with the individual components.
+ * XXX(truncate): with the new truncate sequence this is not true anymore,
+ * and the calls to truncate_setsize can be move around freely.  They should
+ * be moved to the very end of the truncate sequence.
  *
  * But in 'ubifs_writepage()' we have to guarantee that we do not write beyond
  * inode size. How do we do this if @inode->i_size may became smaller while we
@@ -1128,9 +1129,7 @@ static int do_truncation(struct ubifs_info *c, struct inode *inode,
                budgeted = 0;
        }
 
-       err = simple_setsize(inode, new_size);
-       if (err)
-               goto out_budg;
+       truncate_setsize(inode, new_size);
 
        if (offset) {
                pgoff_t index = new_size >> PAGE_CACHE_SHIFT;
@@ -1217,16 +1216,14 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
 
        if (attr->ia_valid & ATTR_SIZE) {
                dbg_gen("size %lld -> %lld", inode->i_size, new_size);
-               err = simple_setsize(inode, new_size);
-               if (err)
-                       goto out;
+               truncate_setsize(inode, new_size);
        }
 
        mutex_lock(&ui->ui_mutex);
        if (attr->ia_valid & ATTR_SIZE) {
                /* Truncation changes inode [mc]time */
                inode->i_mtime = inode->i_ctime = ubifs_current_time(inode);
-               /* 'simple_setsize()' changed @i_size, update @ui_size */
+               /* 'truncate_setsize()' changed @i_size, update @ui_size */
                ui->ui_size = inode->i_size;
        }
 
@@ -1248,10 +1245,6 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode,
        if (IS_SYNC(inode))
                err = inode->i_sb->s_op->write_inode(inode, NULL);
        return err;
-
-out:
-       ubifs_release_budget(c, &req);
-       return err;
 }
 
 int ubifs_setattr(struct dentry *dentry, struct iattr *attr)
index 0431087..0c9876b 100644 (file)
@@ -379,7 +379,7 @@ struct ubifs_gced_idx_leb {
  * The @ui_size is a "shadow" variable for @inode->i_size and UBIFS uses
  * @ui_size instead of @inode->i_size. The reason for this is that UBIFS cannot
  * make sure @inode->i_size is always changed under @ui_mutex, because it
- * cannot call 'simple_setsize()' with @ui_mutex locked, because it would deadlock
+ * cannot call 'truncate_setsize()' with @ui_mutex locked, because it would deadlock
  * with 'ubifs_writepage()' (see file.c). All the other inode fields are
  * changed under @ui_mutex, so they do not need "shadow" fields. Note, one
  * could consider to rework locking and base it on "shadow" fields.
index 085e116..34d5cb1 100644 (file)
@@ -500,11 +500,6 @@ out:
        return err;
 }
 
-/*
- * TODO:
- *     - truncate case should use proper ordering instead of using
- *       simple_setsize
- */
 int ufs_setattr(struct dentry *dentry, struct iattr *attr)
 {
        struct inode *inode = dentry->d_inode;
@@ -518,9 +513,9 @@ int ufs_setattr(struct dentry *dentry, struct iattr *attr)
        if (ia_valid & ATTR_SIZE && attr->ia_size != inode->i_size) {
                loff_t old_i_size = inode->i_size;
 
-               error = simple_setsize(inode, attr->ia_size);
-               if (error)
-                       return error;
+               /* XXX(truncate): truncate_setsize should be called last */
+               truncate_setsize(inode, attr->ia_size);
+
                error = ufs_truncate(inode, old_i_size);
                if (error)
                        return error;
index 6ecb83c..5547b1b 100644 (file)
@@ -2355,7 +2355,6 @@ extern int simple_link(struct dentry *, struct inode *, struct dentry *);
 extern int simple_unlink(struct inode *, struct dentry *);
 extern int simple_rmdir(struct inode *, struct dentry *);
 extern int simple_rename(struct inode *, struct dentry *, struct inode *, struct dentry *);
-extern int simple_setsize(struct inode *, loff_t);
 extern int noop_fsync(struct file *, int);
 extern int simple_empty(struct dentry *);
 extern int simple_readpage(struct file *file, struct page *page);
index a2b4804..980164e 100644 (file)
@@ -815,6 +815,7 @@ static inline void unmap_shared_mapping_range(struct address_space *mapping,
 }
 
 extern void truncate_pagecache(struct inode *inode, loff_t old, loff_t new);
+extern void truncate_setsize(struct inode *inode, loff_t newsize);
 extern int vmtruncate(struct inode *inode, loff_t offset);
 extern int vmtruncate_range(struct inode *inode, loff_t offset, loff_t end);
 
index 0a43505..33222ba 100644 (file)
@@ -805,11 +805,10 @@ static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
                        }
                }
 
-               error = simple_setsize(inode, newsize);
+               /* XXX(truncate): truncate_setsize should be called last */
+               truncate_setsize(inode, newsize);
                if (page)
                        page_cache_release(page);
-               if (error)
-                       return error;
                shmem_truncate_range(inode, newsize, (loff_t)-1);
        }
 
index 937571b..ba887bf 100644 (file)
@@ -541,28 +541,48 @@ void truncate_pagecache(struct inode *inode, loff_t old, loff_t new)
 EXPORT_SYMBOL(truncate_pagecache);
 
 /**
+ * truncate_setsize - update inode and pagecache for a new file size
+ * @inode: inode
+ * @newsize: new file size
+ *
+ * truncate_setsize updastes i_size update and performs pagecache
+ * truncation (if necessary) for a file size updates. It will be
+ * typically be called from the filesystem's setattr function when
+ * ATTR_SIZE is passed in.
+ *
+ * Must be called with inode_mutex held and after all filesystem
+ * specific block truncation has been performed.
+ */
+void truncate_setsize(struct inode *inode, loff_t newsize)
+{
+       loff_t oldsize;
+
+       oldsize = inode->i_size;
+       i_size_write(inode, newsize);
+
+       truncate_pagecache(inode, oldsize, newsize);
+}
+EXPORT_SYMBOL(truncate_setsize);
+
+/**
  * vmtruncate - unmap mappings "freed" by truncate() syscall
  * @inode: inode of the file used
  * @offset: file offset to start truncating
  *
- * NOTE! We have to be ready to update the memory sharing
- * between the file and the memory map for a potential last
- * incomplete page.  Ugly, but necessary.
- *
- * This function is deprecated and simple_setsize or truncate_pagecache
- * should be used instead.
+ * This function is deprecated and truncate_setsize or truncate_pagecache
+ * should be used instead, together with filesystem specific block truncation.
  */
 int vmtruncate(struct inode *inode, loff_t offset)
 {
        int error;
 
-       error = simple_setsize(inode, offset);
+       error = inode_newsize_ok(inode, offset);
        if (error)
                return error;
 
+       truncate_setsize(inode, offset);
        if (inode->i_op->truncate)
                inode->i_op->truncate(inode);
-
-       return error;
+       return 0;
 }
 EXPORT_SYMBOL(vmtruncate);