On Mon, 2007-07-23 at 15:05 -0400, Jeff Layton wrote: > On Thu, 28 Jun 2007 09:38:22 -0400 > Trond Myklebust <trond.myklebust@xxxxxxxxxx> wrote: > > > On Wed, 2007-06-27 at 22:13 -0400, Jeff Layton wrote: > > > Ok. This is a bit more complex now since we remove suid bits on > > > truncate, but don't set ATTR_FORCE. > > > > > > Here's a patch that should do this. I know there's a general > > > aversion to adding new flags to vfs structures, but I couldn't think of > > > a way to cleanly do this without adding one. > > > > > > Note that I've not tested this patch at all so this is just a RFC. > > > > > > CC'ing Al here since he's expressed interest in this problem as well. > > > > > > Thoughts? > > > > We don't really need to do this with extra VFS flags. Here is my > > preferred approach for dealing with this problem. > > > > http://article.gmane.org/gmane.linux.nfs/8511/match=attr%5fkill%5fsuid > > > > As you can see, that still allows the filesystem to determine how it > > should deal with the ATTR_KILL_SUID/ATTR_KILL_SGID flags. The default > > behaviour is provided by inode_setattr(), and is the same as today. Only > > filesystems that don't use inode_setattr() will need to be audited for > > whether or not they need a fix. > > > > Cheers, > > Trond > > > I had a look at this today, and I have some reservations about this > approach. Quite a few filesystems define a .setattr inode op, but don't > call inode_setattr. As a first pass through the current git tree, the > following setattr ops never seem to call inode_setattr: > adfs_notify_change ^^^ doesn't support setuid/setgid mode bits in the first place. > afs_setattr networked filesystem: may need a solution like nfs. Can band aid over the problem by calling a helper to translate. ATTR_KILL_SUID/ATTR_KILL_SGID into the appropriate ATTR_MODE. > coda_setattr ditto > ecryptfs_setattr this is a layered filesystem. The underlying filesystem needs to deal with the mode. > fuse_setattr fix iattr_to_fattr to call helper to translate ATTR_KILL_SUID/ATTR_KILL_SGID into ATTR_MODE > jffs2_setattr add call to helper to translate ATTR_KILL_SUID/ATTR_KILL_SGID to ATTR_MODE. > nfs_setattr (expected) > ntfs_setattr Returns EOPNOTSUPP if you try to set ATTR_MODE. > smb_notify_change Returns EPERM if you try to set ATTR_MODE > xfs_vn_setattr add call to helper to translate ATTR_KILL_SUID/ATTR_KILL_SGID > ...some of these won't matter, but some will need to be fixed. There > may be other situations we'll need to fix as well. > My concern here is that we'll be moving from a "default safe" model for > filesystems that define a .setattr op to one where those filesystems > are expected to make sure that they clear setuid/gid bits. They can do > this with a simple call to inode_setattr, or on their own, but they > must do it. Is this really the right thing to do here? What is so bloody difficult about remembering to support ATTR_KILL_SUID ATTR_KILL_SGID vs all the other ATTR_* flags if you are choosing to implement your own .setattr? As long as there exists a simple VFS helper to do the translation into an ATTR_MODE request, so that those filesystems that rely on the current translation by 'notify_change' can easily migrate, then I can't see why this is such a problem. Trond - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
