[PATCH] rds: Make rds_message_copy_from_user() return 0 on success.

[PATCH] rds: Make rds_message_copy_from_user() return 0 on success.

[Sowmini Varadhan]

Commit c310e72c8992 ("rds: switch ->inc_copy_to_user() to passing iov_iter")
breaks rds_message_copy_from_user() semantics on success, and causes it
to return nbytes copied, when it should return 0.  This commit fixes that bug.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 net/rds/message.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/rds/message.c b/net/rds/message.c
index 5a21e6f..756c737 100644
--- a/net/rds/message.c
+++ b/net/rds/message.c
@@ -266,7 +266,7 @@ struct rds_message *rds_message_map_pages(unsigned long *page_addrs, unsigned in
 
 int rds_message_copy_from_user(struct rds_message *rm, struct iov_iter *from)
 {
-	unsigned long to_copy;
+	unsigned long to_copy, nbytes;
 	unsigned long sg_off;
 	struct scatterlist *sg;
 	int ret = 0;
@@ -293,9 +293,9 @@ int rds_message_copy_from_user(struct rds_message *rm, struct iov_iter *from)
 				sg->length - sg_off);
 
 		rds_stats_add(s_copy_from_user, to_copy);
-		ret = copy_page_from_iter(sg_page(sg), sg->offset + sg_off,
-					  to_copy, from);
-		if (ret != to_copy)
+		nbytes = copy_page_from_iter(sg_page(sg), sg->offset + sg_off,
+					     to_copy, from);
+		if (nbytes != to_copy)
 			return -EFAULT;
 
 		sg_off += to_copy;
-- 
1.7.1

Re: [PATCH] rds: Make rds_message_copy_from_user() return 0 on success.

[Al Viro]

On Thu, Feb 05, 2015 at 03:20:50PM -0500, Sowmini Varadhan wrote:
> 
> Commit c310e72c8992 ("rds: switch ->inc_copy_to_user() to passing iov_iter")
> breaks rds_message_copy_from_user() semantics on success, and causes it
> to return nbytes copied, when it should return 0.  This commit fixes that bug.

*blink*

Commit 083735f4b0 ("rds: switch rds_message_copy_from_user() to iov_iter"),
surely?  That's the one that touches copy-from side of things...

And unlike the previous commit (the one you'd mentioned) here the return value
of copy_page_....() ended up passed to caller of its caller.  Mea culpa.

So, modulo correction of the commit message,

Acked-by: Al Viro <viro@zeniv.linux.org.uk>

Re: [PATCH] rds: Make rds_message_copy_from_user() return 0 on success.

[Sowmini Varadhan]

On (02/05/15 21:58), Al Viro wrote:
> *blink*
> 
> Commit 083735f4b0 ("rds: switch rds_message_copy_from_user() to iov_iter"),
> surely?  That's the one that touches copy-from side of things...
> 
> And unlike the previous commit (the one you'd mentioned) here the return value
> of copy_page_....() ended up passed to caller of its caller.  Mea culpa.
> 
> So, modulo correction of the commit message,
> 
> Acked-by: Al Viro <viro@zeniv.linux.org.uk>

That was the one that git-bisect flagged for me.. I think this one
had the iov_iter_init() in rds_sendmsg() itself? As I recall, this 
change was already refactoring rds_message_copy_from_user() (and was the
earliest version that was broken when I tested it)?

should it have been 'put iov_iter into msghdr'? I just wanted
to make sure some innocent user would know how far back to go,
if they wanted to not trip on this one.

--Sowmini

Re: [PATCH] rds: Make rds_message_copy_from_user() return 0 on success.

[Al Viro]

On Thu, Feb 05, 2015 at 05:07:34PM -0500, Sowmini Varadhan wrote:

> That was the one that git-bisect flagged for me.. I think this one
> had the iov_iter_init() in rds_sendmsg() itself?

No, it doesn't - it only touches recvmsg side of things.

> As I recall, this 
> change was already refactoring rds_message_copy_from_user() (and was the
> earliest version that was broken when I tested it)?
> 
> should it have been 'put iov_iter into msghdr'? I just wanted
> to make sure some innocent user would know how far back to go,
> if they wanted to not trip on this one.

Look at your own patch; it affects only sendmsg() path, as does
"rds: switch rds_message_copy_from_user() to iov_iter".  In fact, the code
it fixes had been introduced in that commit.  I agree that the bug is
there and I agree with your fix; the only problem I have is your interpretation
of bisect.  "rds: switch ->inc_copy_to_user() to passing iov_iter" is
the last one *not* to have that particular breakage.

Seriously, reread both commits and your fix:
"rds: switch ->inc_copy_to_user() to passing iov_iter" - only recvmsg side
"rds: switch rds_message_copy_from_user() to iov_iter" - only sendmsg side
your fix to rds_message_copy_from_user() - only sendmsg side

[PATCHv2] rds: Make rds_message_copy_from_user() return 0 on success.

[Sowmini Varadhan]

Commit 083735f4b01b ("rds: switch rds_message_copy_from_user() to iov_iter")
breaks rds_message_copy_from_user() semantics on success, and causes it
to return nbytes copied, when it should return 0.  This commit fixes that bug.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
changes from v1: incorporate Al.Viro comment. 

 net/rds/message.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/rds/message.c b/net/rds/message.c
index 5a21e6f..756c737 100644
--- a/net/rds/message.c
+++ b/net/rds/message.c
@@ -266,7 +266,7 @@ struct rds_message *rds_message_map_pages(unsigned long *page_addrs, unsigned in
 
 int rds_message_copy_from_user(struct rds_message *rm, struct iov_iter *from)
 {
-	unsigned long to_copy;
+	unsigned long to_copy, nbytes;
 	unsigned long sg_off;
 	struct scatterlist *sg;
 	int ret = 0;
@@ -293,9 +293,9 @@ int rds_message_copy_from_user(struct rds_message *rm, struct iov_iter *from)
 				sg->length - sg_off);
 
 		rds_stats_add(s_copy_from_user, to_copy);
-		ret = copy_page_from_iter(sg_page(sg), sg->offset + sg_off,
-					  to_copy, from);
-		if (ret != to_copy)
+		nbytes = copy_page_from_iter(sg_page(sg), sg->offset + sg_off,
+					     to_copy, from);
+		if (nbytes != to_copy)
 			return -EFAULT;
 
 		sg_off += to_copy;
-- 
1.7.1

Re: [PATCHv2] rds: Make rds_message_copy_from_user() return 0 on success.

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Thu, 5 Feb 2015 17:41:43 -0500

> Commit 083735f4b01b ("rds: switch rds_message_copy_from_user() to iov_iter")
> breaks rds_message_copy_from_user() semantics on success, and causes it
> to return nbytes copied, when it should return 0.  This commit fixes that bug.
> 
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> changes from v1: incorporate Al.Viro comment. 

Applied.