From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [PATCH 2/2] check ATTR_SIZE contraints in inode_change_ok Date: Wed, 9 Jun 2010 17:33:36 +1000 Message-ID: <20100609073336.GV26335@laptop> References: <20100601113915.GA4861@lst.de> <20100601113937.GB4929@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: viro@zeniv.linux.org.uk, jack@suse.cz, linux-fsdevel@vger.kernel.org To: Christoph Hellwig Return-path: Received: from cantor.suse.de ([195.135.220.2]:56377 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752775Ab0FIHdn (ORCPT ); Wed, 9 Jun 2010 03:33:43 -0400 Content-Disposition: inline In-Reply-To: <20100601113937.GB4929@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > > 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 > #include > > -/* 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?