| Message ID | 1496829322-17099-3-git-send-email-zhangchen.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 2017年06月07日 17:55, Zhang Chen wrote: > We add the vnet_hdr_support option for filter-mirror, default is disable. s/disable/disabled/ > If you use virtio-net-pci net driver, please enable it. > You can use it for example: > -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0,vnet_hdr_support > > If have vnet_hdr_support flag, If it has > we will change the send packet format from s/send/sending/ > struct {int size; const uint8_t buf[];} to {int size; int vnet_hdr_len; const uint8_t buf[];}. > make other module(like colo-compare) know how to parse net packet correctly. Please double check the commit logs, some have obvious grammar issues. > > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > --- > net/filter-mirror.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---- > qemu-options.hx | 5 ++-- > 2 files changed, 66 insertions(+), 8 deletions(-) > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c > index 72fa7c2..50aa81b 100644 > --- a/net/filter-mirror.c > +++ b/net/filter-mirror.c > @@ -38,15 +38,17 @@ typedef struct MirrorState { > NetFilterState parent_obj; > char *indev; > char *outdev; > + bool vnet_hdr; > CharBackend chr_in; > CharBackend chr_out; > SocketReadState rs; > } MirrorState; > > -static int filter_mirror_send(CharBackend *chr_out, > +static int filter_mirror_send(MirrorState *s, > const struct iovec *iov, > int iovcnt) This function were renamed to filter_send in your commit e05dc4cf56d70225fc76225a3fd7df9f7b8ca5f9 ("net/filter-mirror.c: Rename filter_mirror_send() and fix codestyle") > { > + NetFilterState *nf = NETFILTER(s); > int ret = 0; > ssize_t size = 0; > uint32_t len = 0; > @@ -58,14 +60,43 @@ static int filter_mirror_send(CharBackend *chr_out, > } > > len = htonl(size); > - ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)&len, sizeof(len)); > + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); This looks a independent change, please do it in another patch. > if (ret != sizeof(len)) { > goto err; > } > > + if (s->vnet_hdr) { > + /* > + * If vnet_hdr = on, we send vnet header len to make other > + * module(like colo-compare) know how to parse net > + * packet correctly. > + */ > + ssize_t vnet_hdr_len; > + > + /* > + * In anytime, nf->netdev and nf->netdev->peer both have a vnet_hdr_len, > + * Here we just find out which is we need. When filter set RX or TX > + * that the real vnet_hdr_len are different. > + */ Let's remove the comment and let code explain itself. > + if (nf->direction == NET_FILTER_DIRECTION_RX || > + nf->direction == NET_FILTER_DIRECTION_ALL) { > + vnet_hdr_len = nf->netdev->vnet_hdr_len; This can only work if e.g virtio-net set its own vnet_hdr_len. But looks like it use guest_hdr_len instead. And using NET_FILTER_DIRECTION_ALL looks suspicious. Maybe we should accept sender from its caller and compare it with nf->netdev to get the correct direction. > + } else if (nf->direction == NET_FILTER_DIRECTION_TX) { > + vnet_hdr_len = nf->netdev->peer->vnet_hdr_len; This looks wrong, TX means the packet were sent from netdev, we should care nf->netdev's hdrlen. > + } else { > + return 0; > + } > + > + len = htonl(vnet_hdr_len); > + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); > + if (ret != sizeof(len)) { > + goto err; > + } > + } > + > buf = g_malloc(size); > iov_to_buf(iov, iovcnt, 0, buf, size); > - ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)buf, size); > + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size); > g_free(buf); > if (ret != size) { > goto err; > @@ -141,7 +172,7 @@ static ssize_t filter_mirror_receive_iov(NetFilterState *nf, > MirrorState *s = FILTER_MIRROR(nf); > int ret; > > - ret = filter_mirror_send(&s->chr_out, iov, iovcnt); > + ret = filter_mirror_send(s, iov, iovcnt); > if (ret) { > error_report("filter_mirror_send failed(%s)", strerror(-ret)); > } > @@ -164,7 +195,7 @@ static ssize_t filter_redirector_receive_iov(NetFilterState *nf, > int ret; > > if (qemu_chr_fe_get_driver(&s->chr_out)) { > - ret = filter_mirror_send(&s->chr_out, iov, iovcnt); > + ret = filter_mirror_send(s, iov, iovcnt); > if (ret) { > error_report("filter_mirror_send failed(%s)", strerror(-ret)); > } > @@ -308,6 +339,13 @@ static char *filter_mirror_get_outdev(Object *obj, Error **errp) > return g_strdup(s->outdev); > } > > +static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp) > +{ > + MirrorState *s = FILTER_MIRROR(obj); > + > + return s->vnet_hdr; > +} > + > static void > filter_mirror_set_outdev(Object *obj, const char *value, Error **errp) > { > @@ -322,6 +360,15 @@ filter_mirror_set_outdev(Object *obj, const char *value, Error **errp) > } > } > > +static void filter_mirror_set_vnet_hdr(Object *obj, > + bool value, > + Error **errp) > +{ > + MirrorState *s = FILTER_MIRROR(obj); > + > + s->vnet_hdr = value; > +} > + > static char *filter_redirector_get_outdev(Object *obj, Error **errp) > { > MirrorState *s = FILTER_REDIRECTOR(obj); > @@ -340,8 +387,20 @@ filter_redirector_set_outdev(Object *obj, const char *value, Error **errp) > > static void filter_mirror_init(Object *obj) > { > + MirrorState *s = FILTER_MIRROR(obj); > + > object_property_add_str(obj, "outdev", filter_mirror_get_outdev, > filter_mirror_set_outdev, NULL); > + > + /* > + * The vnet_hdr is disabled by default, if you want to enable > + * this option, you must enable all the option on related modules > + * (like other filter or colo-compare). > + */ This comment makes sense for docs but is useless here, let code explain itself. Thanks > + s->vnet_hdr = false; > + object_property_add_bool(obj, "vnet_hdr_support", > + filter_mirror_get_vnet_hdr, > + filter_mirror_set_vnet_hdr, NULL); > } > > static void filter_redirector_init(Object *obj) > diff --git a/qemu-options.hx b/qemu-options.hx > index 70c0ded..5c09fae 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -4024,10 +4024,9 @@ queue @var{all|rx|tx} is an option that can be applied to any netfilter. > @option{tx}: the filter is attached to the transmit queue of the netdev, > where it will receive packets sent by the netdev. > > -@item -object filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid}[,queue=@var{all|rx|tx}] > +@item -object filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support] > > -filter-mirror on netdev @var{netdevid},mirror net packet to chardev > -@var{chardevid} > +filter-mirror on netdev @var{netdevid},mirror net packet to chardev@var{chardevid}, if have the vnet_hdr_support flag, filter-mirror will mirror packet with vnet_hdr_len. > > @item -object filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid}, > outdev=@var{chardevid}[,queue=@var{all|rx|tx}]
On 06/09/2017 02:08 PM, Jason Wang wrote: > > > On 2017年06月07日 17:55, Zhang Chen wrote: >> We add the vnet_hdr_support option for filter-mirror, default is >> disable. > > s/disable/disabled/ > >> If you use virtio-net-pci net driver, please enable it. >> You can use it for example: >> -object >> filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0,vnet_hdr_support >> >> If have vnet_hdr_support flag, > > If it has > >> we will change the send packet format from > > s/send/sending/ > >> struct {int size; const uint8_t buf[];} to {int size; int >> vnet_hdr_len; const uint8_t buf[];}. >> make other module(like colo-compare) know how to parse net packet >> correctly. > > Please double check the commit logs, some have obvious grammar issues. OK, I will fix the garmmar issues. > >> >> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> --- >> net/filter-mirror.c | 69 >> +++++++++++++++++++++++++++++++++++++++++++++++++---- >> qemu-options.hx | 5 ++-- >> 2 files changed, 66 insertions(+), 8 deletions(-) >> >> diff --git a/net/filter-mirror.c b/net/filter-mirror.c >> index 72fa7c2..50aa81b 100644 >> --- a/net/filter-mirror.c >> +++ b/net/filter-mirror.c >> @@ -38,15 +38,17 @@ typedef struct MirrorState { >> NetFilterState parent_obj; >> char *indev; >> char *outdev; >> + bool vnet_hdr; >> CharBackend chr_in; >> CharBackend chr_out; >> SocketReadState rs; >> } MirrorState; >> -static int filter_mirror_send(CharBackend *chr_out, >> +static int filter_mirror_send(MirrorState *s, >> const struct iovec *iov, >> int iovcnt) > > This function were renamed to filter_send in your commit > e05dc4cf56d70225fc76225a3fd7df9f7b8ca5f9 ("net/filter-mirror.c: Rename > filter_mirror_send() and fix codestyle") I will rebase the code for upstream. > >> { >> + NetFilterState *nf = NETFILTER(s); >> int ret = 0; >> ssize_t size = 0; >> uint32_t len = 0; >> @@ -58,14 +60,43 @@ static int filter_mirror_send(CharBackend *chr_out, >> } >> len = htonl(size); >> - ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)&len, sizeof(len)); >> + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, >> sizeof(len)); > > This looks a independent change, please do it in another patch. OK. > >> if (ret != sizeof(len)) { >> goto err; >> } >> + if (s->vnet_hdr) { >> + /* >> + * If vnet_hdr = on, we send vnet header len to make other >> + * module(like colo-compare) know how to parse net >> + * packet correctly. >> + */ >> + ssize_t vnet_hdr_len; >> + >> + /* >> + * In anytime, nf->netdev and nf->netdev->peer both have a >> vnet_hdr_len, >> + * Here we just find out which is we need. When filter set >> RX or TX >> + * that the real vnet_hdr_len are different. >> + */ > > Let's remove the comment and let code explain itself. I will remove it in next version. > >> + if (nf->direction == NET_FILTER_DIRECTION_RX || >> + nf->direction == NET_FILTER_DIRECTION_ALL) { >> + vnet_hdr_len = nf->netdev->vnet_hdr_len; > > This can only work if e.g virtio-net set its own vnet_hdr_len. But > looks like it use guest_hdr_len instead. I see in hw/net/virtio-net.c use the "qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);" to set the nf->netdev->vnet_hdr_len, any case not include here? > > And using NET_FILTER_DIRECTION_ALL looks suspicious. Maybe we should > accept sender from its caller and compare it with nf->netdev to get > the correct direction. > >> + } else if (nf->direction == NET_FILTER_DIRECTION_TX) { >> + vnet_hdr_len = nf->netdev->peer->vnet_hdr_len; > > This looks wrong, TX means the packet were sent from netdev, we should > care nf->netdev's hdrlen. I rethink and test the code, you are right. I will remove it. > >> + } else { >> + return 0; >> + } >> + >> + len = htonl(vnet_hdr_len); >> + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, >> sizeof(len)); >> + if (ret != sizeof(len)) { >> + goto err; >> + } >> + } >> + >> buf = g_malloc(size); >> iov_to_buf(iov, iovcnt, 0, buf, size); >> - ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)buf, size); >> + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size); >> g_free(buf); >> if (ret != size) { >> goto err; >> @@ -141,7 +172,7 @@ static ssize_t >> filter_mirror_receive_iov(NetFilterState *nf, >> MirrorState *s = FILTER_MIRROR(nf); >> int ret; >> - ret = filter_mirror_send(&s->chr_out, iov, iovcnt); >> + ret = filter_mirror_send(s, iov, iovcnt); >> if (ret) { >> error_report("filter_mirror_send failed(%s)", strerror(-ret)); >> } >> @@ -164,7 +195,7 @@ static ssize_t >> filter_redirector_receive_iov(NetFilterState *nf, >> int ret; >> if (qemu_chr_fe_get_driver(&s->chr_out)) { >> - ret = filter_mirror_send(&s->chr_out, iov, iovcnt); >> + ret = filter_mirror_send(s, iov, iovcnt); >> if (ret) { >> error_report("filter_mirror_send failed(%s)", >> strerror(-ret)); >> } >> @@ -308,6 +339,13 @@ static char *filter_mirror_get_outdev(Object >> *obj, Error **errp) >> return g_strdup(s->outdev); >> } >> +static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp) >> +{ >> + MirrorState *s = FILTER_MIRROR(obj); >> + >> + return s->vnet_hdr; >> +} >> + >> static void >> filter_mirror_set_outdev(Object *obj, const char *value, Error **errp) >> { >> @@ -322,6 +360,15 @@ filter_mirror_set_outdev(Object *obj, const char >> *value, Error **errp) >> } >> } >> +static void filter_mirror_set_vnet_hdr(Object *obj, >> + bool value, >> + Error **errp) >> +{ >> + MirrorState *s = FILTER_MIRROR(obj); >> + >> + s->vnet_hdr = value; >> +} >> + >> static char *filter_redirector_get_outdev(Object *obj, Error **errp) >> { >> MirrorState *s = FILTER_REDIRECTOR(obj); >> @@ -340,8 +387,20 @@ filter_redirector_set_outdev(Object *obj, const >> char *value, Error **errp) >> static void filter_mirror_init(Object *obj) >> { >> + MirrorState *s = FILTER_MIRROR(obj); >> + >> object_property_add_str(obj, "outdev", filter_mirror_get_outdev, >> filter_mirror_set_outdev, NULL); >> + >> + /* >> + * The vnet_hdr is disabled by default, if you want to enable >> + * this option, you must enable all the option on related modules >> + * (like other filter or colo-compare). >> + */ > > This comment makes sense for docs but is useless here, let code > explain itself. OK. Thanks Zhang Chen > > Thanks > >> + s->vnet_hdr = false; >> + object_property_add_bool(obj, "vnet_hdr_support", >> + filter_mirror_get_vnet_hdr, >> + filter_mirror_set_vnet_hdr, NULL); >> } >> static void filter_redirector_init(Object *obj) >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 70c0ded..5c09fae 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -4024,10 +4024,9 @@ queue @var{all|rx|tx} is an option that can be >> applied to any netfilter. >> @option{tx}: the filter is attached to the transmit queue of the >> netdev, >> where it will receive packets sent by the netdev. >> -@item -object >> filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid}[,queue=@var{all|rx|tx}] >> +@item -object >> filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support] >> -filter-mirror on netdev @var{netdevid},mirror net packet to chardev >> -@var{chardevid} >> +filter-mirror on netdev @var{netdevid},mirror net packet to >> chardev@var{chardevid}, if have the vnet_hdr_support flag, >> filter-mirror will mirror packet with vnet_hdr_len. >> @item -object >> filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid}, >> outdev=@var{chardevid}[,queue=@var{all|rx|tx}] > > > > . >
On 2017年06月12日 17:27, Zhang Chen wrote: >> >>> + if (nf->direction == NET_FILTER_DIRECTION_RX || >>> + nf->direction == NET_FILTER_DIRECTION_ALL) { >>> + vnet_hdr_len = nf->netdev->vnet_hdr_len; >> >> This can only work if e.g virtio-net set its own vnet_hdr_len. But >> looks like it use guest_hdr_len instead. > > I see in hw/net/virtio-net.c use the "qemu_set_vnet_hdr_len(nc->peer, > n->guest_hdr_len);" to > set the nf->netdev->vnet_hdr_len, any case not include here? You mean: if (peer_has_vnet_hdr(n) && qemu_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) { qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len); n->host_hdr_len = n->guest_hdr_len; } ? From the code, it only set peer's vnet header when peer supports vnet_hdr up to n->guest_hdr_len. If peer can't, virtio-net will strip the header. Thanks
On 06/13/2017 05:14 PM, Jason Wang wrote: > > > On 2017年06月12日 17:27, Zhang Chen wrote: >>> >>>> + if (nf->direction == NET_FILTER_DIRECTION_RX || >>>> + nf->direction == NET_FILTER_DIRECTION_ALL) { >>>> + vnet_hdr_len = nf->netdev->vnet_hdr_len; >>> >>> This can only work if e.g virtio-net set its own vnet_hdr_len. But >>> looks like it use guest_hdr_len instead. >> >> I see in hw/net/virtio-net.c use the "qemu_set_vnet_hdr_len(nc->peer, >> n->guest_hdr_len);" to >> set the nf->netdev->vnet_hdr_len, any case not include here? > > You mean: > > if (peer_has_vnet_hdr(n) && > qemu_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) { > qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len); > n->host_hdr_len = n->guest_hdr_len; > } > > ? > > From the code, it only set peer's vnet header when peer supports > vnet_hdr up to n->guest_hdr_len. If peer can't, virtio-net will strip > the header. So, We should get the n->guest_hdr_len in another way? like set a new parameter in NetClientState? Any suggestion about it? Thanks Zhang Chen > > Thanks > > >
On 2017年06月14日 16:04, Zhang Chen wrote: > > > On 06/13/2017 05:14 PM, Jason Wang wrote: >> >> >> On 2017年06月12日 17:27, Zhang Chen wrote: >>>> >>>>> + if (nf->direction == NET_FILTER_DIRECTION_RX || >>>>> + nf->direction == NET_FILTER_DIRECTION_ALL) { >>>>> + vnet_hdr_len = nf->netdev->vnet_hdr_len; >>>> >>>> This can only work if e.g virtio-net set its own vnet_hdr_len. But >>>> looks like it use guest_hdr_len instead. >>> >>> I see in hw/net/virtio-net.c use the >>> "qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);" to >>> set the nf->netdev->vnet_hdr_len, any case not include here? >> >> You mean: >> >> if (peer_has_vnet_hdr(n) && >> qemu_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) { >> qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len); >> n->host_hdr_len = n->guest_hdr_len; >> } >> >> ? >> >> From the code, it only set peer's vnet header when peer supports >> vnet_hdr up to n->guest_hdr_len. If peer can't, virtio-net will strip >> the header. > > So, We should get the n->guest_hdr_len in another way? like set a new > parameter in NetClientState? > Any suggestion about it? > > Thanks > Zhang Chen Rethink about this, a question is do we really care guest vnet header len? During guest transmission, when packet reaches netfilter. The vnet_header should have been stripped to netdev->vnet_hdr_len. I still think use nf->netdev->vnet_hdr_len is sufficient. Thanks > >> >> Thanks >> >> >> >
On 06/15/2017 12:31 PM, Jason Wang wrote: > > > On 2017年06月14日 16:04, Zhang Chen wrote: >> >> >> On 06/13/2017 05:14 PM, Jason Wang wrote: >>> >>> >>> On 2017年06月12日 17:27, Zhang Chen wrote: >>>>> >>>>>> + if (nf->direction == NET_FILTER_DIRECTION_RX || >>>>>> + nf->direction == NET_FILTER_DIRECTION_ALL) { >>>>>> + vnet_hdr_len = nf->netdev->vnet_hdr_len; >>>>> >>>>> This can only work if e.g virtio-net set its own vnet_hdr_len. But >>>>> looks like it use guest_hdr_len instead. >>>> >>>> I see in hw/net/virtio-net.c use the >>>> "qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);" to >>>> set the nf->netdev->vnet_hdr_len, any case not include here? >>> >>> You mean: >>> >>> if (peer_has_vnet_hdr(n) && >>> qemu_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) { >>> qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len); >>> n->host_hdr_len = n->guest_hdr_len; >>> } >>> >>> ? >>> >>> From the code, it only set peer's vnet header when peer supports >>> vnet_hdr up to n->guest_hdr_len. If peer can't, virtio-net will >>> strip the header. >> >> So, We should get the n->guest_hdr_len in another way? like set a new >> parameter in NetClientState? >> Any suggestion about it? >> >> Thanks >> Zhang Chen > > Rethink about this, a question is do we really care guest vnet header > len? During guest transmission, when packet reaches netfilter. The > vnet_header should have been stripped to netdev->vnet_hdr_len. But we use filter-redirector send packet to colo-compare that get the packet with the vnet header. That is colo-compare see: 1241@1497507100.108799:colo_compare_pkt_info_src src/dst: 192.168.4.144 s: seq/ack=2181260317/2075177759 res=0 flags=18 spkt_size: 95 1241@1497507100.108803:colo_compare_pkt_info_dst src/dst: 192.168.4.88 d: seq/ack=2181260317/2075177759 res=0 flags=18 dpkt_size: 95 colo-compare ppkt: 0000: 01 00 00 00 00 00 22 00 10 00 00 00 12 14 fc 1d ......"......... colo-compare ppkt: 0010: 1a 6b 52 54 00 12 34 56 08 00 45 00 00 45 12 61 .kRT..4V..E..E.a colo-compare ppkt: 0020: 40 00 40 06 9e 19 c0 a8 04 90 c0 a8 04 58 04 d2 @.@..........X.. colo-compare ppkt: 0030: ca 98 82 03 64 1d 7b b0 b3 1f 80 18 00 e3 8a 70 ....d.{........p colo-compare ppkt: 0040: 00 00 01 01 08 0a 00 00 5b fa 00 4d f1 aa 73 65 ........[..M..se colo-compare ppkt: 0050: 72 76 65 72 20 67 65 74 20 69 74 20 34 36 38 rver get it 468 colo-compare spkt: 0000: 01 00 00 00 00 00 22 00 10 00 00 00 12 14 fc 1d ......"......... colo-compare spkt: 0010: 1a 6b 52 54 00 12 34 56 08 00 45 00 00 45 12 61 .kRT..4V..E..E.a colo-compare spkt: 0020: 40 00 40 06 9e 19 c0 a8 04 90 c0 a8 04 58 04 d2 @.@..........X.. colo-compare spkt: 0030: ca 98 82 03 64 1d 7b b0 b3 1f 80 18 00 e3 8a 70 ....d.{........p colo-compare spkt: 0040: 00 00 01 01 08 0a 00 00 5c 02 00 4d f1 aa 73 65 ........\..M..se colo-compare spkt: 0050: 72 76 65 72 20 67 65 74 20 69 74 20 34 36 38 rver get it 468 So, colo-compare need to know the vnet_hdr_len for skip this field, then colo-compare can parse the net packet correctly. Back to this patch, how should I fix it? Thanks Zhang Chen > > I still think use nf->netdev->vnet_hdr_len is sufficient. > > Thanks > >> >>> >>> Thanks >>> >>> >>> >> > > > > . >
On 2017年06月15日 16:10, Zhang Chen wrote: > > > On 06/15/2017 12:31 PM, Jason Wang wrote: >> >> >> On 2017年06月14日 16:04, Zhang Chen wrote: >>> >>> >>> On 06/13/2017 05:14 PM, Jason Wang wrote: >>>> >>>> >>>> On 2017年06月12日 17:27, Zhang Chen wrote: >>>>>> >>>>>>> + if (nf->direction == NET_FILTER_DIRECTION_RX || >>>>>>> + nf->direction == NET_FILTER_DIRECTION_ALL) { >>>>>>> + vnet_hdr_len = nf->netdev->vnet_hdr_len; >>>>>> >>>>>> This can only work if e.g virtio-net set its own vnet_hdr_len. >>>>>> But looks like it use guest_hdr_len instead. >>>>> >>>>> I see in hw/net/virtio-net.c use the >>>>> "qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);" to >>>>> set the nf->netdev->vnet_hdr_len, any case not include here? >>>> >>>> You mean: >>>> >>>> if (peer_has_vnet_hdr(n) && >>>> qemu_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) { >>>> qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len); >>>> n->host_hdr_len = n->guest_hdr_len; >>>> } >>>> >>>> ? >>>> >>>> From the code, it only set peer's vnet header when peer supports >>>> vnet_hdr up to n->guest_hdr_len. If peer can't, virtio-net will >>>> strip the header. >>> >>> So, We should get the n->guest_hdr_len in another way? like set a >>> new parameter in NetClientState? >>> Any suggestion about it? >>> >>> Thanks >>> Zhang Chen >> >> Rethink about this, a question is do we really care guest vnet header >> len? During guest transmission, when packet reaches netfilter. The >> vnet_header should have been stripped to netdev->vnet_hdr_len. > > But we use filter-redirector send packet to colo-compare that get the > packet with the vnet header. > > That is colo-compare see: > > 1241@1497507100.108799:colo_compare_pkt_info_src src/dst: > 192.168.4.144 s: seq/ack=2181260317/2075177759 res=0 flags=18 > spkt_size: 95 > > 1241@1497507100.108803:colo_compare_pkt_info_dst src/dst: 192.168.4.88 > d: seq/ack=2181260317/2075177759 res=0 flags=18 dpkt_size: 95 > > colo-compare ppkt: 0000: 01 00 00 00 00 00 22 00 10 00 00 00 12 14 > fc 1d ......"......... > colo-compare ppkt: 0010: 1a 6b 52 54 00 12 34 56 08 00 45 00 00 45 > 12 61 .kRT..4V..E..E.a > colo-compare ppkt: 0020: 40 00 40 06 9e 19 c0 a8 04 90 c0 a8 04 58 > 04 d2 @.@..........X.. > colo-compare ppkt: 0030: ca 98 82 03 64 1d 7b b0 b3 1f 80 18 00 e3 > 8a 70 ....d.{........p > colo-compare ppkt: 0040: 00 00 01 01 08 0a 00 00 5b fa 00 4d f1 aa > 73 65 ........[..M..se > colo-compare ppkt: 0050: 72 76 65 72 20 67 65 74 20 69 74 20 34 36 > 38 rver get it 468 > colo-compare spkt: 0000: 01 00 00 00 00 00 22 00 10 00 00 00 12 14 > fc 1d ......"......... > colo-compare spkt: 0010: 1a 6b 52 54 00 12 34 56 08 00 45 00 00 45 > 12 61 .kRT..4V..E..E.a > colo-compare spkt: 0020: 40 00 40 06 9e 19 c0 a8 04 90 c0 a8 04 58 > 04 d2 @.@..........X.. > colo-compare spkt: 0030: ca 98 82 03 64 1d 7b b0 b3 1f 80 18 00 e3 > 8a 70 ....d.{........p > colo-compare spkt: 0040: 00 00 01 01 08 0a 00 00 5c 02 00 4d f1 aa > 73 65 ........\..M..se > colo-compare spkt: 0050: 72 76 65 72 20 67 65 74 20 69 74 20 34 36 > 38 rver get it 468 > > So, colo-compare need to know the vnet_hdr_len for skip this field, > then colo-compare > can parse the net packet correctly. > Back to this patch, how should I fix it? Let me explain a little bit more. What I mean is that we should care only about the vnet header len of netdev instead of guest. On RX direction (guest transmission). If you look at virtio_net_flush_tx(), before calling qemu_sendv_packet_async() where the packet can reach netfilers, virtio-net will copy only the parts that host is interested. So what we can see in e.g filter-mirror is netdev's vnet_hdr_len which is not guaranteed to be equal to guest hdr len. On TX direction, the packet will reach netfilter before it reaches virtio-net backend, so the vnet header is also netdev's vnet_hdr_len. Does this explain? Thanks > > Thanks > Zhang Chen > > >> >> I still think use nf->netdev->vnet_hdr_len is sufficient. >> >> Thanks >> >>> >>>> >>>> Thanks >>>> >>>> >>>> >>> >> >> >> >> . >> >
On 06/16/2017 10:17 AM, Jason Wang wrote: > > > On 2017年06月15日 16:10, Zhang Chen wrote: >> >> >> On 06/15/2017 12:31 PM, Jason Wang wrote: >>> >>> >>> On 2017年06月14日 16:04, Zhang Chen wrote: >>>> >>>> >>>> On 06/13/2017 05:14 PM, Jason Wang wrote: >>>>> >>>>> >>>>> On 2017年06月12日 17:27, Zhang Chen wrote: >>>>>>> >>>>>>>> + if (nf->direction == NET_FILTER_DIRECTION_RX || >>>>>>>> + nf->direction == NET_FILTER_DIRECTION_ALL) { >>>>>>>> + vnet_hdr_len = nf->netdev->vnet_hdr_len; >>>>>>> >>>>>>> This can only work if e.g virtio-net set its own vnet_hdr_len. >>>>>>> But looks like it use guest_hdr_len instead. >>>>>> >>>>>> I see in hw/net/virtio-net.c use the >>>>>> "qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len);" to >>>>>> set the nf->netdev->vnet_hdr_len, any case not include here? >>>>> >>>>> You mean: >>>>> >>>>> if (peer_has_vnet_hdr(n) && >>>>> qemu_has_vnet_hdr_len(nc->peer, n->guest_hdr_len)) { >>>>> qemu_set_vnet_hdr_len(nc->peer, n->guest_hdr_len); >>>>> n->host_hdr_len = n->guest_hdr_len; >>>>> } >>>>> >>>>> ? >>>>> >>>>> From the code, it only set peer's vnet header when peer supports >>>>> vnet_hdr up to n->guest_hdr_len. If peer can't, virtio-net will >>>>> strip the header. >>>> >>>> So, We should get the n->guest_hdr_len in another way? like set a >>>> new parameter in NetClientState? >>>> Any suggestion about it? >>>> >>>> Thanks >>>> Zhang Chen >>> >>> Rethink about this, a question is do we really care guest vnet >>> header len? During guest transmission, when packet reaches >>> netfilter. The vnet_header should have been stripped to >>> netdev->vnet_hdr_len. >> >> But we use filter-redirector send packet to colo-compare that get the >> packet with the vnet header. >> >> That is colo-compare see: >> >> 1241@1497507100.108799:colo_compare_pkt_info_src src/dst: >> 192.168.4.144 s: seq/ack=2181260317/2075177759 res=0 flags=18 >> spkt_size: 95 >> >> 1241@1497507100.108803:colo_compare_pkt_info_dst src/dst: >> 192.168.4.88 d: seq/ack=2181260317/2075177759 res=0 flags=18 >> dpkt_size: 95 >> >> colo-compare ppkt: 0000: 01 00 00 00 00 00 22 00 10 00 00 00 12 14 >> fc 1d ......"......... >> colo-compare ppkt: 0010: 1a 6b 52 54 00 12 34 56 08 00 45 00 00 45 >> 12 61 .kRT..4V..E..E.a >> colo-compare ppkt: 0020: 40 00 40 06 9e 19 c0 a8 04 90 c0 a8 04 58 >> 04 d2 @.@..........X.. >> colo-compare ppkt: 0030: ca 98 82 03 64 1d 7b b0 b3 1f 80 18 00 e3 >> 8a 70 ....d.{........p >> colo-compare ppkt: 0040: 00 00 01 01 08 0a 00 00 5b fa 00 4d f1 aa >> 73 65 ........[..M..se >> colo-compare ppkt: 0050: 72 76 65 72 20 67 65 74 20 69 74 20 34 36 >> 38 rver get it 468 >> colo-compare spkt: 0000: 01 00 00 00 00 00 22 00 10 00 00 00 12 14 >> fc 1d ......"......... >> colo-compare spkt: 0010: 1a 6b 52 54 00 12 34 56 08 00 45 00 00 45 >> 12 61 .kRT..4V..E..E.a >> colo-compare spkt: 0020: 40 00 40 06 9e 19 c0 a8 04 90 c0 a8 04 58 >> 04 d2 @.@..........X.. >> colo-compare spkt: 0030: ca 98 82 03 64 1d 7b b0 b3 1f 80 18 00 e3 >> 8a 70 ....d.{........p >> colo-compare spkt: 0040: 00 00 01 01 08 0a 00 00 5c 02 00 4d f1 aa >> 73 65 ........\..M..se >> colo-compare spkt: 0050: 72 76 65 72 20 67 65 74 20 69 74 20 34 36 >> 38 rver get it 468 >> >> So, colo-compare need to know the vnet_hdr_len for skip this field, >> then colo-compare >> can parse the net packet correctly. >> Back to this patch, how should I fix it? > > Let me explain a little bit more. What I mean is that we should care > only about the vnet header len of netdev instead of guest. > > On RX direction (guest transmission). If you look at > virtio_net_flush_tx(), before calling qemu_sendv_packet_async() where > the packet can reach netfilers, virtio-net will copy only the parts > that host is interested. So what we can see in e.g filter-mirror is > netdev's vnet_hdr_len which is not guaranteed to be equal to guest hdr > len. > If I understand correctly, should I copy the n->needs_vnet_hdr_swap flag to NetClientState ? Then, e.g filter-mirror can read the flag to decide the vnet_hdr_len value to send. > On TX direction, the packet will reach netfilter before it reaches > virtio-net backend, so the vnet header is also netdev's vnet_hdr_len. > > Does this explain? Make sense. Thanks Zhang Chen > > > Thanks > >> >> Thanks >> Zhang Chen >> >> >>> >>> I still think use nf->netdev->vnet_hdr_len is sufficient. >>> >>> Thanks >>> >>>> >>>>> >>>>> Thanks >>>>> >>>>> >>>>> >>>> >>> >>> >>> >>> . >>> >> > > > > . >
diff --git a/net/filter-mirror.c b/net/filter-mirror.c index 72fa7c2..50aa81b 100644 --- a/net/filter-mirror.c +++ b/net/filter-mirror.c @@ -38,15 +38,17 @@ typedef struct MirrorState { NetFilterState parent_obj; char *indev; char *outdev; + bool vnet_hdr; CharBackend chr_in; CharBackend chr_out; SocketReadState rs; } MirrorState; -static int filter_mirror_send(CharBackend *chr_out, +static int filter_mirror_send(MirrorState *s, const struct iovec *iov, int iovcnt) { + NetFilterState *nf = NETFILTER(s); int ret = 0; ssize_t size = 0; uint32_t len = 0; @@ -58,14 +60,43 @@ static int filter_mirror_send(CharBackend *chr_out, } len = htonl(size); - ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)&len, sizeof(len)); + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); if (ret != sizeof(len)) { goto err; } + if (s->vnet_hdr) { + /* + * If vnet_hdr = on, we send vnet header len to make other + * module(like colo-compare) know how to parse net + * packet correctly. + */ + ssize_t vnet_hdr_len; + + /* + * In anytime, nf->netdev and nf->netdev->peer both have a vnet_hdr_len, + * Here we just find out which is we need. When filter set RX or TX + * that the real vnet_hdr_len are different. + */ + if (nf->direction == NET_FILTER_DIRECTION_RX || + nf->direction == NET_FILTER_DIRECTION_ALL) { + vnet_hdr_len = nf->netdev->vnet_hdr_len; + } else if (nf->direction == NET_FILTER_DIRECTION_TX) { + vnet_hdr_len = nf->netdev->peer->vnet_hdr_len; + } else { + return 0; + } + + len = htonl(vnet_hdr_len); + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); + if (ret != sizeof(len)) { + goto err; + } + } + buf = g_malloc(size); iov_to_buf(iov, iovcnt, 0, buf, size); - ret = qemu_chr_fe_write_all(chr_out, (uint8_t *)buf, size); + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size); g_free(buf); if (ret != size) { goto err; @@ -141,7 +172,7 @@ static ssize_t filter_mirror_receive_iov(NetFilterState *nf, MirrorState *s = FILTER_MIRROR(nf); int ret; - ret = filter_mirror_send(&s->chr_out, iov, iovcnt); + ret = filter_mirror_send(s, iov, iovcnt); if (ret) { error_report("filter_mirror_send failed(%s)", strerror(-ret)); } @@ -164,7 +195,7 @@ static ssize_t filter_redirector_receive_iov(NetFilterState *nf, int ret; if (qemu_chr_fe_get_driver(&s->chr_out)) { - ret = filter_mirror_send(&s->chr_out, iov, iovcnt); + ret = filter_mirror_send(s, iov, iovcnt); if (ret) { error_report("filter_mirror_send failed(%s)", strerror(-ret)); } @@ -308,6 +339,13 @@ static char *filter_mirror_get_outdev(Object *obj, Error **errp) return g_strdup(s->outdev); } +static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp) +{ + MirrorState *s = FILTER_MIRROR(obj); + + return s->vnet_hdr; +} + static void filter_mirror_set_outdev(Object *obj, const char *value, Error **errp) { @@ -322,6 +360,15 @@ filter_mirror_set_outdev(Object *obj, const char *value, Error **errp) } } +static void filter_mirror_set_vnet_hdr(Object *obj, + bool value, + Error **errp) +{ + MirrorState *s = FILTER_MIRROR(obj); + + s->vnet_hdr = value; +} + static char *filter_redirector_get_outdev(Object *obj, Error **errp) { MirrorState *s = FILTER_REDIRECTOR(obj); @@ -340,8 +387,20 @@ filter_redirector_set_outdev(Object *obj, const char *value, Error **errp) static void filter_mirror_init(Object *obj) { + MirrorState *s = FILTER_MIRROR(obj); + object_property_add_str(obj, "outdev", filter_mirror_get_outdev, filter_mirror_set_outdev, NULL); + + /* + * The vnet_hdr is disabled by default, if you want to enable + * this option, you must enable all the option on related modules + * (like other filter or colo-compare). + */ + s->vnet_hdr = false; + object_property_add_bool(obj, "vnet_hdr_support", + filter_mirror_get_vnet_hdr, + filter_mirror_set_vnet_hdr, NULL); } static void filter_redirector_init(Object *obj) diff --git a/qemu-options.hx b/qemu-options.hx index 70c0ded..5c09fae 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4024,10 +4024,9 @@ queue @var{all|rx|tx} is an option that can be applied to any netfilter. @option{tx}: the filter is attached to the transmit queue of the netdev, where it will receive packets sent by the netdev. -@item -object filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid}[,queue=@var{all|rx|tx}] +@item -object filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support] -filter-mirror on netdev @var{netdevid},mirror net packet to chardev -@var{chardevid} +filter-mirror on netdev @var{netdevid},mirror net packet to chardev@var{chardevid}, if have the vnet_hdr_support flag, filter-mirror will mirror packet with vnet_hdr_len. @item -object filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid}, outdev=@var{chardevid}[,queue=@var{all|rx|tx}]
We add the vnet_hdr_support option for filter-mirror, default is disable. If you use virtio-net-pci net driver, please enable it. You can use it for example: -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0,vnet_hdr_support If have vnet_hdr_support flag, we will change the send packet format from struct {int size; const uint8_t buf[];} to {int size; int vnet_hdr_len; const uint8_t buf[];}. make other module(like colo-compare) know how to parse net packet correctly. Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> --- net/filter-mirror.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---- qemu-options.hx | 5 ++-- 2 files changed, 66 insertions(+), 8 deletions(-)