[Qemu-devel] [PATCH] sheepdog: discard the payload if the header is invalid

[Qemu-devel] [PATCH] sheepdog: discard the payload if the header is invalid

[Liu Yuan]

From: Liu Yuan <liuyuan@cmss.chinamobile.com>

We need to discard the payload if we get a invalid header due to whatever reason
to avoid data stream curruption. For e.g., the response consists of header plus
data payload. If we simply read the header then the data payload is left in the
socket buffer and the next time we would read the garbage data and currupt the
whole connection.

Cc: qemu-devel@nongnu.org
Cc: Jeff Cody <jcody@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
---
 block/sheepdog.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 9585beb..9ed3458 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -794,6 +794,14 @@ static void coroutine_fn aio_read_response(void *opaque)
         }
     }
     if (!aio_req) {
+        if (rsp.data_length) {
+            void *garbage = g_malloc(rsp.data_length);
+            ret = qemu_co_recv(fd, garbage, rsp.data_length);
+            if (ret != rsp.data_length) {
+                error_report("failed to discard the data, %s", strerror(errno));
+            }
+            g_free(garbage);
+        }
         error_report("cannot find aio_req %x", rsp.id);
         goto err;
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH] sheepdog: discard the payload if the header is invalid

[Jeff Cody]

On Tue, Sep 01, 2015 at 09:29:31AM +0800, Liu Yuan wrote:
> From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> 
> We need to discard the payload if we get a invalid header due to whatever reason
> to avoid data stream curruption.

If the header is invalid / corrupted, how can rsp.data_length be
trusted?  Out of curiosity, is this an issue you are seeing occur "in
the wild"?

> For e.g., the response consists of header plus
> data payload. If we simply read the header then the data payload is left in the
> socket buffer and the next time we would read the garbage data and currupt the
> whole connection.
> 
> Cc: qemu-devel@nongnu.org
> Cc: Jeff Cody <jcody@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
> ---
>  block/sheepdog.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 9585beb..9ed3458 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -794,6 +794,14 @@ static void coroutine_fn aio_read_response(void *opaque)
>          }
>      }
>      if (!aio_req) {
> +        if (rsp.data_length) {
> +            void *garbage = g_malloc(rsp.data_length);
> +            ret = qemu_co_recv(fd, garbage, rsp.data_length);
> +            if (ret != rsp.data_length) {
> +                error_report("failed to discard the data, %s", strerror(errno));
> +            }
> +            g_free(garbage);
> +        }
>          error_report("cannot find aio_req %x", rsp.id);
>          goto err;
>      }
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH] sheepdog: discard the payload if the header is invalid

[Liu Yuan]

On Mon, Aug 31, 2015 at 09:51:00PM -0400, Jeff Cody wrote:
> On Tue, Sep 01, 2015 at 09:29:31AM +0800, Liu Yuan wrote:
> > From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> > 
> > We need to discard the payload if we get a invalid header due to whatever reason
> > to avoid data stream curruption.
> 
> If the header is invalid / corrupted, how can rsp.data_length be
> trusted?  Out of curiosity, is this an issue you are seeing occur "in
> the wild"?

This is the defensive patch. Header is invalid in the sense that only rsp.id is
invalid due to sheepdog driver bugs, for e.g., the request was misplaced after
being sent or duplicated requests sending to sheep daemon and get the duplicated
responses for the same request.

Actually in the late 2012 we had seen this problem but we didn't find the root
cause how this happened by looking at the code statically and the problem was
gone silently while we restructured the code.

But yesterday some centos6 users reported to me the problem of
'cannot find aio_req' and hang the guest disk. That QEMU's sheepdog driver was
rather old and would bump rsp.id mismatch problem as we did in the late 2012.
By looking at the code again, I found this error handling problem. However,
this patch is not aimed to solve the rsp.id mismatch problem (If it still exist)
but just a defensive one after this problem happens.

Thanks,
Yuan

Re: [Qemu-devel] [PATCH] sheepdog: discard the payload if the header is invalid

[Liu Yuan]

On Tue, Sep 01, 2015 at 10:05:38AM +0800, Liu Yuan wrote:
> On Mon, Aug 31, 2015 at 09:51:00PM -0400, Jeff Cody wrote:
> > On Tue, Sep 01, 2015 at 09:29:31AM +0800, Liu Yuan wrote:
> > > From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> > > 
> > > We need to discard the payload if we get a invalid header due to whatever reason
> > > to avoid data stream curruption.
> > 
> > If the header is invalid / corrupted, how can rsp.data_length be
> > trusted?  Out of curiosity, is this an issue you are seeing occur "in
> > the wild"?

For a second thought, we might not need this patch for the upstream because of
auto-connection feature, which close the socket to bury the whole buffer.

But old QEMU without auto-reconnection, might need this patch to drain the
buffer.

Thanks,
Yuan

> 
> This is the defensive patch. Header is invalid in the sense that only rsp.id is
> invalid due to sheepdog driver bugs, for e.g., the request was misplaced after
> being sent or duplicated requests sending to sheep daemon and get the duplicated
> responses for the same request.
> 
> Actually in the late 2012 we had seen this problem but we didn't find the root
> cause how this happened by looking at the code statically and the problem was
> gone silently while we restructured the code.
> 
> But yesterday some centos6 users reported to me the problem of
> 'cannot find aio_req' and hang the guest disk. That QEMU's sheepdog driver was
> rather old and would bump rsp.id mismatch problem as we did in the late 2012.
> By looking at the code again, I found this error handling problem. However,
> this patch is not aimed to solve the rsp.id mismatch problem (If it still exist)
> but just a defensive one after this problem happens.
> 
> Thanks,
> Yuan