Patchwork [RFC,RESEND,v1,09/15] virtproxy: add handler for data packets

login
register
mail settings
Submitter Michael Roth
Date Nov. 3, 2010, 3:28 p.m.
Message ID <1288798090-7127-10-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/70024/
State New
Headers show

Comments

Michael Roth - Nov. 3, 2010, 3:28 p.m.
Process VPPackets coming in from channel and send them to the
appropriate server/client connections.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
Adam Litke - Nov. 4, 2010, 12:46 a.m.
On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> Process VPPackets coming in from channel and send them to the
> appropriate server/client connections.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 6c3611b..57ab2b0 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -235,6 +235,48 @@ static void vp_channel_accept(void *opaque)
>      vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>  }
> 
> +/* handle data packets
> + *
> + * process VPPackets containing data and send them to the corresponding
> + * FDs
> + */
> +static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
> +{
> +    int fd, ret;
> +
> +    TRACE("called with drv: %p", drv);
> +
> +    if (pkt->type == VP_PKT_CLIENT) {
> +        TRACE("recieved client packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.server_fd;
> +    } else if (pkt->type == VP_PKT_SERVER) {
> +        TRACE("recieved server packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.client_fd;
> +    } else {
> +        TRACE("unknown packet type");
> +        return -1;
> +    }
> +
> +    /* TODO: proxied in non-blocking mode can causes us to spin here
> +     * for slow servers/clients. need to use write()'s and maintain
> +     * a per-conn write queue that we clear out before sending any
> +     * more data to the fd
> +     */

Hmm, so a guest can cause a denial of service in the host qemu process?
Are you working on adding the write queues?

> +    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
> +            pkt->payload.proxied.bytes);
> +    if (ret == -1) {
> +        LOG("error sending data over channel");
> +        return -1;
> +    } else if (ret != pkt->payload.proxied.bytes) {
> +        TRACE("buffer full?");

This can happen?  Does this bring the whole connection down?  The whole
virtproxy instance?
Adam Litke - Nov. 4, 2010, 1:48 a.m.
On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote: 
> Process VPPackets coming in from channel and send them to the
> appropriate server/client connections.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 6c3611b..57ab2b0 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -235,6 +235,48 @@ static void vp_channel_accept(void *opaque)
>      vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>  }
> 
> +/* handle data packets
> + *
> + * process VPPackets containing data and send them to the corresponding
> + * FDs
> + */
> +static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
> +{
> +    int fd, ret;
> +
> +    TRACE("called with drv: %p", drv);
> +
> +    if (pkt->type == VP_PKT_CLIENT) {
> +        TRACE("recieved client packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.server_fd;
> +    } else if (pkt->type == VP_PKT_SERVER) {
> +        TRACE("recieved server packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.client_fd;
> +    } else {
> +        TRACE("unknown packet type");
> +        return -1;
> +    }
> +
> +    /* TODO: proxied in non-blocking mode can causes us to spin here
> +     * for slow servers/clients. need to use write()'s and maintain
> +     * a per-conn write queue that we clear out before sending any
> +     * more data to the fd
> +     */

Hmm, so a guest can cause a denial of service in the host qemu process?
Are you working on adding the write queues?

> +    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
> +            pkt->payload.proxied.bytes);
> +    if (ret == -1) {
> +        LOG("error sending data over channel");
> +        return -1;
> +    } else if (ret != pkt->payload.proxied.bytes) {
> +        TRACE("buffer full?");

This can happen?  

> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  /* read handler for communication channel
>   *
>   * de-multiplexes data coming in over the channel. for control messages
Michael Roth - Nov. 4, 2010, 6:23 p.m.
On 11/03/2010 07:46 PM, Adam Litke wrote:
> On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
>> Process VPPackets coming in from channel and send them to the
>> appropriate server/client connections.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 42 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtproxy.c b/virtproxy.c
>> index 6c3611b..57ab2b0 100644
>> --- a/virtproxy.c
>> +++ b/virtproxy.c
>> @@ -235,6 +235,48 @@ static void vp_channel_accept(void *opaque)
>>       vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>>   }
>>
>> +/* handle data packets
>> + *
>> + * process VPPackets containing data and send them to the corresponding
>> + * FDs
>> + */
>> +static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
>> +{
>> +    int fd, ret;
>> +
>> +    TRACE("called with drv: %p", drv);
>> +
>> +    if (pkt->type == VP_PKT_CLIENT) {
>> +        TRACE("recieved client packet, client fd: %d, server fd: %d",
>> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
>> +        fd = pkt->payload.proxied.server_fd;
>> +    } else if (pkt->type == VP_PKT_SERVER) {
>> +        TRACE("recieved server packet, client fd: %d, server fd: %d",
>> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
>> +        fd = pkt->payload.proxied.client_fd;
>> +    } else {
>> +        TRACE("unknown packet type");
>> +        return -1;
>> +    }
>> +
>> +    /* TODO: proxied in non-blocking mode can causes us to spin here
>> +     * for slow servers/clients. need to use write()'s and maintain
>> +     * a per-conn write queue that we clear out before sending any
>> +     * more data to the fd
>> +     */
>
> Hmm, so a guest can cause a denial of service in the host qemu process?
> Are you working on adding the write queues?
>

Not a guest...though they could DoS the agent in this manner. But there 
seems to be an analogous situation in qemu currently, where a malicious 
client connects to, say, a socket setup to listen for telnet connections 
to the qemu monitor, sends a bunch of info commands, and doesn't read 
anything coming back to it. That might eventually cause the qemu process 
to hang when a monitor_flush->qemu_chr_write->send_all(client_fd) 
happens. A malicious client connecting to a forwarding port/socket on 
the host side could cause a similiar situation.

So if it's a problem, it seems like a problem that should be addressed 
more generally. A general, asynchronous implementation of send_all for 
instance.

>> +    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
>> +            pkt->payload.proxied.bytes);
>> +    if (ret == -1) {
>> +        LOG("error sending data over channel");
>> +        return -1;
>> +    } else if (ret != pkt->payload.proxied.bytes) {
>> +        TRACE("buffer full?");
>
> This can happen?  Does this bring the whole connection down?  The whole
> virtproxy instance?
>

It can if write() returns 0, so only if we do a non-blocking write() 
inside vp_send_all/send_all. It's basically a TODO...since right now the 
writes are blocking ones, so it shouldn't happen. If we used 
non-blocking writes though we'd need to call vp_send_all(fd, (void 
*)pkt->payload.proxied.data + ret, pkt->payload.proxied.bytes - ret) in 
a loop, it wouldn't be an error case. I'll clean this up a bit.

Patch

diff --git a/virtproxy.c b/virtproxy.c
index 6c3611b..57ab2b0 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -235,6 +235,48 @@  static void vp_channel_accept(void *opaque)
     vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
 }
 
+/* handle data packets
+ *
+ * process VPPackets containing data and send them to the corresponding
+ * FDs
+ */
+static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
+{
+    int fd, ret;
+
+    TRACE("called with drv: %p", drv);
+
+    if (pkt->type == VP_PKT_CLIENT) {
+        TRACE("recieved client packet, client fd: %d, server fd: %d",
+              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
+        fd = pkt->payload.proxied.server_fd;
+    } else if (pkt->type == VP_PKT_SERVER) {
+        TRACE("recieved server packet, client fd: %d, server fd: %d",
+              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
+        fd = pkt->payload.proxied.client_fd;
+    } else {
+        TRACE("unknown packet type");
+        return -1;
+    }
+
+    /* TODO: proxied in non-blocking mode can causes us to spin here
+     * for slow servers/clients. need to use write()'s and maintain
+     * a per-conn write queue that we clear out before sending any
+     * more data to the fd
+     */
+    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
+            pkt->payload.proxied.bytes);
+    if (ret == -1) {
+        LOG("error sending data over channel");
+        return -1;
+    } else if (ret != pkt->payload.proxied.bytes) {
+        TRACE("buffer full?");
+        return -1;
+    }
+
+    return 0;
+}
+
 /* read handler for communication channel
  *
  * de-multiplexes data coming in over the channel. for control messages