Re: [Qemu-devel] [PATCH V4 1/2] net/filter-mirror: implement filter-redirector

[Wen Congyang]

On 03/16/2016 04:18 PM, Jason Wang wrote:
> 
> 
> On 03/15/2016 06:03 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>
>> Signed-off-by: Li Zhijian <address@hidden>
>> ---
>>  net/filter-mirror.c | 236 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-options.hx     |   9 ++
>>  vl.c                |   3 +-
>>  3 files changed, 247 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
>> index 1b1ec16..77ece41 100644
>> --- a/net/filter-mirror.c
>> +++ b/net/filter-mirror.c
>> @@ -26,12 +26,23 @@
>>  #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 REDIRECTOR_MAX_LEN NET_BUFSIZE
>>  
>>  typedef struct MirrorState {
>>      NetFilterState parent_obj;
>> +    char *indev;
>>      char *outdev;
>> +    CharDriverState *chr_in;
>>      CharDriverState *chr_out;
>> +    int state; /* 0 = getting length, 1 = getting data */
>> +    unsigned int index;
>> +    unsigned int packet_len;
>> +    uint8_t buf[REDIRECTOR_MAX_LEN];
>>  } MirrorState;
>>  
>>  static int filter_mirror_send(CharDriverState *chr_out,
>> @@ -68,6 +79,89 @@ err:
>>      return ret < 0 ? ret : -EIO;
>>  }
>>  
>> +static void
>> +redirector_to_filter(NetFilterState *nf, const uint8_t *buf, int len)
>> +{
>> +    struct iovec iov = {
>> +        .iov_base = (void *)buf,
>> +        .iov_len = len,
>> +    };
>> +
>> +    if (nf->direction == NET_FILTER_DIRECTION_ALL ||
>> +        nf->direction == NET_FILTER_DIRECTION_TX) {
>> +        qemu_netfilter_pass_to_next(nf->netdev, 0, &iov, 1, nf);
>> +    }
>> +
>> +    if (nf->direction == NET_FILTER_DIRECTION_ALL ||
>> +        nf->direction == NET_FILTER_DIRECTION_RX) {
>> +        qemu_netfilter_pass_to_next(nf->netdev->peer, 0, &iov, 1, nf);
>> +     }
>> +}
>> +
>> +static int redirector_chr_can_read(void *opaque)
>> +{
>> +    return REDIRECTOR_MAX_LEN;
>> +}
>> +
>> +static void redirector_chr_read(void *opaque, const uint8_t *buf, int size)
>> +{
>> +    NetFilterState *nf = opaque;
>> +    MirrorState *s = FILTER_REDIRECTOR(nf);
>> +    unsigned int l;
>> +
>> +    if (size == 0) {
>> +        /* the peer is closed ? */
>> +        return ;
>> +    }
> 
> Looks like if you want to handle connection close, you need use event
> handler when calling qemu_chr_add_handlers().

In which case, we will see size is 0 if we don't have a event handler?

For redirector filter, I think we don't care about if the char device
is disconnected. If the char device is ready again, we will continue
to read from the char device.

So I think we just add more comments here.

> 
>> +
>> +    /* most of code is stolen from net_socket_send */
> 
> This comment seems redundant.
> 
>> +    while (size > 0) {
>> +        /* reassemble a packet from the network */
>> +        switch (s->state) {
>> +        case 0:
>> +            l = 4 - s->index;
>> +            if (l > size) {
>> +                l = size;
>> +            }
>> +            memcpy(s->buf + s->index, buf, l);
>> +            buf += l;
>> +            size -= l;
>> +            s->index += l;
>> +            if (s->index == 4) {
>> +                /* got length */
>> +                s->packet_len = ntohl(*(uint32_t *)s->buf);
>> +                s->index = 0;
>> +                s->state = 1;
>> +            }
>> +            break;
>> +        case 1:
>> +            l = s->packet_len - s->index;
>> +            if (l > size) {
>> +                l = size;
>> +            }
>> +            if (s->index + l <= sizeof(s->buf)) {
>> +                memcpy(s->buf + s->index, buf, l);
>> +            } else {
>> +                fprintf(stderr, "serious error: oversized packet received,"
>> +                    "connection terminated.\n");
> 
> error_report() looks better.
> 
>> +                s->state = 0;
>> +                /* FIXME: do something ? */
> 
> This needs some thought, but at least reset the fd handler and state is
> needed.

Maybe we can read the data from the chardev, and drop it?

Thanks
Wen Congyang

> 
>> +                return;
>> +            }
>> +
>> +            s->index += l;
>> +            buf += l;
>> +            size -= l;
>> +            if (s->index >= s->packet_len) {
>> +                s->index = 0;
>> +                s->state = 0;
>> +                redirector_to_filter(nf, s->buf, s->packet_len);
>> +            }
>> +            break;
>> +        }
>> +    }
>> +}
>> +
>>  static ssize_t filter_mirror_receive_iov(NetFilterState *nf,
>>                                           NetClientState *sender,
>>                                           unsigned flags,
>> @@ -90,6 +184,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);
>> @@ -99,6 +214,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);
>> @@ -122,6 +249,48 @@ 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;
>> +        }
>> +    }
>> +
>> +    s->state = s->index = 0;
>> +
>> +    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);
>> +    }
>> +
>> +    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);
>> @@ -131,6 +300,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);
>> @@ -152,12 +346,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);
>> @@ -165,6 +383,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);
> 
> We probably want to reset chr handlers there, otherwise there may be
> use-after-free.
> 
>> +}
>> +
>> +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,
>> @@ -177,6 +412,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 5379cb5..796d053 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3821,6 +3821,15 @@ 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 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 79bb11a..dc6e63a 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2841,7 +2841,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;
>>      }
>>  
> 
> 
> 
> 
> .
>