Re: [NFS] [PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[Jeff Layton <jlayton@redhat.com>]

On Tue, 28 Aug 2007 15:49:51 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> 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:
> 

That was my concern with this scheme as well...

> 
> > 	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)'?
> 

Sounds reasonable. I'll also throw in a comment that explains this
reasoning...

-- 
Jeff Layton <jlayton@redhat.com>