Re: [PATCH v2 6/7] nfs: get a reference to the credential in ff_layout_alloc_lseg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 04/29/2016 06:49 AM, Jeff Layton wrote:
> Oh! The original patch should be dropped. I should have sent a self-NAK 
> on it before. I'll do that now...

Great, thanks!

Anna

> 
> Thanks,
> Jeff
> 
> On Thu, 2016-04-28 at 16:37 -0400, Anna Schumaker wrote:
>> Hi Jeff,
>>
>> It looks like this patch removes the mirror->uid and mirror->gid
>> parameters that your earlier patch (nfs: don't match flexfiles
>> mirrors that have different creds) makes use of.  Should that patch
>> be updated, or does this series make that patch obsolete?
>>
>> Thanks,
>> Anna
>>
>> On 04/21/2016 08:51 PM, Jeff Layton wrote:
>>> We're just as likely to have allocation problems here as we would
>>> if we
>>> delay looking up the credential like we currently do. Fix the code
>>> to
>>> get a rpc_cred reference early, as soon as the mirror is set up.
>>>
>>> This allows us to eliminate the mirror early if there is a problem
>>> getting an rpc credential. This also allows us to drop the uid/gid
>>> from the layout_mirror struct as well.
>>>
>>> In the event that we find an existing mirror where this one would
>>> go, we
>>> swap in the new creds unconditionally, and drop the reference to
>>> the old
>>> one.
>>>
>>> Note that the old ff_layout_update_mirror_cred function wouldn't
>>> set
>>> this pointer unless the DS version was 3, but we don't know what
>>> the DS
>>> version is at this point. I'm a little unclear on why it did that
>>> as you
>>> still need creds to talk to v4 servers as well. I have the code set
>>> it regardless of the DS version here.
>>>
>>> Also note the change to using generic creds instead of calling
>>> lookup_cred directly. With that change, we also need to populate
>>> the
>>> group_info pointer in the acred as some functions expect that to
>>> never
>>> be NULL. Instead of allocating one every time however, we can
>>> allocate
>>> one when the module is loaded and share it since the group_info is
>>> refcounted.
>>>
>>> Signed-off-by: Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
>>> ---
>>>  fs/nfs/flexfilelayout/flexfilelayout.c    | 41
>>> ++++++++++++++++++++++-----
>>>  fs/nfs/flexfilelayout/flexfilelayout.h    |  2 --
>>>  fs/nfs/flexfilelayout/flexfilelayoutdev.c | 47 ++-----------------
>>> ------------
>>>  3 files changed, 36 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c
>>> b/fs/nfs/flexfilelayout/flexfilelayout.c
>>> index 432d5b0009c7..19806caa8ff2 100644
>>> --- a/fs/nfs/flexfilelayout/flexfilelayout.c
>>> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
>>> @@ -26,6 +26,8 @@
>>>  
>>>  #define FF_LAYOUT_POLL_RETRY_MAX     (15*HZ)
>>>  
>>> +static struct group_info	*ff_zero_group;
>>> +
>>>  static struct pnfs_layout_hdr *
>>>  ff_layout_alloc_layout_hdr(struct inode *inode, gfp_t gfp_flags)
>>>  {
>>> @@ -407,8 +409,9 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr
>>> *lh,
>>>  		struct nfs4_ff_layout_mirror *mirror;
>>>  		struct nfs4_deviceid devid;
>>>  		struct nfs4_deviceid_node *idnode;
>>> -		u32 ds_count;
>>> -		u32 fh_count;
>>> +		struct auth_cred acred = { .group_info =
>>> ff_zero_group };
>>> +		struct rpc_cred	*cred;
>>> +		u32 ds_count, fh_count, id;
>>>  		int j;
>>>  
>>>  		rc = -EIO;
>>> @@ -484,24 +487,39 @@ ff_layout_alloc_lseg(struct pnfs_layout_hdr
>>> *lh,
>>>  		fls->mirror_array[i]->fh_versions_cnt = fh_count;
>>>  
>>>  		/* user */
>>> -		rc = decode_name(&stream, &fls->mirror_array[i]-
>>>> uid);
>>> +		rc = decode_name(&stream, &id);
>>>  		if (rc)
>>>  			goto out_err_free;
>>>  
>>> +		acred.uid = make_kuid(&init_user_ns, id);
>>> +
>>>  		/* group */
>>> -		rc = decode_name(&stream, &fls->mirror_array[i]-
>>>> gid);
>>> +		rc = decode_name(&stream, &id);
>>>  		if (rc)
>>>  			goto out_err_free;
>>>  
>>> +		acred.gid = make_kgid(&init_user_ns, id);
>>> +
>>> +		/* find the cred for it */
>>> +		cred = rpc_lookup_generic_cred(&acred, 0,
>>> gfp_flags);
>>> +		if (IS_ERR(cred)) {
>>> +			rc = PTR_ERR(cred);
>>> +			goto out_err_free;
>>> +		}
>>> +
>>> +		rcu_assign_pointer(fls->mirror_array[i]->cred,
>>> cred);
>>> +
>>>  		mirror = ff_layout_add_mirror(lh, fls-
>>>> mirror_array[i]);
>>>  		if (mirror != fls->mirror_array[i]) {
>>> +			/* swap cred ptrs so free_mirror will
>>> clean up old */
>>> +			fls->mirror_array[i]->cred = xchg(&mirror-
>>>> cred, cred);
>>>  			ff_layout_free_mirror(fls-
>>>> mirror_array[i]);
>>>  			fls->mirror_array[i] = mirror;
>>>  		}
>>>  
>>> -		dprintk("%s: uid %d gid %d\n", __func__,
>>> -			fls->mirror_array[i]->uid,
>>> -			fls->mirror_array[i]->gid);
>>> +		dprintk("%s: uid %u gid %u\n", __func__,
>>> +			from_kuid(&init_user_ns, acred.uid),
>>> +			from_kgid(&init_user_ns, acred.gid));
>>>  	}
>>>  
>>>  	p = xdr_inline_decode(&stream, 4);
>>> @@ -2226,6 +2244,11 @@ static int __init
>>> nfs4flexfilelayout_init(void)
>>>  {
>>>  	printk(KERN_INFO "%s: NFSv4 Flexfile Layout Driver
>>> Registering...\n",
>>>  	       __func__);
>>> +	if (!ff_zero_group) {
>>> +		ff_zero_group = groups_alloc(0);
>>> +		if (!ff_zero_group)
>>> +			return -ENOMEM;
>>> +	}
>>>  	return pnfs_register_layoutdriver(&flexfilelayout_type);
>>>  }
>>>  
>>> @@ -2234,6 +2257,10 @@ static void __exit
>>> nfs4flexfilelayout_exit(void)
>>>  	printk(KERN_INFO "%s: NFSv4 Flexfile Layout Driver
>>> Unregistering...\n",
>>>  	       __func__);
>>>  	pnfs_unregister_layoutdriver(&flexfilelayout_type);
>>> +	if (ff_zero_group) {
>>> +		put_group_info(ff_zero_group);
>>> +		ff_zero_group = NULL;
>>> +	}
>>>  }
>>>  
>>>  MODULE_ALIAS("nfs-layouttype4-4");
>>> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h
>>> b/fs/nfs/flexfilelayout/flexfilelayout.h
>>> index dd353bb7dc0a..c29fc853ce74 100644
>>> --- a/fs/nfs/flexfilelayout/flexfilelayout.h
>>> +++ b/fs/nfs/flexfilelayout/flexfilelayout.h
>>> @@ -76,8 +76,6 @@ struct nfs4_ff_layout_mirror {
>>>  	u32				fh_versions_cnt;
>>>  	struct nfs_fh			*fh_versions;
>>>  	nfs4_stateid			stateid;
>>> -	u32				uid;
>>> -	u32				gid;
>>>  	struct rpc_cred			*cred;
>>>  	atomic_t			ref;
>>>  	spinlock_t			lock;
>>> diff --git a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> index baee22929174..6ddd8a5c5ae0 100644
>>> --- a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> +++ b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
>>> @@ -302,42 +302,6 @@ int ff_layout_track_ds_error(struct
>>> nfs4_flexfile_layout *flo,
>>>  	return 0;
>>>  }
>>>  
>>> -/* currently we only support AUTH_NONE and AUTH_SYS */
>>> -static rpc_authflavor_t
>>> -nfs4_ff_layout_choose_authflavor(struct nfs4_ff_layout_mirror
>>> *mirror)
>>> -{
>>> -	if (mirror->uid == (u32)-1)
>>> -		return RPC_AUTH_NULL;
>>> -	return RPC_AUTH_UNIX;
>>> -}
>>> -
>>> -/* fetch cred for NFSv3 DS */
>>> -static int ff_layout_update_mirror_cred(struct
>>> nfs4_ff_layout_mirror *mirror,
>>> -				      struct nfs4_pnfs_ds *ds)
>>> -{
>>> -	if (ds->ds_clp && !mirror->cred &&
>>> -	    mirror->mirror_ds->ds_versions[0].version == 3) {
>>> -		struct rpc_auth *auth = ds->ds_clp->cl_rpcclient-
>>>> cl_auth;
>>> -		struct rpc_cred *cred;
>>> -		struct auth_cred acred = {
>>> -			.uid = make_kuid(&init_user_ns, mirror-
>>>> uid),
>>> -			.gid = make_kgid(&init_user_ns, mirror-
>>>> gid),
>>> -		};
>>> -
>>> -		/* AUTH_NULL ignores acred */
>>> -		cred = auth->au_ops->lookup_cred(auth, &acred, 0);
>>> -		if (IS_ERR(cred)) {
>>> -			dprintk("%s: lookup_cred failed with
>>> %ld\n",
>>> -				__func__, PTR_ERR(cred));
>>> -			return PTR_ERR(cred);
>>> -		} else {
>>> -			if (cmpxchg(&mirror->cred, NULL, cred))
>>> -				put_rpccred(cred);
>>> -		}
>>> -	}
>>> -	return 0;
>>> -}
>>> -
>>>  static struct rpc_cred *
>>>  ff_layout_get_mirror_cred(struct nfs4_ff_layout_mirror *mirror,
>>> u32 iomode)
>>>  {
>>> @@ -386,7 +350,6 @@ nfs4_ff_layout_prepare_ds(struct
>>> pnfs_layout_segment *lseg, u32 ds_idx,
>>>  	struct inode *ino = lseg->pls_layout->plh_inode;
>>>  	struct nfs_server *s = NFS_SERVER(ino);
>>>  	unsigned int max_payload;
>>> -	rpc_authflavor_t flavor;
>>>  
>>>  	if (!ff_layout_mirror_valid(lseg, mirror)) {
>>>  		pr_err_ratelimited("NFS: %s: No data server for
>>> offset index %d\n",
>>> @@ -402,9 +365,7 @@ nfs4_ff_layout_prepare_ds(struct
>>> pnfs_layout_segment *lseg, u32 ds_idx,
>>>  	/* matching smp_wmb() in _nfs4_pnfs_v3/4_ds_connect */
>>>  	smp_rmb();
>>>  	if (ds->ds_clp)
>>> -		goto out_update_creds;
>>> -
>>> -	flavor = nfs4_ff_layout_choose_authflavor(mirror);
>>> +		goto out;
>>>  
>>>  	/* FIXME: For now we assume the server sent only one
>>> version of NFS
>>>  	 * to use for the DS.
>>> @@ -413,7 +374,7 @@ nfs4_ff_layout_prepare_ds(struct
>>> pnfs_layout_segment *lseg, u32 ds_idx,
>>>  			     dataserver_retrans,
>>>  			     mirror->mirror_ds-
>>>> ds_versions[0].version,
>>>  			     mirror->mirror_ds-
>>>> ds_versions[0].minor_version,
>>> -			     flavor);
>>> +			     RPC_AUTH_UNIX);
>>>  
>>>  	/* connect success, check rsize/wsize limit */
>>>  	if (ds->ds_clp) {
>>> @@ -438,11 +399,7 @@ nfs4_ff_layout_prepare_ds(struct
>>> pnfs_layout_segment *lseg, u32 ds_idx,
>>>  		} else
>>>  			pnfs_error_mark_layout_for_return(ino,
>>> lseg);
>>>  		ds = NULL;
>>> -		goto out;
>>>  	}
>>> -out_update_creds:
>>> -	if (ff_layout_update_mirror_cred(mirror, ds))
>>> -		ds = NULL;
>>>  out:
>>>  	return ds;
>>>  }
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
>> in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux