Subject: Re: [NFS] [PATCH 0/4] add killattr inode operation to allow
	filesystems to interpret ATTR_KILL_S*ID bits
From: Trond Myklebust <trond.myklebust@fys.uio.no>
To: Christoph Hellwig <hch@infradead.org>
Cc: Jeff Layton <jlayton@redhat.com>, linux-fsdevel@vger.kernel.org,
       nfs@lists.sourceforge.net, linux-cifs-client@lists.samba.org,
       linux-kernel@vger.kernel.org
In-Reply-To: <20070828191114.GA3419@infradead.org>
References: <200708271727.l7RHR36m007248@dantu.rdu.redhat.com>
	 <20070828191114.GA3419@infradead.org>
Content-Type: text/plain
Date: Tue, 28 Aug 2007 15:49:51 -0400
Message-Id: <1188330591.6701.202.camel@heimdal.trondhjem.org>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1902
Lines: 50

On Tue, 2007-08-28 at 20:11 +0100, Christoph Hellwig wrote:
> Sorry for not replying to the previsious revisions, but I've been out
> for on vacation.
> 
> I can't say I like this version.  Now we've got callouts at two rather close
> levels which is not very nice from the interface POV.

Agreed.

> Maybe preference is for the first scheme where we simply move interpreation
> of the ATTR_KILL_SUID/ATTR_KILL_SGID into the setattr routine and provide
> a nice helper for the normal filesystem to use.
> 
> If people are really concerned about adding two lines of code to the
> handfull of setattr operation there's a variant of this scheme that can
> avoid it:
> 
>  - notify_change is modified to not clear the ATTR_KILL_SUID/ATTR_KILL_SGID
>    but update ia_mode and the ia_valid flag to include ATTR_MODE.
>  - disk filesystems stay unchanged and never look at
>    ATTR_KILL_SUID/ATTR_KILL_SGID, but nfs can check for it and ignore
>    the ATTR_MODE flags and ia_valid in this case and do the right thing
>    on the server side.

Hmm... There has to be an implicit promise here that nobody else will
ever try to set ATTR_KILL_SUID/ATTR_KILL_SGID and ATTR_MODE at the same
time. Currently, that assumption is not there:


> 	if (ia_valid & ATTR_KILL_SGID) {
> 		attr->ia_valid &= ~ ATTR_KILL_SGID;
> 		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> 			if (!(ia_valid & ATTR_MODE)) {
> 				ia_valid = attr->ia_valid |= ATTR_MODE;
> 				attr->ia_mode = inode->i_mode;
> 			}
> 			attr->ia_mode &= ~S_ISGID;
> 		}
> 	}

Should we perhaps just convert the above 'if (!(ia_valid & ATTR_MODE))'
into a 'BUG_ON(ia_valid & ATTR_MODE)'?

Trond

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/