revisit pipe initialization and buffer management

- in dopipe(), get an already zeroed struct (PR_ZERO) instead of manually initialize each member (in pipe_create)
- rename pipespace() and pipe_free_kmem() to pipe_buffer_realloc() and pipe_buffer_free(): it is more evident that the functions works on the same thing
- in pipe_buffer_free(), return early and move the if-body as function body

No functional change intented.

ok anton@ visa@ mpi@

1 files changed, 42 insertions, 35 deletions

diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
index 91df07fdcaf..5397009835e 100644
--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: sys_pipe.c,v 1.93 2019/07/15 04:11:03 visa Exp $	*/
+/*	$OpenBSD: sys_pipe.c,v 1.94 2019/07/15 09:05:46 semarie Exp $	*/
 
 /*
  * Copyright (c) 1996 John S. Dyson
@@ -98,12 +98,13 @@ struct pool pipe_pool;
 
 int	dopipe(struct proc *, int *, int);
 void	pipeclose(struct pipe *);
-void	pipe_free_kmem(struct pipe *);
 int	pipe_create(struct pipe *);
 int	pipelock(struct pipe *);
 void	pipeunlock(struct pipe *);
 void	pipeselwakeup(struct pipe *);
-int	pipespace(struct pipe *, u_int);
+
+int	pipe_buffer_realloc(struct pipe *, u_int);
+void	pipe_buffer_free(struct pipe *);
 
 /*
  * The pipe system call for the DTYPE_PIPE type of pipes
@@ -143,11 +144,11 @@ dopipe(struct proc *p, int *ufds, int flags)
 
 	cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
-	rpipe = pool_get(&pipe_pool, PR_WAITOK);
+	rpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
 	error = pipe_create(rpipe);
 	if (error != 0)
 		goto free1;
-	wpipe = pool_get(&pipe_pool, PR_WAITOK);
+	wpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
 	error = pipe_create(wpipe);
 	if (error != 0)
 		goto free1;
@@ -213,24 +214,30 @@ free1:
  * If it fails it will return ENOMEM.
  */
 int
-pipespace(struct pipe *cpipe, u_int size)
+pipe_buffer_realloc(struct pipe *cpipe, u_int size)
 {
 	caddr_t buffer;
 
+	/* buffer uninitialized or pipe locked */
+	KASSERT((cpipe->pipe_buffer.buffer == NULL) ||
+	    (cpipe->pipe_state & PIPE_LOCK));
+
+	/* buffer should be empty */
+	KASSERT(cpipe->pipe_buffer.cnt == 0);
+
 	KERNEL_LOCK();
 	buffer = km_alloc(size, &kv_any, &kp_pageable, &kd_waitok);
 	KERNEL_UNLOCK();
-	if (buffer == NULL) {
+	if (buffer == NULL)
 		return (ENOMEM);
-	}
 
 	/* free old resources if we are resizing */
-	pipe_free_kmem(cpipe);
+	pipe_buffer_free(cpipe);
+
 	cpipe->pipe_buffer.buffer = buffer;
 	cpipe->pipe_buffer.size = size;
 	cpipe->pipe_buffer.in = 0;
 	cpipe->pipe_buffer.out = 0;
-	cpipe->pipe_buffer.cnt = 0;
 
 	atomic_add_int(&amountpipekva, cpipe->pipe_buffer.size);
 
@@ -245,19 +252,9 @@ pipe_create(struct pipe *cpipe)
 {
 	int error;
 
-	/* so pipe_free_kmem() doesn't follow junk pointer */
-	cpipe->pipe_buffer.buffer = NULL;
-	/*
-	 * protect so pipeclose() doesn't follow a junk pointer
-	 * if pipespace() fails.
-	 */
-	memset(&cpipe->pipe_sel, 0, sizeof(cpipe->pipe_sel));
-	cpipe->pipe_state = 0;
-	cpipe->pipe_peer = NULL;
-	cpipe->pipe_busy = 0;
 	sigio_init(&cpipe->pipe_sigio);
 
-	error = pipespace(cpipe, PIPE_SIZE);
+	error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
 	if (error != 0)
 		return (error);
 
@@ -306,6 +303,7 @@ pipeselwakeup(struct pipe *cpipe)
 		selwakeup(&cpipe->pipe_sel);
 	} else
 		KNOTE(&cpipe->pipe_sel.si_note, 0);
+
 	if (cpipe->pipe_state & PIPE_ASYNC)
 		pgsigio(&cpipe->pipe_sigio, SIGIO, 0);
 }
@@ -464,7 +462,7 @@ pipe_write(struct file *fp, struct uio *uio, int fflags)
 		if ((npipe <= LIMITBIGPIPES) &&
 		    (error = pipelock(wpipe)) == 0) {
 			if ((wpipe->pipe_buffer.cnt != 0) ||
-			    (pipespace(wpipe, BIG_PIPE_SIZE) != 0))
+			    (pipe_buffer_realloc(wpipe, BIG_PIPE_SIZE) != 0))
 				atomic_dec_int(&nbigpipe);
 			pipeunlock(wpipe);
 		} else
@@ -775,20 +773,29 @@ pipe_close(struct file *fp, struct proc *p)
 	return (0);
 }
 
+/*
+ * Free kva for pipe circular buffer.
+ * No pipe lock check as only called from pipe_buffer_realloc() and pipeclose()
+ */
 void
-pipe_free_kmem(struct pipe *cpipe)
+pipe_buffer_free(struct pipe *cpipe)
 {
-	u_int size = cpipe->pipe_buffer.size;
-
-	if (cpipe->pipe_buffer.buffer != NULL) {
-		KERNEL_LOCK();
-		km_free(cpipe->pipe_buffer.buffer, size, &kv_any, &kp_pageable);
-		KERNEL_UNLOCK();
-		atomic_sub_int(&amountpipekva, size);
-		cpipe->pipe_buffer.buffer = NULL;
-		if (size > PIPE_SIZE)
-			atomic_dec_int(&nbigpipe);
-	}
+	u_int size;
+
+	if (cpipe->pipe_buffer.buffer == NULL)
+		return;
+
+	size = cpipe->pipe_buffer.size;
+
+	KERNEL_LOCK();
+	km_free(cpipe->pipe_buffer.buffer, size, &kv_any, &kp_pageable);
+	KERNEL_UNLOCK();
+
+	cpipe->pipe_buffer.buffer = NULL;
+
+	atomic_sub_int(&amountpipekva, size);
+	if (size > PIPE_SIZE)
+		atomic_dec_int(&nbigpipe);
 }
 
 /*
@@ -827,7 +834,7 @@ pipeclose(struct pipe *cpipe)
 		/*
 		 * free resources
 		 */
-		pipe_free_kmem(cpipe);
+		pipe_buffer_free(cpipe);
 		pool_put(&pipe_pool, cpipe);
 	}
 }