Re: [PATCH] backout the xattr override access checks flag

[Andreas Gruenbacher <agruen@suse.de>]

On Friday 21 February 2003 16:54, Christoph Hellwig wrote:
> On Fri, Feb 21, 2003 at 11:20:25AM +0100, Andreas Gruenbacher wrote:
> > > (not that such code even exists yet).
> >
> > There is an HSM project for which this feature has been added. I think
> > they are using a loadable module.
>
> URL?
>
> > > Something even better would probably be to move out the xattr access
> > > checks to common code.
> >
> > There are two problems with that, so this doesn't seem any better to me,
> > either:
> >
> > (a) We would have to decode attribute names twice, once for checking
> > permissions, and a second time for determining how to store them.
>
> Doing it in the VFS would probably mean a rather large interface change
> so it should be decoded only once.  I.e. moving your current
> ext2/ext3-specific handler abstraction to the VFS instead.
>
> > (b) Different file systems may implement different features with
> > different, file system specific limitations. The VFS layer tests would
> > have to accept all potentially useful things. The file system would have
> > to re-check.
>
> What types of EAs do we have?
>
> (1) user attributes - the only access checks needed are the normal DAC ones
> (2) system/trusted - only privilegued access
>
> I think that's doable.

User and Trusted extended attributes have permission checks per namespace 
(i.e., the rules for all user.* attributes are identical, and the rules for 
all trusted.* attributes are identical).  System extended attribute access 
rules are per attribute: ACLs, Capabilities, MAC all have different policies.

Also, different file systems have different mechanisms for storing EAs, so 
that need to decode the attribute name, no matter what the VFS does.

I was thinking of at least abstracting the the permission checks from the file 
systems, so the file system layers can all use the same VFS helper function. 
But this is no real improvement, either:

	enum xattr_which { XATTR_USER, XATTR_TRUSTED,
				XATTR_POSIX_ACL, XATTR_CAP, ... };

	int xattr_permission(struct inode *inode, enum xattr_which which, int mask)
	{
		switch(which) {
			case XATTR_USER:
				return permission(inode, mask);

			case XATTR_TRUSTED:
				return capable(CAP_WHATEVER);

			case XATTR_POSIX_ACL:
				if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
					return -EPERM;
				return 0;
			default:
				return -EOPNOTSUPP;
		}
	}

This pseudo code is actually broken because it needs to be called from within 
the xattr inode operations where the inode lock is held, so the VFS 
permission() function cannot even be called because iops->permission() might 
grab the inode lock as well (and needs to on ACL aware systems).

So I agree with you that the checks could be moved to the VFS, but I don't see 
how this can be done in a way that is clearly better that keeping the checks 
in the FS.


Cheers,
Andreas.