When receiving pages data, return value 'ret' when positive includes
`buf->page_base`, so we should subtract it similarly to how it is done
for `offset`.
This was discovered on the very rare cases where the server returned a
chunk of bytes that when added to the already received amount of bytes
for the pages happened to match the current `recv.len`, for example
on this case:
buf->page_base : 258356
actually received from socket: 1740
ret : 260096
want : 260096
In this case neither of the two 'if ... goto out' trigger, and we
continue to tail parsing.
Worth to mention that the ensuing EMSGSIZE from the continued execution of
`xs_read_xdr_buf` may be observed by an application due to 4 superfluous
bytes being added to the pages data.
Fixes: 277e4ab7d53 ("SUNRPC: Simplify TCP receive code by switching to
using iterators")
Signed-off-by: Dan Aloni <[email protected]>
---
net/sunrpc/xprtsock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7090bbee0ec5..42b680a60d38 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -436,7 +436,7 @@ xs_read_xdr_buf(struct socket *sock, struct msghdr *msg, int flags,
offset += ret - buf->page_base;
if (offset == count || msg->msg_flags & (MSG_EOR|MSG_TRUNC))
goto out;
- if (ret != want)
+ if (ret - buf->page_base != want)
goto out;
seek = 0;
} else {
--
2.26.2
On Fri, 2020-12-04 at 20:34 +0200, Dan Aloni wrote:
> When receiving pages data, return value 'ret' when positive includes
> `buf->page_base`, so we should subtract it similarly to how it is
> done
> for `offset`.
>
> This was discovered on the very rare cases where the server returned
> a
> chunk of bytes that when added to the already received amount of
> bytes
> for the pages happened to match the current `recv.len`, for example
> on this case:
>
> buf->page_base : 258356
> actually received from socket: 1740
> ret : 260096
> want : 260096
>
> In this case neither of the two 'if ... goto out' trigger, and we
> continue to tail parsing.
>
> Worth to mention that the ensuing EMSGSIZE from the continued
> execution of
> `xs_read_xdr_buf` may be observed by an application due to 4
> superfluous
> bytes being added to the pages data.
>
> Fixes: 277e4ab7d53 ("SUNRPC: Simplify TCP receive code by switching
> to
> using iterators")
> Signed-off-by: Dan Aloni <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 7090bbee0ec5..42b680a60d38 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -436,7 +436,7 @@ xs_read_xdr_buf(struct socket *sock, struct
> msghdr *msg, int flags,
> offset += ret - buf->page_base;
> if (offset == count || msg->msg_flags &
> (MSG_EOR|MSG_TRUNC))
> goto out;
> - if (ret != want)
> + if (ret - buf->page_base != want)
> goto out;
> seek = 0;
> } else {
Ouch... Well spotted!
Hmm... I think we want to just subtract out the buf->page_base from the
value of 'ret' after we call xs_flush_bvec() and then adjust the
calculation of 'offset' in the next line. That's more efficient.
Can you please resend with that change?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Fri, Dec 04, 2020 at 08:52:55PM +0000, Trond Myklebust wrote:
> On Fri, 2020-12-04 at 20:34 +0200, Dan Aloni wrote:
[..]
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -436,7 +436,7 @@ xs_read_xdr_buf(struct socket *sock, struct
> > msghdr *msg, int flags,
> > ????????????????offset += ret - buf->page_base;
> > ????????????????if (offset == count || msg->msg_flags &
> > (MSG_EOR|MSG_TRUNC))
> > ????????????????????????goto out;
> > -???????????????if (ret != want)
> > +???????????????if (ret - buf->page_base != want)
> > ????????????????????????goto out;
> > ????????????????seek = 0;
> > ????????} else {
>
> Ouch... Well spotted!
>
> Hmm... I think we want to just subtract out the buf->page_base from the
> value of 'ret' after we call xs_flush_bvec() and then adjust the
> calculation of 'offset' in the next line. That's more efficient.
Yes, it works out, though after being aware that the positive value of
`ret` when returned from `xs_read_xdr_buf` and propagated upward has no
effect except on traces.
--
Dan Aloni
When receiving pages data, return value 'ret' when positive includes
`buf->page_base`, so we should subtract that before it is used for
changing `offset` and comparing against `want`.
This was discovered on the very rare cases where the server returned a
chunk of bytes that when added to the already received amount of bytes
for the pages happened to match the current `recv.len`, for example
on this case:
buf->page_base : 258356
actually received from socket: 1740
ret : 260096
want : 260096
In this case neither of the two 'if ... goto out' trigger, and we
continue to tail parsing.
Worth to mention that the ensuing EMSGSIZE from the continued execution of
`xs_read_xdr_buf` may be observed by an application due to 4 superfluous
bytes being added to the pages data.
Fixes: 277e4ab7d53 ("SUNRPC: Simplify TCP receive code by switching to
using iterators")
Signed-off-by: Dan Aloni <[email protected]>
---
net/sunrpc/xprtsock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7090bbee0ec5..9a4300f07e4c 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -433,7 +433,8 @@ xs_read_xdr_buf(struct socket *sock, struct msghdr *msg, int flags,
if (ret <= 0)
goto sock_err;
xs_flush_bvec(buf->bvec, ret, seek + buf->page_base);
- offset += ret - buf->page_base;
+ ret -= buf->page_base;
+ offset += ret;
if (offset == count || msg->msg_flags & (MSG_EOR|MSG_TRUNC))
goto out;
if (ret != want)
--
2.26.2