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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
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
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
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 <[email protected]>
---
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
From: Sowmini Varadhan <[email protected]>
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 <[email protected]>
> ---
> changes from v1: incorporate Al.Viro comment.
Applied.