[PATCH v2 RFC] userns: Convert xfs to use kuid/kgid where appropriate

[Brian Foster]

On 06/25/2013 04:08 PM, Dwight Engen wrote:
> On Tue, 25 Jun 2013 12:46:19 -0400
> Brian Foster <bfoster at redhat.com> wrote:
> 
>> On 06/24/2013 09:10 AM, Dwight Engen wrote:
...
>>> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
>>> index 96e344e..2c35b13 100644
>>> --- a/fs/xfs/xfs_icache.c
>>> +++ b/fs/xfs/xfs_icache.c
>>> @@ -617,7 +617,7 @@ restart:
>>>  
>>>  /*
>>>   * Background scanning to trim post-EOF preallocated space. This
>>> is queued
>>> - * based on the 'background_prealloc_discard_period' tunable (5m
>>> by default).
>>> + * based on the 'speculative_prealloc_lifetime' tunable (5m by
>>> default). */
>>>  STATIC void
>>>  xfs_queue_eofblocks(
>>> @@ -1202,11 +1202,11 @@ xfs_inode_match_id(
>>>  	struct xfs_eofblocks	*eofb)
>>>  {
>>>  	if (eofb->eof_flags & XFS_EOF_FLAGS_UID &&
>>> -	    ip->i_d.di_uid != eofb->eof_uid)
>>> +	    !uid_eq(VFS_I(ip)->i_uid, eofb->eof_uid))
>>>  		return 0;
>>>  
>>>  	if (eofb->eof_flags & XFS_EOF_FLAGS_GID &&
>>> -	    ip->i_d.di_gid != eofb->eof_gid)
>>> +	    !gid_eq(VFS_I(ip)->i_gid, eofb->eof_gid))
>>
>> More of a question... since we're originally comparing against the
>> on-disk values, is the separate internal structure strictly necessary
>> for making eofblocks userns aware?
> 
> Not sure I fully understand your question, we were comparing on-disk
> uid/gid values to unconverted eof values because xfs didn't support the
> eof ioctl callers passing in ids from a userns. I believe part
> of the idea of userns is that i_uid is an opaque type, hence the use of
> _eq() comparators and why we have to convert eof_[ug]id if we want to
> compare them to i_uid as opposed to di_uid.
> 

That latter point was my question, why does this code want/need to
compare to the i_uid as opposed to di_uid. It seems like technically you
could push the conversion up and not have to change much else.

It's not terribly important since this code is moving into the separate
xfs_eofblocks structure anyway. I'm not against it I guess, I just
wanted to be on the same page as to the intent of the change. I suppose
it makes sense if the idea is that core kernel code should be carrying
around kuid types in general.

...
>> This should probably go into xfs_icache.h along with the
>> aforementioned conversion helper.
>>
>> As I mentioned previously, I have some code around that creates an
>> internal version of the eofblocks structure. The main differences are
>> the name (xfs_eofblocks_internal) and I did the conversion down in
>> xfs_icache.c since I wasn't changing anything that affected the
>> ioctl().
>>
>> I can throw it up on the list for reference or if it's of any use as a
>> base for this work...
> 
> If you have time to put it up that'd be great, but no biggie if not I'll
> write a conversion routine. Thanks for looking at this.
> 

I'll forward it along momentarily...

Brian

>> Brian
>>
>>>  /*
>>>   * Various platform dependent calls that don't fit anywhere else
>>>   */
>>> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
>>> index b75c9bb..57e2c18 100644
>>> --- a/fs/xfs/xfs_qm.c
>>> +++ b/fs/xfs/xfs_qm.c
>>> @@ -1651,8 +1651,8 @@ xfs_qm_write_sb_changes(
>>>  int
>>>  xfs_qm_vop_dqalloc(
>>>  	struct xfs_inode	*ip,
>>> -	uid_t			uid,
>>> -	gid_t			gid,
>>> +	xfs_dqid_t		uid,
>>> +	xfs_dqid_t		gid,
>>>  	prid_t			prid,
>>>  	uint			flags,
>>>  	struct xfs_dquot	**O_udqpp,
>>> @@ -1697,7 +1697,7 @@ xfs_qm_vop_dqalloc(
>>>  			 * holding ilock.
>>>  			 */
>>>  			xfs_iunlock(ip, lockflags);
>>> -			if ((error = xfs_qm_dqget(mp, NULL,
>>> (xfs_dqid_t) uid,
>>> +			if ((error = xfs_qm_dqget(mp, NULL, uid,
>>>  						 XFS_DQ_USER,
>>>  						 XFS_QMOPT_DQALLOC
>>> | XFS_QMOPT_DOWARN,
>>> @@ -1723,7 +1723,7 @@ xfs_qm_vop_dqalloc(
>>>  	if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
>>>  		if (ip->i_d.di_gid != gid) {
>>>  			xfs_iunlock(ip, lockflags);
>>> -			if ((error = xfs_qm_dqget(mp, NULL,
>>> (xfs_dqid_t)gid,
>>> +			if ((error = xfs_qm_dqget(mp, NULL, gid,
>>>  						 XFS_DQ_GROUP,
>>>  						 XFS_QMOPT_DQALLOC
>>> | XFS_QMOPT_DOWARN,
>>> @@ -1842,7 +1842,7 @@ xfs_qm_vop_chown_reserve(
>>>  			XFS_QMOPT_RES_RTBLKS :
>>> XFS_QMOPT_RES_REGBLKS; 
>>>  	if (XFS_IS_UQUOTA_ON(mp) && udqp &&
>>> -	    ip->i_d.di_uid !=
>>> (uid_t)be32_to_cpu(udqp->q_core.d_id)) {
>>> +	    ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
>>>  		delblksudq = udqp;
>>>  		/*
>>>  		 * If there are delayed allocation blocks, then we
>>> have to diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
>>> index c38068f..5f0bfe8 100644
>>> --- a/fs/xfs/xfs_quota.h
>>> +++ b/fs/xfs/xfs_quota.h
>>> @@ -320,8 +320,8 @@ extern int
>>> xfs_trans_reserve_quota_bydquots(struct xfs_trans *, struct
>>> xfs_mount *, struct xfs_dquot *, struct xfs_dquot *, long, long,
>>> uint); 
>>> -extern int xfs_qm_vop_dqalloc(struct xfs_inode *, uid_t, gid_t,
>>> prid_t, uint,
>>> -		struct xfs_dquot **, struct xfs_dquot **);
>>> +extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t,
>>> xfs_dqid_t,
>>> +		prid_t, uint, struct xfs_dquot **, struct
>>> xfs_dquot **); extern void xfs_qm_vop_create_dqattach(struct
>>> xfs_trans *, struct xfs_inode *, struct xfs_dquot *, struct
>>> xfs_dquot *); extern int xfs_qm_vop_rename_dqattach(struct
>>> xfs_inode **); @@ -341,8 +341,9 @@ extern void
>>> xfs_qm_unmount_quotas(struct xfs_mount *); 
>>>  #else
>>>  static inline int
>>> -xfs_qm_vop_dqalloc(struct xfs_inode *ip, uid_t uid, gid_t gid,
>>> prid_t prid,
>>> -		uint flags, struct xfs_dquot **udqp, struct
>>> xfs_dquot **gdqp) +xfs_qm_vop_dqalloc(struct xfs_inode *ip,
>>> xfs_dqid_t uid, xfs_dqid_t gid,
>>> +		prid_t prid, uint flags, struct xfs_dquot **udqp,
>>> +		struct xfs_dquot **gdqp)
>>>  {
>>>  	*udqp = NULL;
>>>  	*gdqp = NULL;
>>> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
>>> index 195a403..c50306e 100644
>>> --- a/fs/xfs/xfs_symlink.c
>>> +++ b/fs/xfs/xfs_symlink.c
>>> @@ -384,7 +384,9 @@ xfs_symlink(
>>>  	/*
>>>  	 * Make sure that we have allocated dquot(s) on disk.
>>>  	 */
>>> -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(),
>>> current_fsgid(), prid,
>>> +	error = xfs_qm_vop_dqalloc(dp,
>>> +			xfs_kuid_to_disk(current_fsuid()),
>>> +			xfs_kgid_to_disk(current_fsgid()), prid,
>>>  			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
>>> &udqp, &gdqp); if (error)
>>>  		goto std_return;
>>> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
>>> index 0176bb2..94f4f9f6 100644
>>> --- a/fs/xfs/xfs_vnodeops.c
>>> +++ b/fs/xfs/xfs_vnodeops.c
>>> @@ -515,7 +515,9 @@ xfs_create(
>>>  	/*
>>>  	 * Make sure that we have allocated dquot(s) on disk.
>>>  	 */
>>> -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(),
>>> current_fsgid(), prid,
>>> +	error = xfs_qm_vop_dqalloc(dp,
>>> +			xfs_kuid_to_disk(current_fsuid()),
>>> +			xfs_kgid_to_disk(current_fsgid()), prid,
>>>  			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
>>> &udqp, &gdqp); if (error)
>>>  		return error;
>>> diff --git a/init/Kconfig b/init/Kconfig
>>> index 9d3a788..8083ffd 100644
>>> --- a/init/Kconfig
>>> +++ b/init/Kconfig
>>> @@ -1065,7 +1065,6 @@ config IPC_NS
>>>  
>>>  config USER_NS
>>>  	bool "User namespace"
>>> -	depends on UIDGID_CONVERTED
>>>  	select UIDGID_STRICT_TYPE_CHECKS
>>>  
>>>  	default n
>>> @@ -1099,21 +1098,9 @@ config NET_NS
>>>  
>>>  endif # NAMESPACES
>>>  
>>> -config UIDGID_CONVERTED
>>> -	# True if all of the selected software conmponents are
>>> known
>>> -	# to have uid_t and gid_t converted to kuid_t and kgid_t
>>> -	# where appropriate and are otherwise safe to use with
>>> -	# the user namespace.
>>> -	bool
>>> -	default y
>>> -
>>> -	# Filesystems
>>> -	depends on XFS_FS = n
>>> -
>>>  config UIDGID_STRICT_TYPE_CHECKS
>>>  	bool "Require conversions between uid/gids and their
>>> internal representation"
>>> -	depends on UIDGID_CONVERTED
>>> -	default n
>>> +	default y
>>>  	help
>>>  	 While the nececessary conversions are being added to all
>>> subsystems this option allows the code to continue to build for
>>> unconverted subsystems.
>>>
>>
>