Re: [PATCH 2/2] NFSv4: Use the net namespace uniquifier if it is set

["bfields@fieldses.org" <bfields@fieldses.org>]

On Wed, Oct 07, 2020 at 09:34:17PM +0000, Trond Myklebust wrote:
> On Wed, 2020-10-07 at 17:25 -0400, J. Bruce Fields wrote:
> > On Wed, Oct 07, 2020 at 05:07:20PM -0400, trondmy@kernel.org wrote:
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > 
> > > If a container sets a net namespace specific uniquifier,
> > 
> > What you're actually checking is if nn->nfs_client != NULL, and
> > that's
> > pretty much always true.  (The only time it's NULL is an allocation
> > failure in some initialization code, I think.)
> > 
> > > +	struct nfs_net *nn = net_generic(clp->cl_net, nfs_net_id);
> > > +	struct nfs_netns_client *nn_clp = nn->nfs_client;
> > > +	const char *id;
> > > +	size_t len;
> > > +
> > >  	buf[0] = '\0';
> > > +
> > > +	if (nn_clp) {
> > 
> > Are you sure you don't mean
> > 
> > 	if (nn_clp->identifier)
> > 
> > ?
> > 
> > I think that's how you tell if someone's set it.
> 
> We're not holding the rcu_read_lock() here, so there is no point in
> dereferencing the identifier pointer. That would cause a toctou race...

Oh, right:

> > > +		rcu_read_lock();
> > > +		id = rcu_dereference(nn_clp->identifier);
> > > +		if (id && *id != '\0')

My bad, I saw the first check and missed that one somehow.

> > > +			len = strlcpy(buf, id, buflen);
> > > +		rcu_read_unlock();
> > > +		if (len)
> > > +			return len;
> 
> Oops. However I do need to ensure that 'len' is always initialised
> here.

OK.--b.

> 
> > > +	}
> > > +
> > >  	if (nfs4_client_id_uniquifier[0] != '\0')
> > >  		return strlcpy(buf, nfs4_client_id_uniquifier, buflen);
> > >  	return 0;
> > > @@ -6034,7 +6051,7 @@ nfs4_init_nonuniform_client_string(struct
> > > nfs_client *clp)
> > >  		1;
> > >  	rcu_read_unlock();
> > >  
> > > -	buflen = nfs4_get_uniquifier(buf, sizeof(buf));
> > > +	buflen = nfs4_get_uniquifier(clp, buf, sizeof(buf));
> > >  	if (buflen)
> > >  		len += buflen + 1;
> > >  
> > > @@ -6081,7 +6098,7 @@ nfs4_init_uniform_client_string(struct
> > > nfs_client *clp)
> > >  	len = 10 + 10 + 1 + 10 + 1 +
> > >  		strlen(clp->cl_rpcclient->cl_nodename) + 1;
> > >  
> > > -	buflen = nfs4_get_uniquifier(buf, sizeof(buf));
> > > +	buflen = nfs4_get_uniquifier(clp, buf, sizeof(buf));
> > >  	if (buflen)
> > >  		len += buflen + 1;
> > >  
> > > -- 
> > > 2.26.2
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
>