NFSD: Clean up legacy NFS SYMLINK argument XDR decoders
authorChuck Lever <chuck.lever@oracle.com>
Tue, 27 Mar 2018 14:54:21 +0000 (10:54 -0400)
committerJ. Bruce Fields <bfields@redhat.com>
Tue, 3 Apr 2018 19:08:16 +0000 (15:08 -0400)
Move common code in NFSD's legacy SYMLINK decoders into a helper.
The immediate benefits include:

 - one fewer data copies on transports that support DDP
 - consistent error checking across all versions
 - reduction of code duplication
 - support for both legal forms of SYMLINK requests on RDMA
   transports for all versions of NFS (in particular, NFSv2, for
   completeness)

In the long term, this helper is an appropriate spot to perform a
per-transport call-out to fill the pathname argument using, say,
RDMA Reads.

Filling the pathname in the proc function also means that eventually
the incoming filehandle can be interpreted so that filesystem-
specific memory can be allocated as a sink for the pathname
argument, rather than using anonymous pages.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
fs/nfsd/nfs3proc.c
fs/nfsd/nfs3xdr.c
fs/nfsd/nfsproc.c
fs/nfsd/nfsxdr.c
fs/nfsd/xdr.h
fs/nfsd/xdr3.h
fs/nfsd/xdr4.h
include/linux/sunrpc/svc.h
net/sunrpc/svc.c

index 2dd95ebf49354d03d2acf9dec5e9109b55c09bad..6259a4b8579faddce80cecdf133b4515d1e558de 100644 (file)
@@ -283,6 +283,16 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
        struct nfsd3_diropres *resp = rqstp->rq_resp;
        __be32  nfserr;
 
+       if (argp->tlen == 0)
+               RETURN_STATUS(nfserr_inval);
+       if (argp->tlen > NFS3_MAXPATHLEN)
+               RETURN_STATUS(nfserr_nametoolong);
+
+       argp->tname = svc_fill_symlink_pathname(rqstp, &argp->first,
+                                               argp->tlen);
+       if (IS_ERR(argp->tname))
+               RETURN_STATUS(nfserrno(PTR_ERR(argp->tname)));
+
        dprintk("nfsd: SYMLINK(3)  %s %.*s -> %.*s\n",
                                SVCFH_fmt(&argp->ffh),
                                argp->flen, argp->fname,
index e19fc5d8bcb5996b4f98d7da7a1299f263cbc0eb..3192b544a441506567423dc3b92c0200a76b80cf 100644 (file)
@@ -481,51 +481,24 @@ int
 nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p)
 {
        struct nfsd3_symlinkargs *args = rqstp->rq_argp;
-       unsigned int len, avail;
-       char *old, *new;
-       struct kvec *vec;
+       char *base = (char *)p;
+       size_t dlen;
 
        if (!(p = decode_fh(p, &args->ffh)) ||
-           !(p = decode_filename(p, &args->fname, &args->flen))
-               )
+           !(p = decode_filename(p, &args->fname, &args->flen)))
                return 0;
        p = decode_sattr3(p, &args->attrs);
 
-       /* now decode the pathname, which might be larger than the first page.
-        * As we have to check for nul's anyway, we copy it into a new page
-        * This page appears in the rq_res.pages list, but as pages_len is always
-        * 0, it won't get in the way
-        */
-       len = ntohl(*p++);
-       if (len == 0 || len > NFS3_MAXPATHLEN || len >= PAGE_SIZE)
-               return 0;
-       args->tname = new = page_address(*(rqstp->rq_next_page++));
-       args->tlen = len;
-       /* first copy and check from the first page */
-       old = (char*)p;
-       vec = &rqstp->rq_arg.head[0];
-       if ((void *)old > vec->iov_base + vec->iov_len)
-               return 0;
-       avail = vec->iov_len - (old - (char*)vec->iov_base);
-       while (len && avail && *old) {
-               *new++ = *old++;
-               len--;
-               avail--;
-       }
-       /* now copy next page if there is one */
-       if (len && !avail && rqstp->rq_arg.page_len) {
-               avail = min_t(unsigned int, rqstp->rq_arg.page_len, PAGE_SIZE);
-               old = page_address(rqstp->rq_arg.pages[0]);
-       }
-       while (len && avail && *old) {
-               *new++ = *old++;
-               len--;
-               avail--;
-       }
-       *new = '\0';
-       if (len)
-               return 0;
+       args->tlen = ntohl(*p++);
 
+       args->first.iov_base = p;
+       args->first.iov_len = rqstp->rq_arg.head[0].iov_len;
+       args->first.iov_len -= (char *)p - base;
+
+       dlen = args->first.iov_len + rqstp->rq_arg.page_len +
+              rqstp->rq_arg.tail[0].iov_len;
+       if (dlen < XDR_QUADLEN(args->tlen) << 2)
+               return 0;
        return 1;
 }
 
index 1995ea6bfd2baae21e230edaba4ffb93798dd5c4..f107f9fa8e158e611d185dc14329ea2ce1fcaa47 100644 (file)
@@ -449,17 +449,19 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
        struct svc_fh   newfh;
        __be32          nfserr;
 
+       if (argp->tlen > NFS_MAXPATHLEN)
+               return nfserr_nametoolong;
+
+       argp->tname = svc_fill_symlink_pathname(rqstp, &argp->first,
+                                               argp->tlen);
+       if (IS_ERR(argp->tname))
+               return nfserrno(PTR_ERR(argp->tname));
+
        dprintk("nfsd: SYMLINK  %s %.*s -> %.*s\n",
                SVCFH_fmt(&argp->ffh), argp->flen, argp->fname,
                argp->tlen, argp->tname);
 
        fh_init(&newfh, NFS_FHSIZE);
-       /*
-        * Crazy hack: the request fits in a page, and already-decoded
-        * attributes follow argp->tname, so it's safe to just write a
-        * null to ensure it's null-terminated:
-        */
-       argp->tname[argp->tlen] = '\0';
        nfserr = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
                                                 argp->tname, &newfh);
 
index db24ae8b67e021e71da10f65457230b3c85ba319..a43e8260520af9980e8c3374781c937c9d8df3b0 100644 (file)
@@ -70,22 +70,6 @@ decode_filename(__be32 *p, char **namp, unsigned int *lenp)
        return p;
 }
 
-static __be32 *
-decode_pathname(__be32 *p, char **namp, unsigned int *lenp)
-{
-       char            *name;
-       unsigned int    i;
-
-       if ((p = xdr_decode_string_inplace(p, namp, lenp, NFS_MAXPATHLEN)) != NULL) {
-               for (i = 0, name = *namp; i < *lenp; i++, name++) {
-                       if (*name == '\0')
-                               return NULL;
-               }
-       }
-
-       return p;
-}
-
 static __be32 *
 decode_sattr(__be32 *p, struct iattr *iap)
 {
@@ -384,14 +368,39 @@ int
 nfssvc_decode_symlinkargs(struct svc_rqst *rqstp, __be32 *p)
 {
        struct nfsd_symlinkargs *args = rqstp->rq_argp;
+       char *base = (char *)p;
+       size_t xdrlen;
 
        if (   !(p = decode_fh(p, &args->ffh))
-           || !(p = decode_filename(p, &args->fname, &args->flen))
-           || !(p = decode_pathname(p, &args->tname, &args->tlen)))
+           || !(p = decode_filename(p, &args->fname, &args->flen)))
                return 0;
-       p = decode_sattr(p, &args->attrs);
 
-       return xdr_argsize_check(rqstp, p);
+       args->tlen = ntohl(*p++);
+       if (args->tlen == 0)
+               return 0;
+
+       args->first.iov_base = p;
+       args->first.iov_len = rqstp->rq_arg.head[0].iov_len;
+       args->first.iov_len -= (char *)p - base;
+
+       /* This request is never larger than a page. Therefore,
+        * transport will deliver either:
+        * 1. pathname in the pagelist -> sattr is in the tail.
+        * 2. everything in the head buffer -> sattr is in the head.
+        */
+       if (rqstp->rq_arg.page_len) {
+               if (args->tlen != rqstp->rq_arg.page_len)
+                       return 0;
+               p = rqstp->rq_arg.tail[0].iov_base;
+       } else {
+               xdrlen = XDR_QUADLEN(args->tlen);
+               if (xdrlen > args->first.iov_len - (8 * sizeof(__be32)))
+                       return 0;
+               p += xdrlen;
+       }
+       decode_sattr(p, &args->attrs);
+
+       return 1;
 }
 
 int
index a765c414015e0ce3c606c7a110806592d3cb31e8..ea7cca3a64b7655bae55af6647603cdc9b51cde8 100644 (file)
@@ -72,6 +72,7 @@ struct nfsd_symlinkargs {
        char *                  tname;
        unsigned int            tlen;
        struct iattr            attrs;
+       struct kvec             first;
 };
 
 struct nfsd_readdirargs {
index deccf7f691e9703c0ea7624d074db141a9e4bd12..2cb29e961a760f6797abfe9317c4903fba868c93 100644 (file)
@@ -90,6 +90,7 @@ struct nfsd3_symlinkargs {
        char *                  tname;
        unsigned int            tlen;
        struct iattr            attrs;
+       struct kvec             first;
 };
 
 struct nfsd3_readdirargs {
index 7cbc129092fe325750b56685a0a849c9bd0b9000..468020fe2a07ebf79e544532bcb1d0ea4f935c4d 100644 (file)
@@ -110,6 +110,7 @@ struct nfsd4_create {
                struct {
                        u32 datalen;
                        char *data;
+                       struct kvec first;
                } link;   /* NF4LNK */
                struct {
                        u32 specdata1;
@@ -124,6 +125,7 @@ struct nfsd4_create {
 };
 #define cr_datalen     u.link.datalen
 #define cr_data                u.link.data
+#define cr_first       u.link.first
 #define cr_specdata1   u.dev.specdata1
 #define cr_specdata2   u.dev.specdata2
 
index fb3fcacc1e98c4fedd730a96cc3598be02dc1a5d..574368e8a16f389eeb249d46f8cc7d9ecad9d449 100644 (file)
@@ -497,6 +497,8 @@ struct svc_pool *  svc_pool_for_cpu(struct svc_serv *serv, int cpu);
 char *            svc_print_addr(struct svc_rqst *, char *, size_t);
 unsigned int      svc_fill_write_vector(struct svc_rqst *rqstp,
                                         struct kvec *first, size_t total);
+char             *svc_fill_symlink_pathname(struct svc_rqst *rqstp,
+                                            struct kvec *first, size_t total);
 
 #define        RPC_MAX_ADDRBUFLEN      (63U)
 
index a155e2de19aa7628a6e6f921c6e41eef132c3160..30a4226baf03e1677a8b06d7a285d124d88a54ed 100644 (file)
@@ -1575,3 +1575,70 @@ unsigned int svc_fill_write_vector(struct svc_rqst *rqstp, struct kvec *first,
        return i;
 }
 EXPORT_SYMBOL_GPL(svc_fill_write_vector);
+
+/**
+ * svc_fill_symlink_pathname - Construct pathname argument for VFS symlink call
+ * @rqstp: svc_rqst to operate on
+ * @first: buffer containing first 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.
+ */
+char *svc_fill_symlink_pathname(struct svc_rqst *rqstp, struct kvec *first,
+                               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);
+
+       /* 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 = page_address(*(rqstp->rq_next_page++));
+               dst = result;
+               remaining = total;
+
+               len = min_t(size_t, total, first->iov_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';
+       }
+
+       /* Sanity check: we don't allow the pathname argument to
+        * contain a NUL byte.
+        */
+       if (strlen(result) != total)
+               return ERR_PTR(-EINVAL);
+       return result;
+}
+EXPORT_SYMBOL_GPL(svc_fill_symlink_pathname);