svn commit: r250027 - head/sys/kern

[Konstantin Belousov]

Author: kib
Date: Sun Apr 28 19:12:09 2013
New Revision: 250027
URL: http://svnweb.freebsd.org/changeset/base/250027

Log:
  Eliminate the layering violation in the kern_sendfile().  When quering
  the file size, use VOP_GETATTR() instead of accessing vnode vm_object
  un_pager.vnp.vnp_size.
  
  Take the shared vnode lock earlier to cover the added VOP_GETATTR()
  call and, as consequence, the whole internal sendfile loop.  Reduce vm
  object lock scope to not protect the local calculations.
  
  Note that this is the last misuse of the vnp_size in the tree, the
  others were removed from the ELF image activator by r230246.
  
  Reviewed by:	alc
  Tested by:	pho, bf (previous version)
  MFC after:	1 week

Modified:
  head/sys/kern/uipc_syscalls.c

Modified: head/sys/kern/uipc_syscalls.c
==============================================================================
--- head/sys/kern/uipc_syscalls.c	Sun Apr 28 18:40:55 2013	(r250026)
+++ head/sys/kern/uipc_syscalls.c	Sun Apr 28 19:12:09 2013	(r250027)
@@ -1902,8 +1902,10 @@ kern_sendfile(struct thread *td, struct 
 	struct mbuf *m = NULL;
 	struct sf_buf *sf;
 	struct vm_page *pg;
+	struct vattr va;
 	off_t off, xfsize, fsbytes = 0, sbytes = 0, rem = 0;
 	int error, hdrlen = 0, mnw = 0;
+	int bsize;
 	struct sendfile_sync *sfs = NULL;
 
 	/*
@@ -2102,6 +2104,16 @@ retry_space:
 		 */
 		space -= hdrlen;
 
+		error = vn_lock(vp, LK_SHARED);
+		if (error != 0)
+			goto done;
+		error = VOP_GETATTR(vp, &va, td->td_ucred);
+		if (error != 0) {
+			VOP_UNLOCK(vp, 0);
+			goto done;
+		}
+		bsize = vp->v_mount->mnt_stat.f_iosize;
+
 		/*
 		 * Loop and construct maximum sized mbuf chain to be bulk
 		 * dumped into socket buffer.
@@ -2111,7 +2123,6 @@ retry_space:
 			vm_offset_t pgoff;
 			struct mbuf *m0;
 
-			VM_OBJECT_WLOCK(obj);
 			/*
 			 * Calculate the amount to transfer.
 			 * Not to exceed a page, the EOF,
@@ -2121,12 +2132,11 @@ retry_space:
 			if (uap->nbytes)
 				rem = (uap->nbytes - fsbytes - loopbytes);
 			else
-				rem = obj->un_pager.vnp.vnp_size -
+				rem = va.va_size -
 				    uap->offset - fsbytes - loopbytes;
 			xfsize = omin(PAGE_SIZE - pgoff, rem);
 			xfsize = omin(space - loopbytes, xfsize);
 			if (xfsize <= 0) {
-				VM_OBJECT_WUNLOCK(obj);
 				done = 1;		/* all data sent */
 				break;
 			}
@@ -2136,7 +2146,6 @@ retry_space:
 			 * Let the outer loop figure out how to handle it.
 			 */
 			if (space <= loopbytes) {
-				VM_OBJECT_WUNLOCK(obj);
 				done = 0;
 				break;
 			}
@@ -2146,6 +2155,7 @@ retry_space:
 			 * if not found or wait and loop if busy.
 			 */
 			pindex = OFF_TO_IDX(off);
+			VM_OBJECT_WLOCK(obj);
 			pg = vm_page_grab(obj, pindex, VM_ALLOC_NOBUSY |
 			    VM_ALLOC_NORMAL | VM_ALLOC_WIRED | VM_ALLOC_RETRY);
 
@@ -2163,7 +2173,6 @@ retry_space:
 			else if (uap->flags & SF_NODISKIO)
 				error = EBUSY;
 			else {
-				int bsize;
 				ssize_t resid;
 
 				/*
@@ -2175,13 +2184,6 @@ retry_space:
 
 				/*
 				 * Get the page from backing store.
-				 */
-				error = vn_lock(vp, LK_SHARED);
-				if (error != 0)
-					goto after_read;
-				bsize = vp->v_mount->mnt_stat.f_iosize;
-
-				/*
 				 * XXXMAC: Because we don't have fp->f_cred
 				 * here, we pass in NOCRED.  This is probably
 				 * wrong, but is consistent with our original
@@ -2191,8 +2193,6 @@ retry_space:
 				    trunc_page(off), UIO_NOCOPY, IO_NODELOCKED |
 				    IO_VMIO | ((MAXBSIZE / bsize) << IO_SEQSHIFT),
 				    td->td_ucred, NOCRED, &resid, td);
-				VOP_UNLOCK(vp, 0);
-			after_read:
 				VM_OBJECT_WLOCK(obj);
 				vm_page_io_finish(pg);
 				if (!error)
@@ -2281,6 +2281,8 @@ retry_space:
 			}
 		}
 
+		VOP_UNLOCK(vp, 0);
+
 		/* Add the buffer chain to the socket buffer. */
 		if (m != NULL) {
 			int mlen, err;