Re: [PATCH 3/3] use inode_change_ok for setattr permission checking

[Timothy Shimmin <tes@sgi.com>]

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