From owner-freebsd-hackers  Sun May 30 19:18:59 1999
Delivered-To: freebsd-hackers@freebsd.org
Received: from apollo.backplane.com (apollo.backplane.com [209.157.86.2])
	by hub.freebsd.org (Postfix) with ESMTP id 5663E14C0D
	for <hackers@freebsd.org>; Sun, 30 May 1999 19:18:55 -0700 (PDT)
	(envelope-from dillon@apollo.backplane.com)
Received: (from dillon@localhost)
	by apollo.backplane.com (8.9.3/8.9.1) id TAA71169;
	Sun, 30 May 1999 19:18:53 -0700 (PDT)
	(envelope-from dillon)
Date: Sun, 30 May 1999 19:18:53 -0700 (PDT)
From: Matthew Dillon <dillon@apollo.backplane.com>
Message-Id: <199905310218.TAA71169@apollo.backplane.com>
To: Alan Cox <alc@cs.rice.edu>
Cc: hackers@freebsd.org
Subject: Re: pipe race
References:  <19990530152238.A81441@nonpc.cs.rice.edu>
Sender: owner-freebsd-hackers@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.ORG

    Ouch.  Definite problems in both the reader and the writer.  

    The writer calculates the writable space before locking, so if you
    have two simultanious writers where one blocks in uiomove(), both
    try to write to the same buffer area.  *plus* the counters get
    screwed up (and would probably lead to a crash)

    The reader has the same problem.

    If the writer is blocked or switches out of uiomove(), it can get 
    confused if the reader attempts to reset its buffer pointers at the
    same time ( prior to the write completing ):

        if ((rpipe->pipe_busy == 0) && (rpipe->pipe_state & PIPE_WANT)) {

		( this condition may not occur because the writer could
		be blocked or switched in uiomove(), so PIPE_WANT will
		not be set )

                rpipe->pipe_state &= ~(PIPE_WANT|PIPE_WANTW);
                wakeup(rpipe);
        } else if (rpipe->pipe_buffer.cnt < MINPIPESIZE) {
                /*
                 * If there is no more to read in the pipe, reset 
                 * its pointers to the beginning.  This improves 
                 * cache hit stats.
                 */
                if (rpipe->pipe_buffer.cnt == 0) {
                        if ((error == 0) && (error = pipelock(rpipe,1)) == 0) {
                                rpipe->pipe_buffer.in = 0;
                                rpipe->pipe_buffer.out = 0;
                                pipeunlock(rpipe);
                        }       
                }


    The writer can also get confused if it tries to resize the buffer.  It
    calculates whether to resize the buffer prior to locking, then resizes
    the buffer without rechecking the calculations.

    I should have patches soon.  I believe the reader-resetting-buffer-pointers
    problem is what caused my friend's data corruption.

						-Matt

:I took a look at the pipe code this morning.  Whether or not this is
:the cause of your problem, I don't know, but there sure are snippets
:of code that set off alarms in my head, e.g.,
:
:from pipe_read:
:
:                /*
:                 * normal pipe buffer receive
:                 */
:                if (rpipe->pipe_buffer.cnt > 0) {
:                        size = rpipe->pipe_buffer.size - rpipe->pipe_buffer.out;
:                        if (size > rpipe->pipe_buffer.cnt)
:                                size = rpipe->pipe_buffer.cnt;
:                        if (size > (u_int) uio->uio_resid)
:                                size = (u_int) uio->uio_resid;
:                        if ((error = pipelock(rpipe,1)) == 0) {
:                                error = uiomove( &rpipe->pipe_buffer.buffer[rpipe->pipe_buffer.out],
:                                        size, uio);
:                                pipeunlock(rpipe);
:                        }
:
:Suppose you block in pipelock.  Who's to say the "size" is still
:valid when you wake up?
:

					Matthew Dillon 
					<dillon@backplane.com>


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message