From 240160d48b39bc424cb3549197221a5f94a45ab2 Mon Sep 17 00:00:00 2001 From: Brian Feldman Date: Sun, 22 Feb 2004 23:00:14 +0000 Subject: [PATCH] Correct some major SMP-harmful problems in the pipe implementation. First of all, PIPE_EOF is not checked pervasively after everything that can drop the pipe mutex and msleep(), so fix. Additionally, though it might not harm anything, pipelock() and pipeunlock() are not used consistently. Third, the kqueue support functions do not use the pipe mutex correctly. Last, but absolutely not least, is a race: if pipe_busy is not set on the closing side of the pipe, the other side that is trying to write to that will crash BECAUSE PIPE_EOF IS NOT SET! Unconditionally set PIPE_EOF, and get rid of all the lockups/crashes I have seen trying to build ports. --- sys/kern/sys_pipe.c | 107 +++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 41 deletions(-) diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c index 3170847ef861..79a9804a9988 100644 --- a/sys/kern/sys_pipe.c +++ b/sys/kern/sys_pipe.c @@ -435,29 +435,6 @@ pipespace(cpipe, size) return (0); } -/* - * Initialize and allocate VM and memory for pipe. The structure - * will start out zero'd from the ctor, so we just manage the kmem. - */ -static int -pipe_create(pipe) - struct pipe *pipe; -{ - int error; - - /* - * Reduce to 1/4th pipe size if we're over our global max. - */ - if (amountpipekva > maxpipekva / 2) - error = pipespace(pipe, SMALL_PIPE_SIZE); - else - error = pipespace(pipe, PIPE_SIZE); - if (error) - return (error); - - return (0); -} - /* * lock a pipe for I/O, blocking other access */ @@ -511,6 +488,35 @@ pipeselwakeup(cpipe) KNOTE(&cpipe->pipe_sel.si_note, 0); } +/* + * Initialize and allocate VM and memory for pipe. The structure + * will start out zero'd from the ctor, so we just manage the kmem. + */ +static int +pipe_create(pipe) + struct pipe *pipe; +{ + int error; + + PIPE_LOCK(pipe); + pipelock(pipe, 0); + PIPE_UNLOCK(pipe); + /* + * Reduce to 1/4th pipe size if we're over our global max. + */ + if (amountpipekva > maxpipekva / 2) + error = pipespace(pipe, SMALL_PIPE_SIZE); + else + error = pipespace(pipe, PIPE_SIZE); + PIPE_LOCK(pipe); + pipeunlock(pipe); + PIPE_UNLOCK(pipe); + if (error) + return (error); + + return (0); +} + /* ARGSUSED */ static int pipe_read(fp, uio, active_cred, flags, td) @@ -870,6 +876,10 @@ pipe_direct_write(wpipe, uio) wpipe->pipe_state |= PIPE_DIRECTW; pipelock(wpipe, 0); + if (wpipe->pipe_state & PIPE_EOF) { + error = EPIPE; + goto error2; + } PIPE_UNLOCK(wpipe); error = pipe_build_write_buffer(wpipe, uio); PIPE_LOCK(wpipe); @@ -901,6 +911,8 @@ pipe_direct_write(wpipe, uio) } pipelock(wpipe,0); + if (wpipe->pipe_state & PIPE_EOF) + error = EPIPE; if (wpipe->pipe_state & PIPE_DIRECTW) { /* * this bit of trickery substitutes a kernel buffer for @@ -912,6 +924,7 @@ pipe_direct_write(wpipe, uio) pipe_destroy_write_buffer(wpipe); PIPE_LOCK(wpipe); } +error2: pipeunlock(wpipe); return (error); @@ -965,10 +978,14 @@ pipe_write(fp, uio, active_cred, flags, td) (wpipe->pipe_buffer.cnt == 0)) { if ((error = pipelock(wpipe, 1)) == 0) { - PIPE_UNLOCK(wpipe); - if (pipespace(wpipe, BIG_PIPE_SIZE) == 0) - atomic_add_int(&nbigpipe, 1); - PIPE_LOCK(wpipe); + if (wpipe->pipe_state & PIPE_EOF) + error = EPIPE; + else { + PIPE_UNLOCK(wpipe); + if (pipespace(wpipe, BIG_PIPE_SIZE) == 0) + atomic_add_int(&nbigpipe, 1); + PIPE_LOCK(wpipe); + } pipeunlock(wpipe); } } @@ -1028,15 +1045,13 @@ pipe_write(fp, uio, active_cred, flags, td) } error = msleep(wpipe, PIPE_MTX(rpipe), PRIBIO | PCATCH, "pipbww", 0); - if (wpipe->pipe_state & PIPE_EOF) + if (wpipe->pipe_state & PIPE_EOF) { + error = EPIPE; break; + } if (error) break; } - if (wpipe->pipe_state & PIPE_EOF) { - error = EPIPE; - break; - } space = wpipe->pipe_buffer.size - wpipe->pipe_buffer.cnt; @@ -1050,9 +1065,11 @@ pipe_write(fp, uio, active_cred, flags, td) int segsize; /* first segment to transfer */ /* - * It is possible for a direct write to - * slip in on us... handle it here... + * It is possible for a direct write/EOF to + * slip in on us... handle them here... */ + if (wpipe->pipe_state & PIPE_EOF) + goto lost_wpipe; if (wpipe->pipe_state & PIPE_DIRECTW) { pipeunlock(wpipe); goto retrywrite; @@ -1133,6 +1150,7 @@ pipe_write(fp, uio, active_cred, flags, td) panic("Pipe buffer overflow"); } +lost_wpipe: pipeunlock(wpipe); } if (error) @@ -1455,9 +1473,10 @@ pipeclose(cpipe) * If the other side is blocked, wake it up saying that * we want to close it down. */ + cpipe->pipe_state |= PIPE_EOF; while (cpipe->pipe_busy) { wakeup(cpipe); - cpipe->pipe_state |= PIPE_WANT | PIPE_EOF; + cpipe->pipe_state |= PIPE_WANT; msleep(cpipe, PIPE_MTX(cpipe), PRIBIO, "pipecl", 0); } @@ -1481,10 +1500,12 @@ pipeclose(cpipe) * doing that, or the pipe might disappear out from under * us. */ + pipelock(cpipe, 0); PIPE_UNLOCK(cpipe); pipe_free_kmem(cpipe); PIPE_LOCK(cpipe); cpipe->pipe_present = 0; + pipeunlock(cpipe); /* * If both endpoints are now closed, release the memory for the @@ -1507,22 +1528,25 @@ pipe_kqfilter(struct file *fp, struct knote *kn) struct pipe *cpipe; cpipe = kn->kn_fp->f_data; + PIPE_LOCK(cpipe); switch (kn->kn_filter) { case EVFILT_READ: kn->kn_fop = &pipe_rfiltops; break; case EVFILT_WRITE: kn->kn_fop = &pipe_wfiltops; - cpipe = cpipe->pipe_peer; - if (!cpipe->pipe_present) + if (!cpipe->pipe_peer->pipe_present) { /* other end of pipe has been closed */ + PIPE_UNLOCK(cpipe); return (EPIPE); + } + cpipe = cpipe->pipe_peer; break; default: + PIPE_UNLOCK(cpipe); return (1); } - PIPE_LOCK(cpipe); SLIST_INSERT_HEAD(&cpipe->pipe_sel.si_note, kn, kn_selnext); PIPE_UNLOCK(cpipe); return (0); @@ -1533,13 +1557,14 @@ filt_pipedetach(struct knote *kn) { struct pipe *cpipe = (struct pipe *)kn->kn_fp->f_data; + PIPE_LOCK(cpipe); if (kn->kn_filter == EVFILT_WRITE) { - if (cpipe->pipe_peer == NULL) + if (!cpipe->pipe_peer->pipe_present) { + PIPE_UNLOCK(cpipe); return; + } cpipe = cpipe->pipe_peer; } - - PIPE_LOCK(cpipe); SLIST_REMOVE(&cpipe->pipe_sel.si_note, kn, knote, kn_selnext); PIPE_UNLOCK(cpipe); }