[PATCH] Fix restoring pipes with full buffers

[PATCH] Fix restoring pipes with full buffers

[Dan Smith]

While this fixes restoring pipes that were completely full, it actually
corrects a potential issue with restoring any pipe buffers.  By using
splice() to do this work when we are reading the image from another pipe,
we depend on userspace setting up the buffers in the pipe perfectly
such that the data to be restored is oriented in the pipe in the same
way as it is expected (or required) to be in the restored pipe.  The
"full" case is the hardest to get right, but userspace could break things
if it loaded up the inbound pipe with lots of small buffers which would
cause splice() to hit the PIPE_BUFFERS limit before having read the
requested amount of data.

Instead, drop the optimization and just read() and write() data into
the pipe.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/pipe.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 9664e4f..0da1e3a 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -926,17 +926,60 @@ static int pipe_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
 	return ret;
 }
 
+static int restore_pipe_buffer(struct ckpt_ctx *ctx,
+			       struct file *dest,
+			       int len)
+{
+	char *buf;
+	int ret;
+	int nread;
+	int nwrote;
+	int nleft = len;
+
+	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	for (nleft = len; nleft > 0; nleft -= nread) {
+		int size = nleft < PAGE_SIZE ? nleft : PAGE_SIZE;
+		loff_t pos;
+
+		pos = file_pos_read(ctx->file);
+		nread = kernel_read(ctx->file, pos, buf, size);
+		if (nread < 0) {
+			ret = nread;
+			goto out;
+		}
+		file_pos_write(ctx->file, pos + nread);
+
+		pos = file_pos_read(dest);
+		nwrote = kernel_write(dest, pos, buf, nread);
+		if (nwrote < 0) {
+			ret = nwrote;
+			goto out;
+		}
+		file_pos_write(dest, pos + nwrote);
+
+		if (nwrote != nread) {
+			ret = -EPIPE;
+			goto out;
+		}
+	}
+	ret = len;
+ out:
+	kfree(buf);
+	return ret;
+}
+
 static int restore_pipe(struct ckpt_ctx *ctx, struct file *file)
 {
-	struct pipe_inode_info *pipe;
 	int len, ret;
 
 	len = _ckpt_read_obj_type(ctx, NULL, 0, CKPT_HDR_PIPE_BUF);
 	if (len <= 0)
 		return len;
 
-	pipe = file->f_dentry->d_inode->i_pipe;
-	ret = do_splice_to(ctx->file, &ctx->file->f_pos, pipe, len, 0);
+	ret = restore_pipe_buffer(ctx, file, len);
 
 	if (ret >= 0 && ret != len)
 		ret = -EPIPE;  /* can occur due to an error in source file */
-- 
1.7.2.2

Re: [PATCH] Fix restoring pipes with full buffers

[Oren Laadan]

Dan,

Thanks for pointing this out and the patch.

I think it would be simpler to use ckpt_kread(), no ?
If so, I'll go ahead and fix then import.

Oren.

On 01/28/2011 01:45 PM, Dan Smith wrote:
> While this fixes restoring pipes that were completely full, it actually
> corrects a potential issue with restoring any pipe buffers.  By using
> splice() to do this work when we are reading the image from another pipe,
> we depend on userspace setting up the buffers in the pipe perfectly
> such that the data to be restored is oriented in the pipe in the same
> way as it is expected (or required) to be in the restored pipe.  The
> "full" case is the hardest to get right, but userspace could break things
> if it loaded up the inbound pipe with lots of small buffers which would
> cause splice() to hit the PIPE_BUFFERS limit before having read the
> requested amount of data.
> 
> Instead, drop the optimization and just read() and write() data into
> the pipe.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/pipe.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 9664e4f..0da1e3a 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -926,17 +926,60 @@ static int pipe_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
>  	return ret;
>  }
>  
> +static int restore_pipe_buffer(struct ckpt_ctx *ctx,
> +			       struct file *dest,
> +			       int len)
> +{
> +	char *buf;
> +	int ret;
> +	int nread;
> +	int nwrote;
> +	int nleft = len;
> +
> +	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	for (nleft = len; nleft > 0; nleft -= nread) {
> +		int size = nleft < PAGE_SIZE ? nleft : PAGE_SIZE;
> +		loff_t pos;
> +
> +		pos = file_pos_read(ctx->file);
> +		nread = kernel_read(ctx->file, pos, buf, size);
> +		if (nread < 0) {
> +			ret = nread;
> +			goto out;
> +		}
> +		file_pos_write(ctx->file, pos + nread);
> +
> +		pos = file_pos_read(dest);
> +		nwrote = kernel_write(dest, pos, buf, nread);
> +		if (nwrote < 0) {
> +			ret = nwrote;
> +			goto out;
> +		}
> +		file_pos_write(dest, pos + nwrote);
> +
> +		if (nwrote != nread) {
> +			ret = -EPIPE;
> +			goto out;
> +		}
> +	}
> +	ret = len;
> + out:
> +	kfree(buf);
> +	return ret;
> +}
> +
>  static int restore_pipe(struct ckpt_ctx *ctx, struct file *file)
>  {
> -	struct pipe_inode_info *pipe;
>  	int len, ret;
>  
>  	len = _ckpt_read_obj_type(ctx, NULL, 0, CKPT_HDR_PIPE_BUF);
>  	if (len <= 0)
>  		return len;
>  
> -	pipe = file->f_dentry->d_inode->i_pipe;
> -	ret = do_splice_to(ctx->file, &ctx->file->f_pos, pipe, len, 0);
> +	ret = restore_pipe_buffer(ctx, file, len);
>  
>  	if (ret >= 0 && ret != len)
>  		ret = -EPIPE;  /* can occur due to an error in source file */

Re: [PATCH] Fix restoring pipes with full buffers

[Dan Smith]

OL> I think it would be simpler to use ckpt_kread(), no ?  If so, I'll
OL> go ahead and fix then import.

Yeah, I can't remember why I didn't do that.  I swear I had a reason,
but I must be thinking of something else.

Thanks!

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH] Fix restoring pipes with full buffers

[Matt Helsley]

On Fri, Jan 28, 2011 at 10:45:11AM -0800, Dan Smith wrote:
> While this fixes restoring pipes that were completely full, it actually
> corrects a potential issue with restoring any pipe buffers.  By using
> splice() to do this work when we are reading the image from another pipe,
> we depend on userspace setting up the buffers in the pipe perfectly
> such that the data to be restored is oriented in the pipe in the same
> way as it is expected (or required) to be in the restored pipe.  The
> "full" case is the hardest to get right, but userspace could break things
> if it loaded up the inbound pipe with lots of small buffers which would
> cause splice() to hit the PIPE_BUFFERS limit before having read the
> requested amount of data.
> 
> Instead, drop the optimization and just read() and write() data into
> the pipe.

Looks good:

Reviewed-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>


While it seems like it should be possible to modify splice internals to
handle this Oren's checkpoint/restart tree doesn't seem like the right
place to attempt that work.

> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/pipe.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 9664e4f..0da1e3a 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -926,17 +926,60 @@ static int pipe_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
>  	return ret;
>  }
> 
> +static int restore_pipe_buffer(struct ckpt_ctx *ctx,
> +			       struct file *dest,
> +			       int len)
> +{
> +	char *buf;
> +	int ret;
> +	int nread;
> +	int nwrote;
> +	int nleft = len;
> +
> +	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	for (nleft = len; nleft > 0; nleft -= nread) {
> +		int size = nleft < PAGE_SIZE ? nleft : PAGE_SIZE;
> +		loff_t pos;
> +
> +		pos = file_pos_read(ctx->file);
> +		nread = kernel_read(ctx->file, pos, buf, size);
> +		if (nread < 0) {
> +			ret = nread;
> +			goto out;
> +		}
> +		file_pos_write(ctx->file, pos + nread);
> +
> +		pos = file_pos_read(dest);
> +		nwrote = kernel_write(dest, pos, buf, nread);
> +		if (nwrote < 0) {
> +			ret = nwrote;
> +			goto out;
> +		}
> +		file_pos_write(dest, pos + nwrote);
> +
> +		if (nwrote != nread) {
> +			ret = -EPIPE;
> +			goto out;
> +		}
> +	}
> +	ret = len;
> + out:
> +	kfree(buf);
> +	return ret;
> +}

Could you make this more generic and just take a source struct file *
as a parameter too (instead of the struct ckpt_ctx *)? Perhaps rename it
copy_pipe_to_pipe(src_file, dest_file, len) or some such? Seems like a
helper like this should already exist somewhere...

Cheers,
	-Matt Helsley

Re: [PATCH] Fix restoring pipes with full buffers

[Oren Laadan]

On 01/28/2011 06:08 PM, Matt Helsley wrote:
> On Fri, Jan 28, 2011 at 10:45:11AM -0800, Dan Smith wrote:
>> While this fixes restoring pipes that were completely full, it actually
>> corrects a potential issue with restoring any pipe buffers.  By using
>> splice() to do this work when we are reading the image from another pipe,
>> we depend on userspace setting up the buffers in the pipe perfectly
>> such that the data to be restored is oriented in the pipe in the same
>> way as it is expected (or required) to be in the restored pipe.  The
>> "full" case is the hardest to get right, but userspace could break things
>> if it loaded up the inbound pipe with lots of small buffers which would
>> cause splice() to hit the PIPE_BUFFERS limit before having read the
>> requested amount of data.
>>
>> Instead, drop the optimization and just read() and write() data into
>> the pipe.
> 
> Looks good:
> 
> Reviewed-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> 
> While it seems like it should be possible to modify splice internals to
> handle this Oren's checkpoint/restart tree doesn't seem like the right
> place to attempt that work.
> 
>>
>> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/pipe.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/pipe.c b/fs/pipe.c
>> index 9664e4f..0da1e3a 100644
>> --- a/fs/pipe.c
>> +++ b/fs/pipe.c
>> @@ -926,17 +926,60 @@ static int pipe_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
>>  	return ret;
>>  }
>>
>> +static int restore_pipe_buffer(struct ckpt_ctx *ctx,
>> +			       struct file *dest,
>> +			       int len)
>> +{
>> +	char *buf;
>> +	int ret;
>> +	int nread;
>> +	int nwrote;
>> +	int nleft = len;
>> +
>> +	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	for (nleft = len; nleft > 0; nleft -= nread) {
>> +		int size = nleft < PAGE_SIZE ? nleft : PAGE_SIZE;
>> +		loff_t pos;
>> +
>> +		pos = file_pos_read(ctx->file);
>> +		nread = kernel_read(ctx->file, pos, buf, size);
>> +		if (nread < 0) {
>> +			ret = nread;
>> +			goto out;
>> +		}
>> +		file_pos_write(ctx->file, pos + nread);
>> +
>> +		pos = file_pos_read(dest);
>> +		nwrote = kernel_write(dest, pos, buf, nread);

Actually, this is wrong: it works if checkpoint first found the
write-end and then the read-end; but the opposite case fails 
because the code will try to write to the read-end file. (It did
work before because we used the pipe directly).

>> +		if (nwrote < 0) {
>> +			ret = nwrote;
>> +			goto out;
>> +		}
>> +		file_pos_write(dest, pos + nwrote);
>> +
>> +		if (nwrote != nread) {
>> +			ret = -EPIPE;
>> +			goto out;
>> +		}
>> +	}
>> +	ret = len;
>> + out:
>> +	kfree(buf);
>> +	return ret;
>> +}
> 
> Could you make this more generic and just take a source struct file *
> as a parameter too (instead of the struct ckpt_ctx *)? Perhaps rename it
> copy_pipe_to_pipe(src_file, dest_file, len) or some such? Seems like a
> helper like this should already exist somewhere...

Yes - like ckpt_write, and _ckpt_write()...

Oren.

Re: [PATCH] Fix restoring pipes with full buffers

[Oren Laadan]

From 7d3f62995ed4e83bb43deafa362c78624099e495 Mon Sep 17 00:00:00 2001
From: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
Date: Fri, 28 Jan 2011 19:23:17 -0500
Subject: [PATCH] c/r: fix restoring pipes with full buffers

Dan Smith pointed out a problem with the use of splice to restore a
pipe's contents: since a pipe can have at most PIPE_BUFFERs buffers,
if it happens that the input fed through the userspace feeder is split
into more pieces, the splice will eventually block. This bad behavior
is more likely to occur if the pipe buffer had been nearly full when
checkpointed.

Dan's patch fixes the problem by dropping the optimization (we don't
expect pipe buffers to be a dominant component of the image), but is
inocorrect for cases when the read-end of the pipe was saved first
during checkpoint. This version fixes that problem and makes use of
c/r's ckpt_kread() helpers.

Cc: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
---
 fs/pipe.c                  |   49 +++++++++++++++++++++++++++----------------
 include/linux/checkpoint.h |    2 +
 kernel/checkpoint/sys.c    |    4 +-
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 9ca956d..0d049a3 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -928,20 +928,27 @@ static int pipe_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
 
 static int restore_pipe(struct ckpt_ctx *ctx, struct file *file)
 {
-	struct pipe_inode_info *pipe;
-	int len, ret;
+	int n, len, ret;
 
 	len = _ckpt_read_obj_type(ctx, NULL, 0, CKPT_HDR_PIPE_BUF);
 	if (len < 0)
 		return len;
 
-	pipe = file->f_dentry->d_inode->i_pipe;
-	ret = do_splice_to(ctx->file, &ctx->file->f_pos, pipe, len, 0);
-
-	if (ret >= 0 && ret != len)
-		ret = -EPIPE;  /* can occur due to an error in source file */
+	while (len) {
+		n = min(len, (int) PAGE_SIZE);
+		ret = ckpt_kread(ctx, ctx->scratch_page, n);
+		if (ret < 0)
+			return ret;
+		ret = _ckpt_kwrite(file, ctx->scratch_page, n);
+		if (ret < 0)
+			return ret;
+		/* this should not happen normally ! */
+		if (ret != n)
+			return -EPIPE;
+		len -= n;
+	}
 
-	return ret;
+	return 0;
 }
 
 struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
@@ -971,6 +978,14 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 		if (ret < 0)
 			return file;
 
+		/* use the "write" side to restore the pipe's contents */
+		file = fget(fds[1]);
+		if (!file)	/* this should _never_ happen ! */
+			return ERR_PTR(-EBADF);
+		ret = restore_pipe(ctx, file);
+		if (ret < 0)
+			goto out;
+
 		which = (ptr->f_flags & O_WRONLY ? 1 : 0);
 		/*
 		 * Below we return the file corersponding to one side
@@ -978,25 +993,23 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 		 * other side of the pipe to the hash, to be picked up
 		 * when that side is restored.
 		 */
-		file = fget(fds[1-which]);	/* the 'other' side */
-		if (!file)	/* this should _never_ happen ! */
-			return ERR_PTR(-EBADF);
+		if (which == 1) {	/* the 'other' size */
+			fput(file);
+			file = fget(fds[0]);
+			if (!file)	/* this should _never_ happen ! */
+				return ERR_PTR(-EBADF);
+		}
 		ret = ckpt_obj_insert(ctx, file, h->pipe_objref, CKPT_OBJ_FILE);
 		if (ret < 0)
 			goto out;
 
-		ret = restore_pipe(ctx, file);
-		fput(file);
-		if (ret < 0)
-			return ERR_PTR(ret);
-
 		file = fget(fds[which]);	/* 'this' side */
 		if (!file)	/* this should _never_ happen ! */
 			return ERR_PTR(-EBADF);
 
 		/* get rid of the file descriptors (caller sets that) */
-		sys_close(fds[which]);
-		sys_close(fds[1-which]);
+		sys_close(fds[0]);
+		sys_close(fds[1]);
 	} else {
 		return file;
 	}
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index c015106..adaf6af 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -76,6 +76,8 @@ extern int walk_task_subtree(struct task_struct *task,
 			     void *data);
 extern void exit_checkpoint(struct task_struct *tsk);
 
+extern ssize_t _ckpt_kwrite(struct file *file, void *buf, size_t count);
+extern ssize_t _ckpt_kread(struct file *file, void *buf, size_t count);
 extern int ckpt_kwrite(struct ckpt_ctx *ctx, void *buf, size_t count);
 extern int ckpt_kread(struct ckpt_ctx *ctx, void *buf, size_t count);
 
diff --git a/kernel/checkpoint/sys.c b/kernel/checkpoint/sys.c
index e9a377b..2383db9 100644
--- a/kernel/checkpoint/sys.c
+++ b/kernel/checkpoint/sys.c
@@ -51,7 +51,7 @@ int ckpt_unpriv_allowed = 1;	/* default: unpriv checkpoint not restart */
  * and return 0, or negative error otherwise.
  */
 
-static ssize_t _ckpt_kwrite(struct file *file, void *addr, size_t count)
+ssize_t _ckpt_kwrite(struct file *file, void *addr, size_t count)
 {
 	loff_t pos;
 	int ret;
@@ -82,7 +82,7 @@ int ckpt_kwrite(struct ckpt_ctx *ctx, void *addr, size_t count)
 	return 0;
 }
 
-static ssize_t _ckpt_kread(struct file *file, void *addr, size_t count)
+ssize_t _ckpt_kread(struct file *file, void *addr, size_t count)
 {
 	loff_t pos;
 	int ret;
-- 
1.7.1

Re: [PATCH] Fix restoring pipes with full buffers

[Dan Smith]

OL> Cc: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
OL> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>

Tested-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH] Fix restoring pipes with full buffers

[Oren Laadan]

thanks, pushed to v23-rc1.

On 01/31/2011 11:44 AM, Dan Smith wrote:
> OL> Cc: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> OL> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
> 
> Tested-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>