LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] rds: Make rds_message_copy_from_user() return 0 on success.
@ 2015-02-05 20:20 Sowmini Varadhan
  2015-02-05 21:58 ` Al Viro
  0 siblings, 1 reply; 6+ messages in thread
From: Sowmini Varadhan @ 2015-02-05 20:20 UTC (permalink / raw)
  To: chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel, viro


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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rds: Make rds_message_copy_from_user() return 0 on success.
  2015-02-05 20:20 [PATCH] rds: Make rds_message_copy_from_user() return 0 on success Sowmini Varadhan
@ 2015-02-05 21:58 ` Al Viro
  2015-02-05 22:07   ` Sowmini Varadhan
  0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2015-02-05 21:58 UTC (permalink / raw)
  To: Sowmini Varadhan; +Cc: chien.yen, davem, rds-devel, netdev, linux-kernel

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rds: Make rds_message_copy_from_user() return 0 on success.
  2015-02-05 21:58 ` Al Viro
@ 2015-02-05 22:07   ` Sowmini Varadhan
  2015-02-05 22:23     ` Al Viro
  0 siblings, 1 reply; 6+ messages in thread
From: Sowmini Varadhan @ 2015-02-05 22:07 UTC (permalink / raw)
  To: Al Viro; +Cc: chien.yen, davem, rds-devel, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rds: Make rds_message_copy_from_user() return 0 on success.
  2015-02-05 22:07   ` Sowmini Varadhan
@ 2015-02-05 22:23     ` Al Viro
  2015-02-05 22:41       ` [PATCHv2] " Sowmini Varadhan
  0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2015-02-05 22:23 UTC (permalink / raw)
  To: Sowmini Varadhan; +Cc: chien.yen, davem, rds-devel, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCHv2] rds: Make rds_message_copy_from_user() return 0 on success.
  2015-02-05 22:23     ` Al Viro
@ 2015-02-05 22:41       ` Sowmini Varadhan
  2015-02-08  6:42         ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: Sowmini Varadhan @ 2015-02-05 22:41 UTC (permalink / raw)
  To: chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel, sowmini.varadhan, viro



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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv2] rds: Make rds_message_copy_from_user() return 0 on success.
  2015-02-05 22:41       ` [PATCHv2] " Sowmini Varadhan
@ 2015-02-08  6:42         ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2015-02-08  6:42 UTC (permalink / raw)
  To: sowmini.varadhan; +Cc: chien.yen, rds-devel, netdev, linux-kernel, viro

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-02-08  6:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-05 20:20 [PATCH] rds: Make rds_message_copy_from_user() return 0 on success Sowmini Varadhan
2015-02-05 21:58 ` Al Viro
2015-02-05 22:07   ` Sowmini Varadhan
2015-02-05 22:23     ` Al Viro
2015-02-05 22:41       ` [PATCHv2] " Sowmini Varadhan
2015-02-08  6:42         ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).