Re: [PATCH v1 4/4] NFSD: Handle full-length symlinks

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

 




> On Aug 1, 2018, at 10:14 AM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> 
> On Fri, Jul 27, 2018 at 11:19:10AM -0400, Chuck Lever wrote:
>> I've given up on the idea of zero-copy handling of SYMLINK on the
>> server side. This is because the Linux VFS symlink API requires the
>> symlink pathname to be in a NUL-terminated kmalloc'd buffer. The
>> NUL-termination is going to be problematic (watching out for
>> landing on a page boundary and dealing with a 4096-byte pathname).
>> 
>> I don't believe that SYMLINK creation is on a performance path or is
>> requested frequently enough that it will cause noticeable CPU cache
>> pollution due to data copies.
> 
> Sounds fine.
> 
> The limits here are weird.  In the v2 case the maximum length is
> NFS_MAXPATHLEN == 1024.  OK, I see, I guess that limit was imposed by
> the NFSv2 protocol.

Right.


> In the v3 case it's NFS3_MAXPATHLEN == PATH_MAX ==
> 4096.  But we were imposing an artificial max of 4095 just so we could
> fit the result in one page with null termination.
> 
> OK, makes sense to me (though I can't recall anyone actually complaining
> about that limit).

I haven't heard complaints either. I'm just trying to make this
code a little more sensible. :-)


> --b.
> 
> 
>> 
>> There will be two places where a transport callout will be necessary
>> to fill in the rqstp: one will be in the svc_fill_symlink_pathname()
>> helper that is used by NFSv2 and NFSv3, and the other will be in
>> nfsd4_decode_create().
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>> fs/nfsd/nfs3proc.c         |    2 +
>> fs/nfsd/nfsproc.c          |    2 +
>> include/linux/sunrpc/svc.h |    3 +-
>> net/sunrpc/svc.c           |   67 ++++++++++++++++----------------------------
>> 4 files changed, 31 insertions(+), 43 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index 8d1c2d1a..9eb8086 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -290,6 +290,7 @@
>> 		RETURN_STATUS(nfserr_nametoolong);
>> 
>> 	argp->tname = svc_fill_symlink_pathname(rqstp, &argp->first,
>> +						page_address(rqstp->rq_arg.pages[0]),
>> 						argp->tlen);
>> 	if (IS_ERR(argp->tname))
>> 		RETURN_STATUS(nfserrno(PTR_ERR(argp->tname)));
>> @@ -303,6 +304,7 @@
>> 	fh_init(&resp->fh, NFS3_FHSIZE);
>> 	nfserr = nfsd_symlink(rqstp, &resp->dirfh, argp->fname, argp->flen,
>> 						   argp->tname, &resp->fh);
>> +	kfree(argp->tname);
>> 	RETURN_STATUS(nfserr);
>> }
>> 
>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
>> index a6faee5..0d20fd1 100644
>> --- a/fs/nfsd/nfsproc.c
>> +++ b/fs/nfsd/nfsproc.c
>> @@ -454,6 +454,7 @@
>> 		return nfserr_nametoolong;
>> 
>> 	argp->tname = svc_fill_symlink_pathname(rqstp, &argp->first,
>> +						page_address(rqstp->rq_arg.pages[0]),
>> 						argp->tlen);
>> 	if (IS_ERR(argp->tname))
>> 		return nfserrno(PTR_ERR(argp->tname));
>> @@ -466,6 +467,7 @@
>> 	nfserr = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
>> 						 argp->tname, &newfh);
>> 
>> +	kfree(argp->tname);
>> 	fh_put(&argp->ffh);
>> 	fh_put(&newfh);
>> 	return nfserr;
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 43f88bd..73e130a 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -499,7 +499,8 @@ unsigned int	   svc_fill_write_vector(struct svc_rqst *rqstp,
>> 					 struct page **pages,
>> 					 struct kvec *first, size_t total);
>> char		  *svc_fill_symlink_pathname(struct svc_rqst *rqstp,
>> -					     struct kvec *first, size_t total);
>> +					     struct kvec *first, void *p,
>> +					     size_t total);
>> 
>> #define	RPC_MAX_ADDRBUFLEN	(63U)
>> 
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 2194ed5..d13e05f 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -1577,65 +1577,48 @@ unsigned int svc_fill_write_vector(struct svc_rqst *rqstp, struct page **pages,
>>  * svc_fill_symlink_pathname - Construct pathname argument for VFS symlink call
>>  * @rqstp: svc_rqst to operate on
>>  * @first: buffer containing first section of pathname
>> + * @p: buffer containing remaining section of pathname
>>  * @total: total length of the pathname argument
>>  *
>> - * Returns pointer to a NUL-terminated string, or an ERR_PTR. The buffer is
>> - * released automatically when @rqstp is recycled.
>> + * The VFS symlink API demands a NUL-terminated pathname in mapped memory.
>> + * Returns pointer to a NUL-terminated string, or an ERR_PTR. Caller must free
>> + * the returned string.
>>  */
>> char *svc_fill_symlink_pathname(struct svc_rqst *rqstp, struct kvec *first,
>> -				size_t total)
>> +				void *p, size_t total)
>> {
>> -	struct xdr_buf *arg = &rqstp->rq_arg;
>> -	struct page **pages;
>> -	char *result;
>> -
>> -	/* VFS API demands a NUL-terminated pathname. This function
>> -	 * uses a page from @rqstp as the pathname buffer, to enable
>> -	 * direct placement. Thus the total buffer size is PAGE_SIZE.
>> -	 * Space in this buffer for NUL-termination requires that we
>> -	 * cap the size of the returned symlink pathname just a
>> -	 * little early.
>> -	 */
>> -	if (total > PAGE_SIZE - 1)
>> -		return ERR_PTR(-ENAMETOOLONG);
>> +	size_t len, remaining;
>> +	char *result, *dst;
>> 
>> -	/* Some types of transport can present the pathname entirely
>> -	 * in rq_arg.pages. If not, then copy the pathname into one
>> -	 * page.
>> -	 */
>> -	pages = arg->pages;
>> -	WARN_ON_ONCE(arg->page_base != 0);
>> -	if (first->iov_base == 0) {
>> -		result = page_address(*pages);
>> -		result[total] = '\0';
>> -	} else {
>> -		size_t len, remaining;
>> -		char *dst;
>> +	result = kmalloc(total + 1, GFP_KERNEL);
>> +	if (!result)
>> +		return ERR_PTR(-ESERVERFAULT);
>> 
>> -		result = page_address(*(rqstp->rq_next_page++));
>> -		dst = result;
>> -		remaining = total;
>> +	dst = result;
>> +	remaining = total;
>> 
>> -		len = min_t(size_t, total, first->iov_len);
>> +	len = min_t(size_t, total, first->iov_len);
>> +	if (len) {
>> 		memcpy(dst, first->iov_base, len);
>> 		dst += len;
>> 		remaining -= len;
>> +	}
>> 
>> -		/* No more than one page left */
>> -		if (remaining) {
>> -			len = min_t(size_t, remaining, PAGE_SIZE);
>> -			memcpy(dst, page_address(*pages), len);
>> -			dst += len;
>> -		}
>> -
>> -		*dst = '\0';
>> +	if (remaining) {
>> +		len = min_t(size_t, remaining, PAGE_SIZE);
>> +		memcpy(dst, p, len);
>> +		dst += len;
>> 	}
>> 
>> -	/* Sanity check: we don't allow the pathname argument to
>> +	*dst = '\0';
>> +
>> +	/* Sanity check: Linux doesn't allow the pathname argument to
>> 	 * contain a NUL byte.
>> 	 */
>> -	if (strlen(result) != total)
>> +	if (strlen(result) != total) {
>> +		kfree(result);
>> 		return ERR_PTR(-EINVAL);
>> +	}
>> 	return result;
>> }
>> EXPORT_SYMBOL_GPL(svc_fill_symlink_pathname);

--
Chuck Lever



--
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