On Tue, 28 Aug 2007 15:49:51 -0400
Trond Myklebust <[email protected]> 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 <[email protected]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
