Thanks, Daniele! Pushed to master with the following incremental. Backporting to branch-2.3 as well.
Jarno diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index eb88f10..c14d671 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -4829,11 +4829,10 @@ AT_CAPTURE_FILE([ofctl_monitor.log]) AT_CHECK([ovs-ofctl monitor br0 65534 --detach --no-chdir --pidfile 2> ofctl_monitor.log]) -for i in 1 ; do - ovs-appctl netdev-dummy/receive p1 '0060970769ea0000860580da86dd6000000000203afffe80000000000000020086fffe0580dafe80000000000000026097fffe0769ea870068bd00000000fe80000000000000026097fffe0769ea01010000860580da' -done -OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 1]) +ovs-appctl netdev-dummy/receive p1 '0060970769ea0000860580da86dd6000000000203afffe80000000000000020086fffe0580dafe80000000000000026097fffe0769ea870068bd00000000fe80000000000000026097fffe0769ea01010000860580da' + OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit]) + AT_CHECK([cat ofctl_monitor.log], [0], [dnl NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=86 in_port=1 (via no_match) data_len=86 (unbuffered) icmp6,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src=fe80::200:86ff:fe05:80da,ipv6_dst=fe80::260:97ff:fe07:69ea,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=fe80::260:97ff:fe07:69ea,nd_sll=00:00:86:05:80:da,nd_tll=00:00:00:00:00:00 On Jun 2, 2014, at 10:39 AM, Daniele Di Proietto <[email protected]> wrote: > This patch addresses two bugs related to ICMPv6(NDP) packets: > > - In miniflow_extract() push the words in the correct order > - In parse_icmpv6() use sizeof struct, not size of struct * > > match_wc_init() has been modified, to include the nd_target field > when the transport layer protocol is ICMPv6 > > A testcase has been added to detect the bugs. > > Signed-off-by: Daniele Di Proietto <[email protected]> > --- > I'm wondering if you guys agree with the change to match_wc_init(). > Not every ICMPv6 packet has a valid nd_target field, but the same reasoning > can > be applied to arp_sha and arp_tha fields. > > Thanks, > > Daniele > --- > lib/flow.c | 6 +++--- > lib/match.c | 1 + > tests/ofproto-dpif.at | 21 +++++++++++++++++++++ > 3 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/lib/flow.c b/lib/flow.c > index da4f79b..76cef66 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -277,7 +277,7 @@ parse_icmpv6(void **datap, size_t *sizep, const struct > icmp6_hdr *icmp, > (icmp->icmp6_type == ND_NEIGHBOR_SOLICIT || > icmp->icmp6_type == ND_NEIGHBOR_ADVERT)) { > > - *nd_target = data_try_pull(datap, sizep, sizeof *nd_target); > + *nd_target = data_try_pull(datap, sizep, sizeof **nd_target); > if (OVS_UNLIKELY(!*nd_target)) { > return false; > } > @@ -607,12 +607,12 @@ miniflow_extract(struct ofpbuf *packet, const struct > pkt_metadata *md, > memset(arp_buf, 0, sizeof arp_buf); > if (OVS_LIKELY(parse_icmpv6(&data, &size, icmp, &nd_target, > arp_buf))) { > + miniflow_push_words(mf, arp_sha, arp_buf, > + ETH_ADDR_LEN * 2 / 4); > if (nd_target) { > miniflow_push_words(mf, nd_target, nd_target, > sizeof *nd_target / 4); > } > - miniflow_push_words(mf, arp_sha, arp_buf, > - ETH_ADDR_LEN * 2 / 4); I would have preferred changing the order of fields in struct flow instead, if possible. But that can be done later, if we still care... > miniflow_push_be16(mf, tp_src, htons(icmp->icmp6_type)); > miniflow_push_be16(mf, tp_dst, htons(icmp->icmp6_code)); > } > diff --git a/lib/match.c b/lib/match.c > index 308f906..93b725a 100644 > --- a/lib/match.c > +++ b/lib/match.c > @@ -136,6 +136,7 @@ match_wc_init(struct match *match, const struct flow > *flow) > if (flow->nw_proto == IPPROTO_ICMPV6) { > memset(&wc->masks.arp_sha, 0xff, sizeof wc->masks.arp_sha); > memset(&wc->masks.arp_tha, 0xff, sizeof wc->masks.arp_tha); > + memset(&wc->masks.nd_target, 0xff, sizeof wc->masks.nd_target); It seems that for the message types we are interested, we always expect to have the nd_target value, so this makes sense. As for the MAC addresses above, this will be reported as zeroes for ICMPv6 message types that do not have this/these fields. > } > } > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index cbb8ff8..d1fe734 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -4828,3 +4828,24 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 dump-tables br0 ], > [0], [expout]) > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto-dpif - ICMPv6]) > +OVS_VSWITCHD_START > +ADD_OF_PORTS([br0], 1) > + > +AT_CAPTURE_FILE([ofctl_monitor.log]) > + > +AT_CHECK([ovs-ofctl monitor br0 65534 --detach --no-chdir --pidfile 2> > ofctl_monitor.log]) > + > +for i in 1 ; do The for loop could be dropped. > + ovs-appctl netdev-dummy/receive p1 > '0060970769ea0000860580da86dd6000000000203afffe80000000000000020086fffe0580dafe80000000000000026097fffe0769ea870068bd00000000fe80000000000000026097fffe0769ea01010000860580da' > +done > +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 1]) > +OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit]) > +AT_CHECK([cat ofctl_monitor.log], [0], [dnl > +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=86 in_port=1 (via no_match) > data_len=86 (unbuffered) > +icmp6,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:00:86:05:80:da,dl_dst=00:60:97:07:69:ea,ipv6_src=fe80::200:86ff:fe05:80da,ipv6_dst=fe80::260:97ff:fe07:69ea,ipv6_label=0x00000,nw_tos=0,nw_ecn=0,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=fe80::260:97ff:fe07:69ea,nd_sll=00:00:86:05:80:da,nd_tll=00:00:00:00:00:00 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.0.0.rc2 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
