Re: [PATCH 2/2] check ATTR_SIZE contraints in inode_change_ok

[Nick Piggin <npiggin@suse.de>]

On Tue, Jun 01, 2010 at 01:39:37PM +0200, Christoph Hellwig wrote:
> 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 it's 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>
> 
> Index: linux-2.6/fs/attr.c
> ===================================================================
> --- linux-2.6.orig/fs/attr.c	2010-06-01 12:19:47.987277079 +0200
> +++ linux-2.6/fs/attr.c	2010-06-01 12:48:26.063005672 +0200
> @@ -14,35 +14,53 @@
>  #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 (attr->ia_mode & ATTR_SIZE) {
> +		int error = inode_newsize_ok(inode, attr->ia_size);
> +		if (error)
> +			return error;
> +	}

Hmm, I don't know if we can do this unless you have audited the
filesystems (in which case they should be on the cc list of this
pach).

The problem is whether the i_size is valid and stable at this
point. And it doesn't help even if you do leave the inode_newsize_ok
check inside the vmtruncate part of the fs if the check incorrectly
fails here.

ocfs2 performs inode_change_ok outside ocfs2_rw_lock and
ocfs2_inode_lock, and inode_newsize_ok inside; cifs holds i_lock
while checking inode_newsize_ok and updating size; gfs2 inside
gfs2_trans_begin.

Haven't looked closely at all fses or the consequences of the above.

Thoughts?