Attachment 584796 Details for Bug 814626 – fix oops due to overrunning server's page array

[patch] fix oops due to overrunning server's page array

TMP (text/plain), 12.08 KB, created by J. Bruce Fields on 2012-05-15 22:02:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: J. Bruce Fields

Created: 2012-05-15 22:02:20 UTC

Size: 12.08 KB

patch

obsolete

>From b6999e0f2d6f5df2a0dc5f7ca99c9a05f0e2a98c Mon Sep 17 00:00:00 2001
>From: "J. Bruce Fields" <bfields@redhat.com>
>Date: Mon, 7 May 2012 16:00:23 -0400
>Subject: [PATCH 1/2] svcrpc: fix oops due to overrunning server's page array
>
>The rq_pages array has 1MB/PAGE_SIZE+2 elements.  The loop in svc_recv
>attempts to allocate sv_bufsz/PAGE_SIZE+2 pages.  But the NFS server is
>setting sv_bufsiz to over a megabyte, with the result that svc_recv may
>attempt to allocate sv_bufsz/PAGE_SIZE+3 pages and run past the end of
>the array, overwriting rq_respages.
>
>This was fixed upstream by this patch, with the following changelog,
>which doesn't note the bug described above.
>
>Note to preserve kabi, instead of replacing the sv_bufsz field, we
>retain it and append only one new field (sv_max_mesg).  To prevent
>gratuitous divergence from upstream we define a macro allowing us to
>refer to the existing sv_bufsz as sv_max_payload without removing the
>existing sv_bufsz field.
>
>Upstream changelog:
>
>There is some confusion about the meaning of 'bufsz' for a sunrpc server.
>In some cases it is the largest message that can be sent or received.  In
>other cases it is the largest 'payload' that can be included in a NFS
>message.
>
>In either case, it is not possible for both the request and the reply to be
>this large.  One of the request or reply may only be one page long, which
>fits nicely with NFS.
>
>So we remove 'bufsz' and replace it with two numbers: 'max_payload' and
>'max_mesg'.  Max_payload is the size that the server requests.  It is used
>by the server to check the max size allowed on a particular connection:
>depending on the protocol a lower limit might be used.
>
>max_mesg is the largest single message that can be sent or received.  It is
>calculated as the max_payload, rounded up to a multiple of PAGE_SIZE, and
>with PAGE_SIZE added to overhead.  Only one of the request and reply may be
>this size.  The other must be at most one page.
>
>Cc: Greg Banks <gnb@sgi.com>
>Cc: "J. Bruce Fields" <bfields@fieldses.org>
>Signed-off-by: Neil Brown <neilb@suse.de>
>Signed-off-by: Andrew Morton <akpm@osdl.org>
>Signed-off-by: Linus Torvalds <torvalds@osdl.org>
>
>Bugzilla: 814626
>Upstream: c6b0a9f87b82f25fa35206ec04b5160372eabab4
>---
> fs/nfsd/nfssvc.c           |    2 +-
> include/linux/sunrpc/svc.h |    4 +++-
> net/sunrpc/svc.c           |   17 ++++++++++-------
> net/sunrpc/svcsock.c       |   28 ++++++++++++++--------------
> 4 files changed, 28 insertions(+), 23 deletions(-)
>
>diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>index 71b785f..5514111 100644
>--- a/fs/nfsd/nfssvc.c
>+++ b/fs/nfsd/nfssvc.c
>@@ -230,7 +230,7 @@ int nfsd_create_serv(void)
> 	}
> 
> 	atomic_set(&nfsd_busy, 0);
>-	nfsd_serv = svc_create(&nfsd_program, NFSD_BUFSIZE - NFSSVC_MAXBLKSIZE + nfsd_max_blksize);
>+	nfsd_serv = svc_create(&nfsd_program, nfsd_max_blksize);
> 	if (nfsd_serv == NULL)
> 		err = -ENOMEM;
> 	svc_shutdown(nfsd_serv, nfsd_last_thread);
>diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>index 7bea29d..70979db 100644
>--- a/include/linux/sunrpc/svc.h
>+++ b/include/linux/sunrpc/svc.h
>@@ -35,7 +35,7 @@ struct svc_serv {
> 	struct svc_stat *	sv_stats;	/* RPC statistics */
> 	spinlock_t		sv_lock;
> 	unsigned int		sv_nrthreads;	/* # of server threads */
>-	unsigned int		sv_bufsz;	/* datagram buffer size */
>+	unsigned int		sv_bufsz;
> 	unsigned int		sv_xdrsize;	/* XDR buffer size */
> 
> 	struct list_head	sv_permsocks;	/* all permanent sockets */
>@@ -51,6 +51,8 @@ struct svc_serv {
> 	unsigned int		sv_maxconn;	/* max connections allowed or
> 						 * '0' causing max to be based
> 						 * on number of threads. */
>+#define sv_max_payload	sv_bufsz		/* datagram payload size */
>+	unsigned int		sv_max_mesg;	/* max_payload + 1 page for overheads */
> #endif
> };
> 
>diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>index 8bf9d71..81ada60 100644
>--- a/net/sunrpc/svc.c
>+++ b/net/sunrpc/svc.c
>@@ -39,7 +39,10 @@ svc_create(struct svc_program *prog, unsigned int bufsize)
> 	serv->sv_program   = prog;
> 	serv->sv_nrthreads = 1;
> 	serv->sv_stats     = prog->pg_stats;
>-	serv->sv_bufsz	   = bufsize? bufsize : 4096;
>+	if (bufsize > RPCSVC_MAXPAYLOAD)
>+		bufsize = RPCSVC_MAXPAYLOAD;
>+	serv->sv_max_payload = bufsize? bufsize : 4096;
>+	serv->sv_max_mesg  = roundup(serv->sv_max_payload + PAGE_SIZE, PAGE_SIZE);
> 	xdrsize = 0;
> 	while (prog) {
> 		prog->pg_lovers = prog->pg_nvers-1;
>@@ -119,9 +122,9 @@ svc_init_buffer(struct svc_rqst *rqstp, unsigned int size)
> 	int pages;
> 	int arghi;
> 	
>-	if (size > RPCSVC_MAXPAYLOAD)
>-		size = RPCSVC_MAXPAYLOAD;
>-	pages = 2 + (size+ PAGE_SIZE -1) / PAGE_SIZE;
>+	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
>+				       * We assume one is at most one page
>+				       */
> 	arghi = 0;
> 	BUG_ON(pages > RPCSVC_MAXPAGES);
> 	while (pages) {
>@@ -163,7 +166,7 @@ svc_create_thread(svc_thread_fn func, struct svc_serv *serv)
> 
> 	if (!(rqstp->rq_argp = kmalloc(serv->sv_xdrsize, GFP_KERNEL))
> 	 || !(rqstp->rq_resp = kmalloc(serv->sv_xdrsize, GFP_KERNEL))
>-	 || !svc_init_buffer(rqstp, serv->sv_bufsz))
>+	 || !svc_init_buffer(rqstp, serv->sv_max_mesg))
> 		goto out_thread;
> 
> 	serv->sv_nrthreads++;
>@@ -502,8 +505,8 @@ u32 svc_max_payload(const struct svc_rqst *rqstp)
> 
> 	if (rqstp->rq_sock->sk_sock->type == SOCK_DGRAM)
> 		max = RPCSVC_MAXPAYLOAD_UDP;
>-	if (rqstp->rq_server->sv_bufsz < max)
>-		max = rqstp->rq_server->sv_bufsz;
>+	if (rqstp->rq_server->sv_max_payload < max)
>+		max = rqstp->rq_server->sv_max_payload;
> 	return max;
> }
> EXPORT_SYMBOL_GPL(svc_max_payload);
>diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>index a0edff9..b840ea5 100644
>--- a/net/sunrpc/svcsock.c
>+++ b/net/sunrpc/svcsock.c
>@@ -200,13 +200,13 @@ svc_sock_enqueue(struct svc_sock *svsk)
> 	}
> 
> 	set_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
>-	if (((svsk->sk_reserved + serv->sv_bufsz)*2
>+	if (((svsk->sk_reserved + serv->sv_max_mesg)*2
> 	     > svc_sock_wspace(svsk))
> 	    && !test_bit(SK_CLOSE, &svsk->sk_flags)
> 	    && !test_bit(SK_CONN, &svsk->sk_flags)) {
> 		/* Don't enqueue while not enough space for reply */
> 		dprintk("svc: socket %p  no space, %d*2 > %ld, not enqueued\n",
>-			svsk->sk_sk, svsk->sk_reserved+serv->sv_bufsz,
>+			svsk->sk_sk, svsk->sk_reserved+serv->sv_max_mesg,
> 			svc_sock_wspace(svsk));
> 		goto out_unlock;
> 	}
>@@ -231,7 +231,7 @@ svc_sock_enqueue(struct svc_sock *svsk)
> 				rqstp, rqstp->rq_sock);
> 		rqstp->rq_sock = svsk;
> 		svsk->sk_inuse++;
>-		rqstp->rq_reserved = serv->sv_bufsz;
>+		rqstp->rq_reserved = serv->sv_max_mesg;
> 		svsk->sk_reserved += rqstp->rq_reserved;
> 		wake_up(&rqstp->rq_wait);
> 	} else {
>@@ -649,8 +649,8 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
> 	     * for one reply per thread.
> 	     */
> 	    svc_sock_setbufsize(svsk->sk_sock,
>-				(serv->sv_nrthreads+3) * serv->sv_bufsz,
>-				(serv->sv_nrthreads+3) * serv->sv_bufsz);
>+				(serv->sv_nrthreads+3) * serv->sv_max_mesg,
>+				(serv->sv_nrthreads+3) * serv->sv_max_mesg);
> 
> 	if ((rqstp->rq_deferred = svc_deferred_dequeue(svsk))) {
> 		svc_sock_received(svsk);
>@@ -759,8 +759,8 @@ svc_udp_init(struct svc_sock *svsk)
> 	 * svc_udp_recvfrom will re-adjust if necessary
> 	 */
> 	svc_sock_setbufsize(svsk->sk_sock,
>-			    3 * svsk->sk_server->sv_bufsz,
>-			    3 * svsk->sk_server->sv_bufsz);
>+			    3 * svsk->sk_server->sv_max_mesg,
>+			    3 * svsk->sk_server->sv_max_mesg);
> 
> 	set_bit(SK_DATA, &svsk->sk_flags); /* might have come in before data_ready set up */
> 	set_bit(SK_CHNGBUF, &svsk->sk_flags);
>@@ -1009,8 +1009,8 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
> 		 * as soon a a complete request arrives.
> 		 */
> 		svc_sock_setbufsize(svsk->sk_sock,
>-				    (serv->sv_nrthreads+3) * serv->sv_bufsz,
>-				    3 * serv->sv_bufsz);
>+				    (serv->sv_nrthreads+3) * serv->sv_max_mesg,
>+				    3 * serv->sv_max_mesg);
> 
> 	clear_bit(SK_DATA, &svsk->sk_flags);
> 
>@@ -1048,7 +1048,7 @@ svc_tcp_recvfrom(struct svc_rqst *rqstp)
> 		}
> 		svsk->sk_reclen &= 0x7fffffff;
> 		dprintk("svc: TCP record, %d bytes\n", svsk->sk_reclen);
>-		if (svsk->sk_reclen > serv->sv_bufsz) {
>+		if (svsk->sk_reclen > serv->sv_max_mesg) {
> 			printk(KERN_NOTICE "RPC: bad TCP reclen 0x%08lx (large)\n",
> 			       (unsigned long) svsk->sk_reclen);
> 			goto err_delete;
>@@ -1187,8 +1187,8 @@ svc_tcp_init(struct svc_sock *svsk)
> 		 * svc_tcp_recvfrom will re-adjust if necessary
> 		 */
> 		svc_sock_setbufsize(svsk->sk_sock,
>-				    3 * svsk->sk_server->sv_bufsz,
>-				    3 * svsk->sk_server->sv_bufsz);
>+				    3 * svsk->sk_server->sv_max_mesg,
>+				    3 * svsk->sk_server->sv_max_mesg);
> 
> 		set_bit(SK_CHNGBUF, &svsk->sk_flags);
> 		set_bit(SK_DATA, &svsk->sk_flags);
>@@ -1246,7 +1246,7 @@ svc_recv(struct svc_serv *serv, struct svc_rqst *rqstp, long timeout)
> 
> 
> 	/* now allocate needed pages.  If we get a failure, sleep briefly */
>-	pages = 2 + (serv->sv_bufsz + PAGE_SIZE -1) / PAGE_SIZE;
>+	pages = (serv->sv_max_mesg + PAGE_SIZE) / PAGE_SIZE;
> 	for (i=0; i < pages ; i++)
> 		while (rqstp->rq_pages[i] == NULL) {
> 			struct page *p = alloc_page(GFP_KERNEL);
>@@ -1292,7 +1292,7 @@ svc_recv(struct svc_serv *serv, struct svc_rqst *rqstp, long timeout)
> 	} else if ((svsk = svc_sock_dequeue(serv)) != NULL) {
> 		rqstp->rq_sock = svsk;
> 		svsk->sk_inuse++;
>-		rqstp->rq_reserved = serv->sv_bufsz;	
>+		rqstp->rq_reserved = serv->sv_max_mesg;
> 		svsk->sk_reserved += rqstp->rq_reserved;
> 	} else {
> 		/* No data pending. Go to sleep */
>-- 
>1.7.10.1
>
>
>From 8c76780c7af2ac90cf1f7d10e18469de04ab72a7 Mon Sep 17 00:00:00 2001
>From: NeilBrown <neilb@suse.de>
>Date: Fri, 26 Jan 2007 00:56:59 -0800
>Subject: [PATCH 2/2] [PATCH] knfsd: fix an NFSD bug with full sized,
> non-page-aligned reads
>
>NFSd assumes that largest number of pages that will be needed for a
>request+response is 2+N where N pages is the size of the largest permitted
>read/write request.  The '2' are 1 for the non-data part of the request, and 1
>for the non-data part of the reply.
>
>However, when a read request is not page-aligned, and we choose to use
>->sendfile to send it directly from the page cache, we may need N+1 pages to
>hold the whole reply.  This can overflow and array and cause an Oops.
>
>This patch increases size of the array for holding pages by one and makes sure
>that entry is NULL when it is not in use.
>
>Signed-off-by: Neil Brown <neilb@suse.de>
>Signed-off-by: Andrew Morton <akpm@osdl.org>
>Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>
>Upstream 250f3915183d377d36e012bac9caa7345ce465b8
>Bugzilla 814626
>---
> fs/nfsd/vfs.c              |    3 ++-
> include/linux/sunrpc/svc.h |    5 ++++-
> net/sunrpc/svcsock.c       |    2 ++
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>index 5c3513a..55ee852 100644
>--- a/fs/nfsd/vfs.c
>+++ b/fs/nfsd/vfs.c
>@@ -851,7 +851,8 @@ nfsd_read_actor(read_descriptor_t *desc, struct page *page, unsigned long offset
> 		rqstp->rq_res.page_len = size;
> 	} else if (page != pp[-1]) {
> 		get_page(page);
>-		put_page(*pp);
>+		if (*pp)
>+			put_page(*pp);
> 		*pp = page;
> 		rqstp->rq_resused++;
> 		rqstp->rq_res.page_len += size;
>diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>index 70979db..3416d15 100644
>--- a/include/linux/sunrpc/svc.h
>+++ b/include/linux/sunrpc/svc.h
>@@ -108,8 +108,11 @@ extern u32 svc_max_payload(const struct svc_rqst *rqstp);
>  *
>  * Each request/reply pair can have at most one "payload", plus two pages,
>  * one for the request, and one for the reply.
>+ * We using ->sendfile to return read data, we might need one extra page
>+ * if the request is not page-aligned.  So add another '1'.
>  */
>-#define RPCSVC_MAXPAGES		((RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_SIZE + 2)
>+#define RPCSVC_MAXPAGES		((RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_SIZE \
>+				+ 2 + 1)
> #define OLD_RPCSVC_MAXPAGES	((OLD_RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_SIZE + 2)
> 
> static inline u32 svc_getu32(struct kvec *iov)
>diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>index b840ea5..86607a4 100644
>--- a/net/sunrpc/svcsock.c
>+++ b/net/sunrpc/svcsock.c
>@@ -1254,6 +1254,8 @@ svc_recv(struct svc_serv *serv, struct svc_rqst *rqstp, long timeout)
> 				schedule_timeout_uninterruptible(msecs_to_jiffies(500));
> 			rqstp->rq_pages[i] = p;
> 		}
>+	rqstp->rq_pages[i++] = NULL; /* this might be seen in nfs_read_actor */
>+	BUG_ON(pages >= RPCSVC_MAXPAGES);
> 
> 	/* Make arg->head point to first page and arg->pages point to rest */
> 	arg = &rqstp->rq_arg;
>-- 
>1.7.10.1
>

Actions: View | Diff

Attachments on bug 814626: 578932 | 581493 | 582792 | 583282 | 584796