Re: [Qemu-devel] [PATCH V3 2/3] net/filter-mirror:Add filter-redirector func

[Jason Wang]

On 03/04/2016 08:01 PM, Zhang Chen wrote:
> Filter-redirector is a netfilter plugin.
> It gives qemu the ability to redirect net packet.
> redirector can redirect filter's net packet to outdev.
> and redirect indev's packet to filter.
>
>                       filter
>                         +
>                         |
>                         |
>             redirector  |
>                +--------------+
>                |        |     |
>                |        |     |
>                |        |     |
>   indev +-----------+   +---------->  outdev
>                |    |         |
>                |    |         |
>                |    |         |
>                +--------------+
>                     |
>                     |
>                     v
>                   filter
>
> usage:
>
> -netdev user,id=hn0
> -chardev socket,id=s0,host=ip_primary,port=X,server,nowait
> -chardev socket,id=s1,host=ip_primary,port=Y,server,nowait
> -filter-redirector,id=r0,netdev=hn0,queue=tx/rx/all,indev=s0,outdev=s1
>
> Signed-off-by: Zhang Chen <address@hidden>
> Signed-off-by: Wen Congyang <address@hidden>
> ---
>  net/filter-mirror.c | 211 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx     |   8 ++
>  vl.c                |   3 +-
>  3 files changed, 221 insertions(+), 1 deletion(-)
>
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index 4ff7619..d137168 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -25,11 +25,19 @@
>  #define FILTER_MIRROR(obj) \
>      OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR)
>  
> +#define FILTER_REDIRECTOR(obj) \
> +    OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_REDIRECTOR)
> +
>  #define TYPE_FILTER_MIRROR "filter-mirror"
> +#define TYPE_FILTER_REDIRECTOR "filter-redirector"
> +#define REDIRECT_HEADER_LEN sizeof(uint32_t)
>  
>  typedef struct MirrorState {
>      NetFilterState parent_obj;
> +    NetQueue *incoming_queue;
> +    char *indev;
>      char *outdev;
> +    CharDriverState *chr_in;
>      CharDriverState *chr_out;
>  } MirrorState;
>  
> @@ -67,6 +75,68 @@ err:
>      return ret < 0 ? ret : -EIO;
>  }
>  
> +static int redirector_chr_can_read(void *opaque)
> +{
> +    return REDIRECT_HEADER_LEN;
> +}
> +
> +static void redirector_chr_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    NetFilterState *nf = opaque;
> +    MirrorState *s = FILTER_REDIRECTOR(nf);
> +    uint32_t len;
> +    int ret = 0;
> +    uint8_t *recv_buf;
> +
> +    memcpy(&len, buf, size);

stack overflow if size > sizeof(len)?

> +    if (size < REDIRECT_HEADER_LEN) {
> +        ret = qemu_chr_fe_read_all(s->chr_in, ((uint8_t *)&len) + size,
> +                REDIRECT_HEADER_LEN - size);

There maybe some misunderstanding for my previous reply. You can have a
look at net_socket_send() for reference. You could

- use a buffer for storing len
- each time when you receive partial len, store them in the buffer and
advance the pointer until you receive at least sizeof(len) bytes.

Then you can proceed and do something similar when you're receiving the
packet itself.

> +        if (ret != (REDIRECT_HEADER_LEN - size)) {
> +            error_report("filter-redirector recv size failed");
> +            return;
> +        }
> +    }
> +
> +    len = ntohl(len);
> +
> +    if (len > 0 && len < NET_BUFSIZE) {
> +        recv_buf = g_malloc(len);
> +        ret = qemu_chr_fe_read_all(s->chr_in, recv_buf, len);
> +        if (ret != len) {
> +            error_report("filter-redirector recv buf failed");
> +            g_free(recv_buf);
> +            return;
> +        }
> +
> +        if (nf->direction == NET_FILTER_DIRECTION_ALL ||
> +                nf->direction == NET_FILTER_DIRECTION_TX) {
> +            ret = qemu_net_queue_send(s->incoming_queue, nf->netdev,
> +                            0, (const uint8_t *)recv_buf, len, NULL);

Rethink about this, we don't queue any packet in incoming_queue, so
probably there's no need to have a incoming_queue for redirector. We can
just use qemu_netfilter_pass_to_next() here.

> +
> +            if (ret < 0) {
> +                /*
> +                  *FIXME: just report an error, let NET_FILTER_DIRECTION_RX 
> have chance to pass
> +                  * to next filter ?
> +                  */
> +                error_report("filter-redirector out to guest failed");
> +            }
> +        }
> +
> +        if (nf->direction == NET_FILTER_DIRECTION_ALL ||
> +                nf->direction == NET_FILTER_DIRECTION_RX) {
> +            struct iovec iov = {
> +                .iov_base = recv_buf,
> +                .iov_len = sizeof(recv_buf)
> +            };
> +            qemu_netfilter_pass_to_next(nf->netdev->peer, 0, &iov, 1, nf);

Another issue not relate to this patch. Looks like we can rename this to
qemu_netfilter_iterate(), which seems better. Care to send a patch of this?

> +        }
> +        g_free(recv_buf);
> +    } else {
> +        error_report("filter-redirector recv len failed");

What will happen if the socket was closed at the other end? Can we get
size == 0 here? Btw, the grammar looks not correct :)

> +    }
> +}
> +
>  static ssize_t filter_mirror_receive_iov(NetFilterState *nf,
>                                           NetClientState *sender,
>                                           unsigned flags,
> @@ -89,6 +159,27 @@ static ssize_t filter_mirror_receive_iov(NetFilterState 
> *nf,
>      return 0;
>  }
>  
> +static ssize_t filter_redirector_receive_iov(NetFilterState *nf,
> +                                         NetClientState *sender,
> +                                         unsigned flags,
> +                                         const struct iovec *iov,
> +                                         int iovcnt,
> +                                         NetPacketSent *sent_cb)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(nf);
> +    int ret;
> +
> +    if (s->chr_out) {
> +        ret = filter_mirror_send(s->chr_out, iov, iovcnt);
> +        if (ret) {
> +            error_report("filter_mirror_send failed(%s)", strerror(-ret));
> +        }
> +        return iov_size(iov, iovcnt);
> +    } else {
> +        return 0;
> +    }
> +}
> +
>  static void filter_mirror_cleanup(NetFilterState *nf)
>  {
>      MirrorState *s = FILTER_MIRROR(nf);
> @@ -98,6 +189,18 @@ static void filter_mirror_cleanup(NetFilterState *nf)
>      }
>  }
>  
> +static void filter_redirector_cleanup(NetFilterState *nf)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(nf);
> +
> +    if (s->chr_in) {
> +        qemu_chr_fe_release(s->chr_in);
> +    }
> +    if (s->chr_out) {
> +        qemu_chr_fe_release(s->chr_out);
> +    }
> +}
> +
>  static void filter_mirror_setup(NetFilterState *nf, Error **errp)
>  {
>      MirrorState *s = FILTER_MIRROR(nf);
> @@ -121,6 +224,47 @@ static void filter_mirror_setup(NetFilterState *nf, 
> Error **errp)
>      }
>  }
>  
> +static void filter_redirector_setup(NetFilterState *nf, Error **errp)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(nf);
> +
> +    if (!s->indev && !s->outdev) {
> +        error_setg(errp, "filter redirector needs 'indev' or "
> +                "'outdev' at least one property set");
> +        return;
> +    } else if (s->indev && s->outdev) {
> +        if (!strcmp(s->indev, s->outdev)) {
> +            error_setg(errp, "filter redirector needs 'indev' and "
> +                    "'outdev'  are not same");
> +            return;
> +        }
> +    }
> +
> +    if (s->indev) {
> +        s->chr_in = qemu_chr_find(s->indev);
> +        if (s->chr_in == NULL) {
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "IN Device '%s' not found", s->indev);
> +            return;
> +        }
> +
> +        qemu_chr_fe_claim_no_fail(s->chr_in);
> +        qemu_chr_add_handlers(s->chr_in, redirector_chr_can_read,
> +                                redirector_chr_read, NULL, nf);
> +        s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, 
> nf);
> +    }
> +
> +    if (s->outdev) {
> +        s->chr_out = qemu_chr_find(s->outdev);
> +        if (s->chr_out == NULL) {
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "OUT Device '%s' not found", s->outdev);
> +            return;
> +        }
> +        qemu_chr_fe_claim_no_fail(s->chr_out);
> +    }
> +}
> +
>  static void filter_mirror_class_init(ObjectClass *oc, void *data)
>  {
>      NetFilterClass *nfc = NETFILTER_CLASS(oc);
> @@ -130,6 +274,31 @@ static void filter_mirror_class_init(ObjectClass *oc, 
> void *data)
>      nfc->receive_iov = filter_mirror_receive_iov;
>  }
>  
> +static void filter_redirector_class_init(ObjectClass *oc, void *data)
> +{
> +    NetFilterClass *nfc = NETFILTER_CLASS(oc);
> +
> +    nfc->setup = filter_redirector_setup;
> +    nfc->cleanup = filter_redirector_cleanup;
> +    nfc->receive_iov = filter_redirector_receive_iov;
> +}
> +
> +static char *filter_redirector_get_indev(Object *obj, Error **errp)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(obj);
> +
> +    return g_strdup(s->indev);
> +}
> +
> +static void
> +filter_redirector_set_indev(Object *obj, const char *value, Error **errp)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(obj);
> +
> +    g_free(s->indev);
> +    s->indev = g_strdup(value);
> +}
> +
>  static char *filter_mirror_get_outdev(Object *obj, Error **errp)
>  {
>      MirrorState *s = FILTER_MIRROR(obj);
> @@ -151,12 +320,36 @@ filter_mirror_set_outdev(Object *obj, const char 
> *value, Error **errp)
>      }
>  }
>  
> +static char *filter_redirector_get_outdev(Object *obj, Error **errp)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(obj);
> +
> +    return g_strdup(s->outdev);
> +}
> +
> +static void
> +filter_redirector_set_outdev(Object *obj, const char *value, Error **errp)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(obj);
> +
> +    g_free(s->outdev);
> +    s->outdev = g_strdup(value);
> +}
> +
>  static void filter_mirror_init(Object *obj)
>  {
>      object_property_add_str(obj, "outdev", filter_mirror_get_outdev,
>                              filter_mirror_set_outdev, NULL);
>  }
>  
> +static void filter_redirector_init(Object *obj)
> +{
> +    object_property_add_str(obj, "indev", filter_redirector_get_indev,
> +                            filter_redirector_set_indev, NULL);
> +    object_property_add_str(obj, "outdev", filter_redirector_get_outdev,
> +                            filter_redirector_set_outdev, NULL);
> +}
> +
>  static void filter_mirror_fini(Object *obj)
>  {
>      MirrorState *s = FILTER_MIRROR(obj);
> @@ -164,6 +357,23 @@ static void filter_mirror_fini(Object *obj)
>      g_free(s->outdev);
>  }
>  
> +static void filter_redirector_fini(Object *obj)
> +{
> +    MirrorState *s = FILTER_REDIRECTOR(obj);
> +
> +    g_free(s->indev);
> +    g_free(s->outdev);
> +}
> +
> +static const TypeInfo filter_redirector_info = {
> +    .name = TYPE_FILTER_REDIRECTOR,
> +    .parent = TYPE_NETFILTER,
> +    .class_init = filter_redirector_class_init,
> +    .instance_init = filter_redirector_init,
> +    .instance_finalize = filter_redirector_fini,
> +    .instance_size = sizeof(MirrorState),
> +};
> +
>  static const TypeInfo filter_mirror_info = {
>      .name = TYPE_FILTER_MIRROR,
>      .parent = TYPE_NETFILTER,
> @@ -176,6 +386,7 @@ static const TypeInfo filter_mirror_info = {
>  static void register_types(void)
>  {
>      type_register_static(&filter_mirror_info);
> +    type_register_static(&filter_redirector_info);
>  }
>  
>  type_init(register_types);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ca27863..84584e3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3764,6 +3764,14 @@ queue @var{all|rx|tx} is an option that can be applied 
> to any netfilter.
>  filter-mirror on netdev @var{netdevid},mirror net packet to chardev
>  @var{chardevid}
>  
> address@hidden -object 
> filter-redirector,address@hidden,address@hidden,address@hidden,
> address@hidden,address@hidden|rx|tx}]
> +
> +filter-redirector on netdev @var{netdevid},redirect filter's net packet to 
> chardev
> address@hidden,and redirect indev's packet to filter.
> +Create a filter-redirector we need to differ outdev id from indev id, id can 
> not
> +be the same. we can just use indev or outdev, but at least one to be set.

Not a native English speaker, but this looks better:

At least one of indev or outdev need to be specified.

> +
>  @item -object 
> filter-dump,address@hidden,address@hidden,address@hidden,address@hidden
>  
>  Dump the network traffic on netdev @var{dev} to the file specified by
> diff --git a/vl.c b/vl.c
> index d68533a..c389a3f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2799,7 +2799,8 @@ static bool object_create_initial(const char *type)
>       */
>      if (g_str_equal(type, "filter-buffer") ||
>          g_str_equal(type, "filter-dump") ||
> -        g_str_equal(type, "filter-mirror")) {
> +        g_str_equal(type, "filter-mirror") ||
> +        g_str_equal(type, "filter-redirector")) {
>          return false;
>      }
>