[PATCH] NFS: clean up the new symlink code

  - Now that the VFS no longer uses it, we don't need to cache the symlink
    string length.
  - Make ->readlink() take page offset+length arguments
  - Fix up page under/overflow checking on the readlink XDR code so that
    it matches read/write.
Signed-off-by: Trond Myklebust <trond.myklebust@fys.uio.no>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

fs/nfs/nfs2xdr.c

@@ -57,7 +57,7 @@ extern int			nfs_stat_to_errno(int stat);
 
 #define NFS_attrstat_sz		(1+NFS_fattr_sz)
 #define NFS_diropres_sz		(1+NFS_fhandle_sz+NFS_fattr_sz)
-#define NFS_readlinkres_sz	(1)
+#define NFS_readlinkres_sz	(2)
 #define NFS_readres_sz		(1+NFS_fattr_sz+1)
 #define NFS_writeres_sz         (NFS_attrstat_sz)
 #define NFS_stat_sz		(1)
@@ -530,7 +530,6 @@ static int
 nfs_xdr_readlinkargs(struct rpc_rqst *req, u32 *p, struct nfs_readlinkargs *args)
 {
 	struct rpc_auth *auth = req->rq_task->tk_auth;
-	unsigned int count = args->count - 5;
 	unsigned int replen;
 
 	p = xdr_encode_fhandle(p, args->fh);
@@ -538,7 +537,7 @@ nfs_xdr_readlinkargs(struct rpc_rqst *req, u32 *p, struct nfs_readlinkargs *args
 
 	/* Inline the page array */
 	replen = (RPC_REPHDRSIZE + auth->au_rslack + NFS_readlinkres_sz) << 2;
-	xdr_inline_pages(&req->rq_rcv_buf, replen, args->pages, 0, count);
+	xdr_inline_pages(&req->rq_rcv_buf, replen, args->pages, args->pgbase, args->pglen);
 	return 0;
 }
 
@@ -550,32 +549,38 @@ nfs_xdr_readlinkres(struct rpc_rqst *req, u32 *p, void *dummy)
 {
 	struct xdr_buf *rcvbuf = &req->rq_rcv_buf;
 	struct kvec *iov = rcvbuf->head;
-	unsigned int hdrlen;
-	u32	*strlen, len;
-	char	*string;
+	int hdrlen, len, recvd;
+	char	*kaddr;
 	int	status;
 
 	if ((status = ntohl(*p++)))
 		return -nfs_stat_to_errno(status);
+	/* Convert length of symlink */
+	len = ntohl(*p++);
+	if (len >= rcvbuf->page_len || len <= 0) {
+		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
+		return -ENAMETOOLONG;
+	}
 	hdrlen = (u8 *) p - (u8 *) iov->iov_base;
-	if (iov->iov_len > hdrlen) {
+	if (iov->iov_len < hdrlen) {
+		printk(KERN_WARNING "NFS: READLINK reply header overflowed:"
+				"length %d > %Zu\n", hdrlen, iov->iov_len);
+		return -errno_NFSERR_IO;
+	} else if (iov->iov_len != hdrlen) {
 		dprintk("NFS: READLINK header is short. iovec will be shifted.\n");
 		xdr_shift_buf(rcvbuf, iov->iov_len - hdrlen);
 	}
-
-	strlen = (u32*)kmap_atomic(rcvbuf->pages[0], KM_USER0);
-	/* Convert length of symlink */
-	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len) {
-		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
-		kunmap_atomic(strlen, KM_USER0);
-		return -ENAMETOOLONG;
+	recvd = req->rq_rcv_buf.len - hdrlen;
+	if (recvd < len) {
+		printk(KERN_WARNING "NFS: server cheating in readlink reply: "
+				"count %u > recvd %u\n", len, recvd);
+		return -EIO;
 	}
-	*strlen = len;
+
 	/* NULL terminate the string we got */
-	string = (char *)(strlen + 1);
-	string[len] = '\0';
-	kunmap_atomic(strlen, KM_USER0);
+	kaddr = (char *)kmap_atomic(rcvbuf->pages[0], KM_USER0);
+	kaddr[len+rcvbuf->page_base] = '\0';
+	kunmap_atomic(kaddr, KM_USER0);
 	return 0;
 }
 

fs/nfs/nfs3proc.c

@@ -202,13 +202,14 @@ static int nfs3_proc_access(struct inode *inode, struct nfs_access_entry *entry)
 	return status;
 }
 
-static int
-nfs3_proc_readlink(struct inode *inode, struct page *page)
+static int nfs3_proc_readlink(struct inode *inode, struct page *page,
+		unsigned int pgbase, unsigned int pglen)
 {
 	struct nfs_fattr	fattr;
 	struct nfs3_readlinkargs args = {
 		.fh		= NFS_FH(inode),
-		.count		= PAGE_CACHE_SIZE,
+		.pgbase		= pgbase,
+		.pglen		= pglen,
 		.pages		= &page
 	};
 	int			status;

fs/nfs/nfs3xdr.c

@@ -67,7 +67,7 @@ extern int			nfs_stat_to_errno(int);
 #define NFS3_wccstat_sz		(1+NFS3_wcc_data_sz)
 #define NFS3_lookupres_sz	(1+NFS3_fh_sz+(2 * NFS3_post_op_attr_sz))
 #define NFS3_accessres_sz	(1+NFS3_post_op_attr_sz+1)
-#define NFS3_readlinkres_sz	(1+NFS3_post_op_attr_sz)
+#define NFS3_readlinkres_sz	(1+NFS3_post_op_attr_sz+1)
 #define NFS3_readres_sz		(1+NFS3_post_op_attr_sz+3)
 #define NFS3_writeres_sz	(1+NFS3_wcc_data_sz+4)
 #define NFS3_createres_sz	(1+NFS3_fh_sz+NFS3_post_op_attr_sz+NFS3_wcc_data_sz)
@@ -698,7 +698,6 @@ static int
 nfs3_xdr_readlinkargs(struct rpc_rqst *req, u32 *p, struct nfs3_readlinkargs *args)
 {
 	struct rpc_auth *auth = req->rq_task->tk_auth;
-	unsigned int count = args->count - 5;
 	unsigned int replen;
 
 	p = xdr_encode_fhandle(p, args->fh);
@@ -706,7 +705,7 @@ nfs3_xdr_readlinkargs(struct rpc_rqst *req, u32 *p, struct nfs3_readlinkargs *ar
 
 	/* Inline the page array */
 	replen = (RPC_REPHDRSIZE + auth->au_rslack + NFS3_readlinkres_sz) << 2;
-	xdr_inline_pages(&req->rq_rcv_buf, replen, args->pages, 0, count);
+	xdr_inline_pages(&req->rq_rcv_buf, replen, args->pages, args->pgbase, args->pglen);
 	return 0;
 }
 
@@ -718,9 +717,8 @@ nfs3_xdr_readlinkres(struct rpc_rqst *req, u32 *p, struct nfs_fattr *fattr)
 {
 	struct xdr_buf *rcvbuf = &req->rq_rcv_buf;
 	struct kvec *iov = rcvbuf->head;
-	unsigned int hdrlen;
-	u32	*strlen, len;
-	char	*string;
+	int hdrlen, len, recvd;
+	char	*kaddr;
 	int	status;
 
 	status = ntohl(*p++);
@@ -729,25 +727,33 @@ nfs3_xdr_readlinkres(struct rpc_rqst *req, u32 *p, struct nfs_fattr *fattr)
 	if (status != 0)
 		return -nfs_stat_to_errno(status);
 
+	/* Convert length of symlink */
+	len = ntohl(*p++);
+	if (len >= rcvbuf->page_len || len <= 0) {
+		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
+		return -ENAMETOOLONG;
+	}
+
 	hdrlen = (u8 *) p - (u8 *) iov->iov_base;
-	if (iov->iov_len > hdrlen) {
+	if (iov->iov_len < hdrlen) {
+		printk(KERN_WARNING "NFS: READLINK reply header overflowed:"
+				"length %d > %Zu\n", hdrlen, iov->iov_len);
+		return -errno_NFSERR_IO;
+	} else if (iov->iov_len != hdrlen) {
 		dprintk("NFS: READLINK header is short. iovec will be shifted.\n");
 		xdr_shift_buf(rcvbuf, iov->iov_len - hdrlen);
 	}
-
-	strlen = (u32*)kmap_atomic(rcvbuf->pages[0], KM_USER0);
-	/* Convert length of symlink */
-	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len) {
-		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
-		kunmap_atomic(strlen, KM_USER0);
-		return -ENAMETOOLONG;
+	recvd = req->rq_rcv_buf.len - hdrlen;
+	if (recvd < len) {
+		printk(KERN_WARNING "NFS: server cheating in readlink reply: "
+				"count %u > recvd %u\n", len, recvd);
+		return -EIO;
 	}
-	*strlen = len;
+
 	/* NULL terminate the string we got */
-	string = (char *)(strlen + 1);
-	string[len] = '\0';
-	kunmap_atomic(strlen, KM_USER0);
+	kaddr = (char*)kmap_atomic(rcvbuf->pages[0], KM_USER0);
+	kaddr[len+rcvbuf->page_base] = '\0';
+	kunmap_atomic(kaddr, KM_USER0);
 	return 0;
 }
 

fs/nfs/nfs4proc.c

@@ -1173,11 +1173,13 @@ static int nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry)
  * Both of these changes to the XDR layer would in fact be quite
  * minor, but I decided to leave them for a subsequent patch.
  */
-static int _nfs4_proc_readlink(struct inode *inode, struct page *page)
+static int _nfs4_proc_readlink(struct inode *inode, struct page *page,
+		unsigned int pgbase, unsigned int pglen)
 {
 	struct nfs4_readlink args = {
 		.fh       = NFS_FH(inode),
-		.count    = PAGE_CACHE_SIZE,
+		.pgbase	  = pgbase,
+		.pglen    = pglen,
 		.pages    = &page,
 	};
 	struct rpc_message msg = {
@@ -1189,13 +1191,14 @@ static int _nfs4_proc_readlink(struct inode *inode, struct page *page)
 	return rpc_call_sync(NFS_CLIENT(inode), &msg, 0);
 }
 
-static int nfs4_proc_readlink(struct inode *inode, struct page *page)
+static int nfs4_proc_readlink(struct inode *inode, struct page *page,
+		unsigned int pgbase, unsigned int pglen)
 {
 	struct nfs4_exception exception = { };
 	int err;
 	do {
 		err = nfs4_handle_exception(NFS_SERVER(inode),
-				_nfs4_proc_readlink(inode, page),
+				_nfs4_proc_readlink(inode, page, pgbase, pglen),
 				&exception);
 	} while (exception.retry);
 	return err;

fs/nfs/nfs4xdr.c

@@ -1027,7 +1027,6 @@ static int encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg
 static int encode_readlink(struct xdr_stream *xdr, const struct nfs4_readlink *readlink, struct rpc_rqst *req)
 {
 	struct rpc_auth *auth = req->rq_task->tk_auth;
-	unsigned int count = readlink->count - 5;
 	unsigned int replen;
 	uint32_t *p;
 
@@ -1036,10 +1035,11 @@ static int encode_readlink(struct xdr_stream *xdr, const struct nfs4_readlink *r
 
 	/* set up reply kvec
 	 *    toplevel_status + taglen + rescount + OP_PUTFH + status
-	 *      + OP_READLINK + status  = 7
+	 *      + OP_READLINK + status + string length = 8
 	 */
-	replen = (RPC_REPHDRSIZE + auth->au_rslack + 7) << 2;
-	xdr_inline_pages(&req->rq_rcv_buf, replen, readlink->pages, 0, count);
+	replen = (RPC_REPHDRSIZE + auth->au_rslack + 8) << 2;
+	xdr_inline_pages(&req->rq_rcv_buf, replen, readlink->pages,
+			readlink->pgbase, readlink->pglen);
 	
 	return 0;
 }
@@ -3053,21 +3053,30 @@ static int decode_readlink(struct xdr_stream *xdr, struct rpc_rqst *req)
 {
 	struct xdr_buf *rcvbuf = &req->rq_rcv_buf;
 	struct kvec *iov = rcvbuf->head;
-	uint32_t *strlen;
-	unsigned int hdrlen, len;
-	char *string;
+	int hdrlen, len, recvd;
+	uint32_t *p;
+	char *kaddr;
 	int status;
 
 	status = decode_op_hdr(xdr, OP_READLINK);
 	if (status)
 		return status;
 
+	/* Convert length of symlink */
+	READ_BUF(4);
+	READ32(len);
+	if (len >= rcvbuf->page_len || len <= 0) {
+		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
+		return -ENAMETOOLONG;
+	}
 	hdrlen = (char *) xdr->p - (char *) iov->iov_base;
-	if (iov->iov_len > hdrlen) {
-		dprintk("NFS: READLINK header is short. iovec will be shifted.\n");
-		xdr_shift_buf(rcvbuf, iov->iov_len - hdrlen);
-
+	recvd = req->rq_rcv_buf.len - hdrlen;
+	if (recvd < len) {
+		printk(KERN_WARNING "NFS: server cheating in readlink reply: "
+				"count %u > recvd %u\n", len, recvd);
+		return -EIO;
 	}
+	xdr_read_pages(xdr, len);
 	/*
 	 * The XDR encode routine has set things up so that
 	 * the link text will be copied directly into the
@@ -3075,18 +3084,9 @@ static int decode_readlink(struct xdr_stream *xdr, struct rpc_rqst *req)
 	 * and and null-terminate the text (the VFS expects
 	 * null-termination).
 	 */
-	strlen = (uint32_t *) kmap_atomic(rcvbuf->pages[0], KM_USER0);
-	len = ntohl(*strlen);
-	if (len > rcvbuf->page_len) {
-		dprintk(KERN_WARNING "nfs: server returned giant symlink!\n");
-		kunmap_atomic(strlen, KM_USER0);
-		return -ENAMETOOLONG;
-	}
-	*strlen = len;
-
-	string = (char *)(strlen + 1);
-	string[len] = '\0';
-	kunmap_atomic(strlen, KM_USER0);
+	kaddr = (char *)kmap_atomic(rcvbuf->pages[0], KM_USER0);
+	kaddr[len+rcvbuf->page_base] = '\0';
+	kunmap_atomic(kaddr, KM_USER0);
 	return 0;
 }
 

fs/nfs/proc.c

@@ -140,12 +140,13 @@ nfs_proc_lookup(struct inode *dir, struct qstr *name,
 	return status;
 }
 
-static int
-nfs_proc_readlink(struct inode *inode, struct page *page)
+static int nfs_proc_readlink(struct inode *inode, struct page *page,
+		unsigned int pgbase, unsigned int pglen)
 {
 	struct nfs_readlinkargs	args = {
 		.fh		= NFS_FH(inode),
-		.count		= PAGE_CACHE_SIZE,
+		.pgbase		= pgbase,
+		.pglen		= pglen,
 		.pages		= &page
 	};
 	int			status;

fs/nfs/symlink.c

@@ -28,25 +28,25 @@
 /* Symlink caching in the page cache is even more simplistic
  * and straight-forward than readdir caching.
  *
- * We place the length at the beginning of the page, in host byte order,
- * followed by the string.  The XDR response verification will NUL-terminate
- * it.  In the very end of page we store pointer to struct page in question,
+ * At the beginning of the page we store pointer to struct page in question,
  * simplifying nfs_put_link() (if inode got invalidated we can't find the page
  * to be freed via pagecache lookup).
+ * The NUL-terminated string follows immediately thereafter.
  */
 
 struct nfs_symlink {
-	u32 length;
-	char body[PAGE_SIZE - sizeof(u32) - sizeof(struct page *)];
 	struct page *page;
-} __attribute__((packed));	/* this must be page-sized */
+	char body[];
+};
 
 static int nfs_symlink_filler(struct inode *inode, struct page *page)
 {
+	const unsigned int pgbase = offsetof(struct nfs_symlink, body);
+	const unsigned int pglen = PAGE_SIZE - pgbase;
 	int error;
 
 	lock_kernel();
-	error = NFS_PROTO(inode)->readlink(inode, page);
+	error = NFS_PROTO(inode)->readlink(inode, page, pgbase, pglen);
 	unlock_kernel();
 	if (error < 0)
 		goto error;
@@ -79,15 +79,10 @@ static int nfs_follow_link(struct dentry *dentry, struct nameidata *nd)
 		goto getlink_read_error;
 	}
 	p = kmap(page);
-	if (p->length > sizeof(p->body) - 1)
-		goto too_long;
 	p->page = page;
 	nd_set_link(nd, p->body);
 	return 0;
 
-too_long:
-	err = ERR_PTR(-ENAMETOOLONG);
-	kunmap(page);
 getlink_read_error:
 	page_cache_release(page);
 read_failed:

include/linux/nfs_xdr.h

@@ -361,7 +361,8 @@ struct nfs_diropok {
 
 struct nfs_readlinkargs {
 	struct nfs_fh *		fh;
-	unsigned int		count;
+	unsigned int		pgbase;
+	unsigned int		pglen;
 	struct page **		pages;
 };
 
@@ -455,7 +456,8 @@ struct nfs3_accessres {
 
 struct nfs3_readlinkargs {
 	struct nfs_fh *		fh;
-	unsigned int		count;
+	unsigned int		pgbase;
+	unsigned int		pglen;
 	struct page **		pages;
 };
 
@@ -570,7 +572,8 @@ struct nfs4_readdir_res {
 
 struct nfs4_readlink {
 	const struct nfs_fh *		fh;
-	u32				count;   /* zero-copy data */
+	unsigned int			pgbase;
+	unsigned int			pglen;   /* zero-copy data */
 	struct page **			pages;   /* zero-copy data */
 };
 
@@ -673,7 +676,8 @@ struct nfs_rpc_ops {
 	int	(*lookup)  (struct inode *, struct qstr *,
 			    struct nfs_fh *, struct nfs_fattr *);
 	int	(*access)  (struct inode *, struct nfs_access_entry *);
-	int	(*readlink)(struct inode *, struct page *);
+	int	(*readlink)(struct inode *, struct page *, unsigned int,
+			    unsigned int);
 	int	(*read)    (struct nfs_read_data *);
 	int	(*write)   (struct nfs_write_data *);
 	int	(*commit)  (struct nfs_write_data *);