From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ag8MW-00045l-FG for qemu-devel@nongnu.org; Wed, 16 Mar 2016 06:07:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ag8MR-00049h-AG for qemu-devel@nongnu.org; Wed, 16 Mar 2016 06:07:32 -0400 Received: from [59.151.112.132] (port=45565 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ag8MP-00046o-T1 for qemu-devel@nongnu.org; Wed, 16 Mar 2016 06:07:27 -0400 References: <1458036214-867-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <1458036214-867-2-git-send-email-zhangchen.fnst@cn.fujitsu.com> <56E916EB.3050704@redhat.com> <56E928C2.60100@cn.fujitsu.com> From: Li Zhijian Message-ID: <56E93050.9080009@cn.fujitsu.com> Date: Wed, 16 Mar 2016 18:07:12 +0800 MIME-Version: 1.0 In-Reply-To: <56E928C2.60100@cn.fujitsu.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 1/2] net/filter-mirror: implement filter-redirector List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wen Congyang , Jason Wang , Zhang Chen , qemu devel Cc: Gui jianfeng , Yang Hongyang , "eddie.dong" , zhanghailiang , "Dr. David Alan Gilbert" On 03/16/2016 05:34 PM, Wen Congyang wrote: > 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 >>> Signed-off-by: Wen Congyang >>> Signed-off-by: Li Zhijian >>> --- >>> 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? It seems that the caller will never passes a '0' size to it. So the size will never be 0. But I perfer to have a event handler. e.g. - peer is closed after sending the length part only - read handler always expect the data part - (another) peer is connected again(assume peer can connect successfully) - peer will send a length part first, which will confuse the read 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 I think this is an unrecoverable error, we reset the fd handler would be better. Thanks Li > >> >>> + 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} >>> >>> +@item -object filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid}, >>> +outdev=@var{chardevid}[,queue=@var{all|rx|tx}] >>> + >>> +filter-redirector on netdev @var{netdevid},redirect filter's net packet to chardev >>> +@var{chardevid},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,id=@var{id},netdev=@var{dev},file=@var{filename}][,maxlen=@var{len}] >>> >>> 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; >>> } >>> >> >> >> >> >> . >> > > . > -- Best regards. Li Zhijian (8555)