Christoph Hellwig wrote:
> On Wed, Oct 29, 2008 at 05:35:11PM +1100, Timothy Shimmin wrote:
>> Christoph Hellwig wrote:
>>> Instead of implementing our own checks use inode_change_ok to check for
>>> nessecary permission in setattr.
>> Yeah, the 1st bit I quite like and is similar to what I did in some
>> nfs4acl code, as you know.
>> We put all the EPERM cases early on which is nice.
>
> Yes. The big differene to the NFSv4 ACL patches is that we use the
> standard kernel inode_change_ok routine, which means we are guaranteed
> to have the same checks as all other filesystems and get rid of
> duplicated code.
>
> Btw, I must also say that I really hate the way the NFSv4 ACL patches make
> this filesystem-specific for all filesystems that support the NFSv4
> ACLs. All these permission checks should instead go through
> ->permission with additional MAY_ flags.
>
Yeah, it would be nice to have more out of the filesystem.
>> And then we have similar code in inode_setattr()
>> if (ia_valid & ATTR_MODE) {
>> umode_t mode = attr->ia_mode;
>>
>> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>> mode &= ~S_ISGID;
>> inode->i_mode = mode;
>> }
>>
>> But what about the suid case?
>
> SUID is handled in the inode_change_ok bit you quoted earlier.
For some reason I can't see where? Sorry.
I see the sgid clear but not the suid clear. D'oh.
> But we
> should add this S_ISGID handling here to XFS, too.
>
>> And also, what is with the ATTR_KILL_* bits?
>> Lemme look...
>
> ATTR_KILL_SUID/ATTR_KILL_SGID is a rather special thing added for NFS
> which doesn't want to do these locally but only on the server. Local
> filesystems can simply ignore it.
>
Thanks for the explanation.
> Updated patch below. Note that the S_ISGID hadling required moving
> the ATTR_MODE handling after the ATTR_GID handling.
>
So from above we have:
notify_change(struct dentry * dentry, struct iattr * attr):
> if (inode->i_op && inode->i_op->setattr) {
> error = security_inode_setattr(dentry, attr);
> if (!error)
> error = inode->i_op->setattr(dentry, attr);
> } else {
> error = inode_change_ok(inode, attr);
> if (!error)
> error = security_inode_setattr(dentry, attr);
> if (!error) {
> if ((ia_valid & ATTR_UID && attr->ia_uid !=
> inode->i_uid) ||
> (ia_valid & ATTR_GID && attr->ia_gid !=
> inode->i_gid))
> error = DQUOT_TRANSFER(inode, attr) ? -EDQUOT
> : 0;
> if (!error)
> error = inode_setattr(inode, attr);
> }
> }
So in our case i_op->setattr -> xfs_setattr
And xfs_setattr will now call inode_change_ok().
So we should have similar code to inode_setattr.
So why do we do sgid clear in inode_setattr() as well as in inode_change_ok()?
Doesn't inode_change_ok propagate the attr->ia_mode change back?
Okay, the SGID clearing for mode setting seems fine.
However, we really should have a QA test for this stuff.
Something is bound to stuff up here.
--Tim
|