Jeff Layton wrote:
On Tue, 21 Aug 2007 17:21:28 -0400
Josef Sipek <jsipek@xxxxxxxxxxxxxxxxx> wrote:
On Tue, Aug 21, 2007 at 07:35:51AM -0400, Jeff Layton wrote:
On Tue, 21 Aug 2007 15:35:08 +1000
Timothy Shimmin <tes@xxxxxxx> wrote:
Jeff Layton wrote:
This should fix all of the filesystems in the mainline kernels to handle
ATTR_KILL_SUID and ATTR_KILL_SGID correctly. For most of them, this is
just a matter of making sure that they call generic_attrkill early in
the setattr inode op.
Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
---
fs/xfs/linux-2.6/xfs_iops.c | 5 ++++-
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -651,12 +651,15 @@ xfs_vn_setattr(
struct iattr *attr)
{
struct inode *inode = dentry->d_inode;
- unsigned int ia_valid = attr->ia_valid;
+ unsigned int ia_valid;
bhv_vnode_t *vp = vn_from_inode(inode);
bhv_vattr_t vattr = { 0 };
int flags = 0;
int error;
+ generic_attrkill(inode->i_mode, attr);
+ ia_valid = attr->ia_valid;
+
if (ia_valid & ATTR_UID) {
vattr.va_mask |= XFS_AT_UID;
vattr.va_uid = attr->ia_uid;
Looks reasonable to me for XFS.
Acked-by: Tim Shimmin <tes@xxxxxxx>
So before, this clearing would happen directly in notify_change()
and now this won't happen until notify_change() calls i_op->setattr
which for a particular fs it can call generic_attrkill() to do it.
So I guess for the cases where i_op->setattr is called outside of
via notify_change, we don't normally have ATTR_KILL_SUID/SGID
set so that nothing will happen there?
Right. If neither ATTR_KILL bit is set then generic_attrkill is a
noop.
I guess just wondering the effect with having the code on all
setattr's. (I'm not familiar with the code path)
These bits are referenced in very few places in the current kernel
tree -- mostly in the VFS layer. The *only* place I see that they
actually get interpreted into a mode change is in notify_change. So
places that call setattr ops w/o going through notify_change are
not likely to have those bits set.
But hypothetically, if a fs did set ATTR_KILL_* and call setattr
directly, then the setattr would now include a mode change that
clears setuid or setgid bits where it may not have before.
I should probably clarify -- in the hypothetical situation above,
the setattr function would have to call generic_attrkill (as most
filesystems should do with this change).
Thanks for the confirmation. That's what it looked like to me
but I wanted to know explicitly what the thinking was.
It almost sounds like an argument for a new inode op (NULL would use
generic_attr_kill).
That's not a bad idea at all. I suppose that would be easier than
modifying every fs like this, and it does seem like it might be
cleaner. I need to mull it over, but that might be the best
solution.
Yeah, sounds a much more direct way of handling things and as you
say wouldn't need most of the filesystems to all be modified calling
generic_attrkill.
Not sure what the ramifications of adding a new iop are though.
Cheers,
Tim.
-
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
