On Tue, Oct 27, 2015 at 10:10:26PM +0100, Andreas Gruenbacher wrote: > On Tue, Oct 27, 2015 at 8:55 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Tue, Oct 27, 2015 at 04:55:22PM +0100, Andreas Gruenbacher wrote: > > But now that I've thought about this a bit more, I don't think that > > the changes you've made are correct - we shouldn't be doing uid/gid > > namespace conversion in disk formating functions. That is, the > > conversion of user/group types should be done at the incoming layers > > (i.e. at VFS/ioctl layers), not deep in the guts of the XFS code. > > There are two kinds of code paths where we need to convert between the > SGI_ACL and the kernel's in-memory representation (struct posix_acl > *): one is in the get_acl and set_acl iops, converting to/from the > actual on-disk attrbutes, and the actual IDs stay the same. The other > is in the get and set SGI_ACL xattr handlers which are invoked from > the getxattr and setxattr iops. The conversion there is to/from the > user-facing SGI_ACL xattrs, and ID mappings may be in effect. And then we have the ioctls XFS_IOC_ATTRLIST_BY_HANDLE and XFS_IOC_ATTRMULTI_BY_HANDLE which can directly access the ACL xattrs without even going through the the getxattr and setxattr iops. They go direct to xfs_attr_get/xfs_attr_set() and set/return the xattrs with *exactly* what the caller/on-disk xattr provides. IOWs, you've only addressed one possible vector for direct access to the SGI_ACL xattrs.... > The VFS doesn't know anything about the SGI_ACL attributes, so XFS > will have to do the ID mappings itself. We could convert the IDS > in-place in separate pre- / post-passes over the xattr value and leave > xfs_acl_from_disk and xfs_acl_to_disk alone. That's what the VFS does > for POSIX ACLs. The problem there is that the setxattr iop and set > xattr handler give us a const void * value; we cannot modify it > without casting const away. Hence the additional namespace parameter > to xfs_acl_from_disk and xfs_acl_to_disk instead. Yes, I read your patches - you take a low level disk formating function and make it also work as a high level conversion function through a different interface that has no higher level conversion interface. i.e. you're changing the user visible behaviour of writing a specific xattr. But I think it's also wrong: users are not supposed to manipulate SGI_ACL_FILE/SGI_ACL_DEFAULT xattrs directly. Just because root can see them doesn't mean users should be touching them - users should be using the kernel posix ACL interface/namespace to modify their ACLs. OTOH, xfsdump/restore require direct, unfiltered access to set uid/gid/mode/xattrs exactly as they are found. Changing that behaviour like your change does will break xfsdump/restore and breaking userspace is not a negotiable option. > > Why do you think we added the wrappers in the first place? It was > > because the ns maintainer refused to follow standard, self > > documenting type conversion naming conventions of <type>_to_<type> > > so we added them ourselves. The user namespace code is horrible > > enough without adding confusing type change functions everywhere... > > Well, so far these functions were at least hiding the &init_user_ns > part, they didn't just introduce XFS specific names for generic > things. That was precisely the reason they were introduced - the higher levels should already be doing namespace conversions before anything gets to the filesystem disk formatting routines. Hence the &init_user_ns bullshit for getting a raw uid is just noise. > It would be sad if every subsystem introduced their own names > when they don't like the generic ones. It was a symptom of the much larger problem: the userns maintainer refusing or ignoring *all* requests to change anything or answer any questions about how things like dump/restore is supposed to work across user namespaces. Hence, for better or for worse, we were forced to make up our own rules on how dump/restore and mapped ids are supposed to interact.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
