[PATCH] 2.6.13: POSIX violation in pipes on ia64 for kernels > 2.6.10

[PATCH] 2.6.13: POSIX violation in pipes on ia64 for kernels > 2.6.10

[Jeff Licquia]

On architectures where PAGE_SIZE > PIPE_BUF, a read of PIPE_BUF bytes
from a full pipe does not change the pipe's write state.  This behavior
is different from kernels 2.6.9 and earlier, and it triggers failures of
the LSB tests.  

This patch fixes two parts of this problem: "short" writes are allowed
to partially succeed if the whole write won't fit on the current buffer,
and the last buffer for a full pipe is "held back" and gradually
released in PIPE_BUF blocks as reads happen.  This patch has been tested
against the LSB runtime test suite, with no kernel-related failures.

A more detailed description of the problem can be found on the
linux-ia64 list:

http://marc.theaimsgroup.com/?l=linux-ia64&m=112906384826364

Please CC me on replies, thanks.

Signed-off-by: Jeff Licquia <licquia@progeny.com>

--- linux-2.6.13-orig/fs/pipe.c	2005-10-10 13:51:38.000000000 -0500
+++ linux-2.6.13/fs/pipe.c	2005-10-12 17:45:34.000000000 -0500
@@ -15,6 +15,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/uio.h>
 #include <linux/highmem.h>
+#include <linux/limits.h>
 
 #include <asm/uaccess.h>
 #include <asm/ioctls.h>
@@ -220,11 +221,13 @@
 {
 	struct inode *inode = filp->f_dentry->d_inode;
 	struct pipe_inode_info *info;
+	struct pipe_buffer *buf;
 	ssize_t ret;
 	int do_wakeup;
 	struct iovec *iov = (struct iovec *)_iov;
 	size_t total_len;
 	ssize_t chars;
+	size_t last_allowed_len;
 
 	total_len = iov_length(iov, nr_segs);
 	/* Null write succeeds. */
@@ -242,16 +245,28 @@
 		goto out;
 	}
 
+	buf = info->bufs + info->curbuf;
+	last_allowed_len = buf->offset & ~(PIPE_BUF-1);
+
 	/* We try to merge small writes */
 	chars = total_len & (PAGE_SIZE-1); /* size of the last buffer */
 	if (info->nrbufs && chars != 0) {
 		int lastbuf = (info->curbuf + info->nrbufs - 1) & (PIPE_BUFFERS-1);
-		struct pipe_buffer *buf = info->bufs + lastbuf;
-		struct pipe_buf_operations *ops = buf->ops;
-		int offset = buf->offset + buf->len;
-		if (ops->can_merge && offset + chars <= PAGE_SIZE) {
+		struct pipe_buf_operations *ops;
+		int offset;
+		size_t max_write = PAGE_SIZE;
+
+		if (lastbuf == info->curbuf - 1 || lastbuf == (PIPE_BUFFERS-1))
+			max_write = last_allowed_len;
+		buf = info->bufs + lastbuf;
+		ops = buf->ops;
+		offset = buf->offset + buf->len;
+		if (ops->can_merge && offset < max_write) {
 			void *addr = ops->map(filp, info, buf);
-			int error = pipe_iov_copy_from_user(offset + addr, iov, chars);
+			int error;
+			if (chars > (max_write - offset))
+				chars = max_write - offset;
+			error = pipe_iov_copy_from_user(offset + addr, iov, chars);
 			ops->unmap(info, buf);
 			ret = error;
 			do_wakeup = 1;
@@ -275,10 +290,13 @@
 		bufs = info->nrbufs;
 		if (bufs < PIPE_BUFFERS) {
 			int newbuf = (info->curbuf + bufs) & (PIPE_BUFFERS-1);
-			struct pipe_buffer *buf = info->bufs + newbuf;
 			struct page *page = info->tmp_page;
 			int error;
+			size_t max_write = PAGE_SIZE;
 
+			if (newbuf == info->curbuf - 1 || newbuf == (PIPE_BUFFERS-1))
+				max_write = last_allowed_len;
+			buf = info->bufs + newbuf;
 			if (!page) {
 				page = alloc_page(GFP_HIGHUSER);
 				if (unlikely(!page)) {
@@ -293,7 +311,7 @@
 			 * FIXME! Is this really true?
 			 */
 			do_wakeup = 1;
-			chars = PAGE_SIZE;
+			chars = max_write;
 			if (chars > total_len)
 				chars = total_len;
 

Re: [PATCH] 2.6.13: POSIX violation in pipes on ia64 for kernels > 2.6.10

[Linus Torvalds]

On Thu, 13 Oct 2005, Jeff Licquia wrote:
>
> On architectures where PAGE_SIZE > PIPE_BUF, a read of PIPE_BUF bytes
> from a full pipe does not change the pipe's write state.  This behavior
> is different from kernels 2.6.9 and earlier, and it triggers failures of
> the LSB tests.  

Sounds like the tests are broken.

It also sounds like your patch is broken: allowing partial short writes is 
in explicit violation of the POSIX specs, and breaks the only thing that 
PIPE_BUF _really_ guarantees, namely that writes smaller than that size 
must be atomic.

The _only_ guarantees wrt PIPE_BUF is literally that a write smaller than 
or equal to the PIPE_BUF will always either complete fully or not at all, 
and that a reader will see the write as an atomic packet (ie two writers 
will never have their write buffers interleaved within such a single 
"write()" system call).

How empty the pipe has to be for a write to be able to do so is outside 
the spec, and any code (including LSB tests) that depends on it is broken.

			Linus

Re: [PATCH] 2.6.13: POSIX violation in pipes on ia64 for kernels > 2.6.10

[Jeff Licquia]

On Thu, 2005-10-13 at 12:54 -0700, Linus Torvalds wrote:
> It also sounds like your patch is broken: allowing partial short writes is 
> in explicit violation of the POSIX specs, and breaks the only thing that 
> PIPE_BUF _really_ guarantees, namely that writes smaller than that size 
> must be atomic.

That guarantee is still there, because short writes take place on the
last buffer in use.  We either have a full PAGE_SIZE or more bytes in
the next available buffer(s), or we're on the last buffer, and the
max_write code kicks in and we control writes in PIPE_BUF increments.

I wrote a short test program to make sure writes < PIPE_BUF length are
still atomic (by reading PIPE_BUF/2 bytes from a full pipe and then
trying to write PIPE_BUF * (3/4) bytes to it), and the behavior seems to
be correct (ret = -1, errno = EAGAIN).

> The _only_ guarantees wrt PIPE_BUF is literally that a write smaller than 
> or equal to the PIPE_BUF will always either complete fully or not at all, 
> and that a reader will see the write as an atomic packet (ie two writers 
> will never have their write buffers interleaved within such a single 
> "write()" system call).
> 
> How empty the pipe has to be for a write to be able to do so is outside 
> the spec, and any code (including LSB tests) that depends on it is broken.

Hmm.  My reading was different, but on reflection you seem to be more
accurate.  I suppose I will have to bring this to the attention to the
LSB.

I will give one more reason to consider the patch.  Whatever the spec
said, the kernel's behavior has changed, and only for certain
architectures.  On ia64 (at least), the amount one must read from a pipe
in order to unblock it cannot be determined except via extreme means
(parsing a kernel config, doing test pipe I/O to try and deduce
PAGE_SIZE, etc.)  There may be benefit in restoring the previous
behavior, which my patch does without sacrificing the benefits of the
new pipe code.

Of course, that's up to you to decide.  Thanks for your time.