| Message ID | 35b6cdf4751c4ac5aebd2f018c859b794997970f.1592387613.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] northd: set packet length in check_pkt_larger() | expand |
On Wed, Jun 17, 2020 at 3:26 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > Set packet length in lr_in_chk_pkt_len router pipeline instead of > gw interface MTU since ovs kernel datapath usually works on L2 frames > > Fixes: 7d42c146be ("ovn: Generate ICMPv4 packet in router pipeline for > larger packets") > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > northd/ovn-northd.c | 4 ++-- > tests/ovn.at | 33 ++++++++++++++++++--------------- > 2 files changed, 20 insertions(+), 17 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index b8c9e9325..53d5bf245 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -10076,7 +10076,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > ds_clear(&actions); > ds_put_format(&actions, > REGBIT_PKT_LARGER" = check_pkt_larger(%d);" > - " next;", gw_mtu); > + " next;", gw_mtu + 18); > Hi Lorenzo, Thanks for the fix. Can you please add the comment why 18. May be a macro instead of "18" number. What would be the case if there is a VLAN tag set ? Thanks Numan > ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_CHK_PKT_LEN, > 50, > ds_cstr(&match), ds_cstr(&actions), > &od->l3dgw_port->nbrp->header_); > @@ -10109,7 +10109,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > "next(pipeline=ingress, table=0); };", > rp->lrp_networks.ea_s, > rp->lrp_networks.ipv4_addrs[0].addr_s, > - gw_mtu - 18); > + gw_mtu); > ovn_lflow_add_with_hint(lflows, od, > S_ROUTER_IN_LARGER_PKTS, > 50, ds_cstr(&match), > ds_cstr(&actions), > &rp->nbrp->header_); > diff --git a/tests/ovn.at b/tests/ovn.at > index 7e1ace556..1801a3151 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -14801,14 +14801,16 @@ test_ip_packet_larger() { > dst_mac="00000000ff01" # sw0-lr0 mac (internal router leg) > src_ip=`ip_to_hex 10 0 0 3` > dst_ip=`ip_to_hex 172 168 0 3` > - # Set the packet length to 100. > - pkt_len=0064 > - packet=${dst_mac}${src_mac}08004500${pkt_len}0000000040010000 > + # Set the packet length to 118. > + pkt_len=0076 > + packet=${dst_mac}${src_mac}08004500${pkt_len}000000004001c3d9 > orig_packet_l3=${src_ip}${dst_ip}0304000000000000 > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > + orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > + > packet=${packet}${orig_packet_l3} > > > gw_ip_garp=ffffffffffff00002020121308060001080006040001000020201213aca80064000000000000aca80064 > @@ -14818,32 +14820,33 @@ test_ip_packet_larger() { > # localnet port. > # If icmp_pmtu_reply_expected is 1, it means the packet is larger than > # the gateway mtu and ovn-controller should drop the packet and > instead > - # generate ICMPv4 Destination Unreachable message with pmtu set to > 42. > + # generate ICMPv4 Destination Unreachable message with pmtu set to > 100. > if test $icmp_pmtu_reply_expected = 0; then > # Packet to expect at br-phys. > src_mac="000020201213" > dst_mac="00000012af11" > src_ip=`ip_to_hex 10 0 0 3` > dst_ip=`ip_to_hex 172 168 0 3` > - expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f010100 > + expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f01c4d9 > expected=${expected}${src_ip}${dst_ip}0304000000000000 > expected=${expected}000000000000000000000000000000000000 > expected=${expected}000000000000000000000000000000000000 > expected=${expected}000000000000000000000000000000000000 > expected=${expected}000000000000000000000000000000000000 > + expected=${expected}000000000000000000000000000000000000 > echo $expected > br_phys_n1.expected > echo $gw_ip_garp >> br_phys_n1.expected > else > - # MTU would be 100 - 18 = 82 (hex 0052) > - mtu=0052 > + # MTU would be 118 - 18 = 100 (hex 0064) > + mtu=0064 > src_ip=`ip_to_hex 10 0 0 1` > dst_ip=`ip_to_hex 10 0 0 3` > - # pkt len should be 128 (28 (icmp packet) + 100 (orig ip + > payload)) > - reply_pkt_len=0080 > - ip_csum=bd91 > - > icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016879 > + # pkt len should be 146 (28 (icmp packet) + 118 (orig ip + > payload)) > + reply_pkt_len=0092 > + ip_csum=f993 > + > icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016867 > icmp_reply=${icmp_reply}${src_ip}${dst_ip}0304${ip_csum}0000${mtu} > - icmp_reply=${icmp_reply}4500${pkt_len}000000003f010100 > + icmp_reply=${icmp_reply}4500${pkt_len}000000003f01c4d9 > icmp_reply=${icmp_reply}${orig_packet_l3} > echo $icmp_reply > hv1-vif1.expected > fi > @@ -14883,12 +14886,12 @@ awk '{print $3}') > ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \ > logical_port=lr0-public mac="00\:00\:00\:12\:af\:11" > > -# Set the gateway mtu to 100. If the packet length is > 100, > ovn-controller > +# Set the gateway mtu to 100. If the packet length is > 118, > ovn-controller > # should send icmp host not reachable with pmtu set to 100. > ovn-nbctl --wait=hv set logical_router_port lr0-public > options:gateway_mtu=100 > as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply > OVS_WAIT_UNTIL([ > - test `as hv1 ovs-ofctl dump-flows br-int | grep > "check_pkt_larger(100)" | \ > + test `as hv1 ovs-ofctl dump-flows br-int | grep > "check_pkt_larger(118)" | \ > wc -l` -eq 1 > ]) > > @@ -14899,7 +14902,7 @@ test_ip_packet_larger $icmp_reply_expected > ovn-nbctl --wait=hv set logical_router_port lr0-public > options:gateway_mtu=500 > as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply > OVS_WAIT_UNTIL([ > - test `as hv1 ovs-ofctl dump-flows br-int | grep > "check_pkt_larger(500)" | \ > + test `as hv1 ovs-ofctl dump-flows br-int | grep > "check_pkt_larger(518)" | \ > wc -l` -eq 1 > ]) > > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
> lorenzo.bianconi@redhat.com> wrote: > > > Set packet length in lr_in_chk_pkt_len router pipeline instead of > > gw interface MTU since ovs kernel datapath usually works on L2 frames > > > > Fixes: 7d42c146be ("ovn: Generate ICMPv4 packet in router pipeline for > > larger packets") > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > northd/ovn-northd.c | 4 ++-- > > tests/ovn.at | 33 ++++++++++++++++++--------------- > > 2 files changed, 20 insertions(+), 17 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index b8c9e9325..53d5bf245 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -10076,7 +10076,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > > hmap *ports, > > ds_clear(&actions); > > ds_put_format(&actions, > > REGBIT_PKT_LARGER" = check_pkt_larger(%d);" > > - " next;", gw_mtu); > > + " next;", gw_mtu + 18); > > > > Hi Lorenzo, > > Thanks for the fix. Can you please add the comment why 18. May be a macro > instead of "18" number. Hi Numan, ack, will do in v2 > > What would be the case if there is a VLAN tag set ? I am not expert about how ovs manages vlan tags, but looking at [1] and [2], it seems to me vlan header is not accounted in skb->len Regards, Lorenzo [1] https://github.com/torvalds/linux/blob/master/net/core/dev.c#L5117 [2] https://github.com/torvalds/linux/blob/master/net/openvswitch/vport-netdev.c#L29 > > Thanks > Numan > > > > > > ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_CHK_PKT_LEN, > > 50, > > ds_cstr(&match), ds_cstr(&actions), > > &od->l3dgw_port->nbrp->header_); > > @@ -10109,7 +10109,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > > hmap *ports, > > "next(pipeline=ingress, table=0); };", > > rp->lrp_networks.ea_s, > > rp->lrp_networks.ipv4_addrs[0].addr_s, > > - gw_mtu - 18); > > + gw_mtu); > > ovn_lflow_add_with_hint(lflows, od, > > S_ROUTER_IN_LARGER_PKTS, > > 50, ds_cstr(&match), > > ds_cstr(&actions), > > &rp->nbrp->header_); > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 7e1ace556..1801a3151 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -14801,14 +14801,16 @@ test_ip_packet_larger() { > > dst_mac="00000000ff01" # sw0-lr0 mac (internal router leg) > > src_ip=`ip_to_hex 10 0 0 3` > > dst_ip=`ip_to_hex 172 168 0 3` > > - # Set the packet length to 100. > > - pkt_len=0064 > > - packet=${dst_mac}${src_mac}08004500${pkt_len}0000000040010000 > > + # Set the packet length to 118. > > + pkt_len=0076 > > + packet=${dst_mac}${src_mac}08004500${pkt_len}000000004001c3d9 > > orig_packet_l3=${src_ip}${dst_ip}0304000000000000 > > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > > orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > > + orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > > + > > packet=${packet}${orig_packet_l3} > > > > > > gw_ip_garp=ffffffffffff00002020121308060001080006040001000020201213aca80064000000000000aca80064 > > @@ -14818,32 +14820,33 @@ test_ip_packet_larger() { > > # localnet port. > > # If icmp_pmtu_reply_expected is 1, it means the packet is larger than > > # the gateway mtu and ovn-controller should drop the packet and > > instead > > - # generate ICMPv4 Destination Unreachable message with pmtu set to > > 42. > > + # generate ICMPv4 Destination Unreachable message with pmtu set to > > 100. > > if test $icmp_pmtu_reply_expected = 0; then > > # Packet to expect at br-phys. > > src_mac="000020201213" > > dst_mac="00000012af11" > > src_ip=`ip_to_hex 10 0 0 3` > > dst_ip=`ip_to_hex 172 168 0 3` > > - expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f010100 > > + expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f01c4d9 > > expected=${expected}${src_ip}${dst_ip}0304000000000000 > > expected=${expected}000000000000000000000000000000000000 > > expected=${expected}000000000000000000000000000000000000 > > expected=${expected}000000000000000000000000000000000000 > > expected=${expected}000000000000000000000000000000000000 > > + expected=${expected}000000000000000000000000000000000000 > > echo $expected > br_phys_n1.expected > > echo $gw_ip_garp >> br_phys_n1.expected > > else > > - # MTU would be 100 - 18 = 82 (hex 0052) > > - mtu=0052 > > + # MTU would be 118 - 18 = 100 (hex 0064) > > + mtu=0064 > > src_ip=`ip_to_hex 10 0 0 1` > > dst_ip=`ip_to_hex 10 0 0 3` > > - # pkt len should be 128 (28 (icmp packet) + 100 (orig ip + > > payload)) > > - reply_pkt_len=0080 > > - ip_csum=bd91 > > - > > icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016879 > > + # pkt len should be 146 (28 (icmp packet) + 118 (orig ip + > > payload)) > > + reply_pkt_len=0092 > > + ip_csum=f993 > > + > > icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016867 > > icmp_reply=${icmp_reply}${src_ip}${dst_ip}0304${ip_csum}0000${mtu} > > - icmp_reply=${icmp_reply}4500${pkt_len}000000003f010100 > > + icmp_reply=${icmp_reply}4500${pkt_len}000000003f01c4d9 > > icmp_reply=${icmp_reply}${orig_packet_l3} > > echo $icmp_reply > hv1-vif1.expected > > fi > > @@ -14883,12 +14886,12 @@ awk '{print $3}') > > ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \ > > logical_port=lr0-public mac="00\:00\:00\:12\:af\:11" > > > > -# Set the gateway mtu to 100. If the packet length is > 100, > > ovn-controller > > +# Set the gateway mtu to 100. If the packet length is > 118, > > ovn-controller > > # should send icmp host not reachable with pmtu set to 100. > > ovn-nbctl --wait=hv set logical_router_port lr0-public > > options:gateway_mtu=100 > > as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply > > OVS_WAIT_UNTIL([ > > - test `as hv1 ovs-ofctl dump-flows br-int | grep > > "check_pkt_larger(100)" | \ > > + test `as hv1 ovs-ofctl dump-flows br-int | grep > > "check_pkt_larger(118)" | \ > > wc -l` -eq 1 > > ]) > > > > @@ -14899,7 +14902,7 @@ test_ip_packet_larger $icmp_reply_expected > > ovn-nbctl --wait=hv set logical_router_port lr0-public > > options:gateway_mtu=500 > > as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply > > OVS_WAIT_UNTIL([ > > - test `as hv1 ovs-ofctl dump-flows br-int | grep > > "check_pkt_larger(500)" | \ > > + test `as hv1 ovs-ofctl dump-flows br-int | grep > > "check_pkt_larger(518)" | \ > > wc -l` -eq 1 > > ]) > > > > -- > > 2.26.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
On 17 Jun, Numan Siddique wrote: > On Wed, Jun 17, 2020 at 3:26 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > Set packet length in lr_in_chk_pkt_len router pipeline instead of > > gw interface MTU since ovs kernel datapath usually works on L2 frames > > > > Fixes: 7d42c146be ("ovn: Generate ICMPv4 packet in router pipeline for > > larger packets") > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > northd/ovn-northd.c | 4 ++-- > > tests/ovn.at | 33 ++++++++++++++++++--------------- > > 2 files changed, 20 insertions(+), 17 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index b8c9e9325..53d5bf245 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -10076,7 +10076,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > > hmap *ports, > > ds_clear(&actions); > > ds_put_format(&actions, > > REGBIT_PKT_LARGER" = check_pkt_larger(%d);" > > - " next;", gw_mtu); > > + " next;", gw_mtu + 18); > > > > Hi Lorenzo, > > Thanks for the fix. Can you please add the comment why 18. May be a macro > instead of "18" number. > Maybe it's also worth modifying the comment here https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L10103
On Thu, Jun 18, 2020 at 2:00 PM Gabriele Cerami <gcerami@redhat.com> wrote: > On 17 Jun, Numan Siddique wrote: > > On Wed, Jun 17, 2020 at 3:26 PM Lorenzo Bianconi < > > lorenzo.bianconi@redhat.com> wrote: > > > > > Set packet length in lr_in_chk_pkt_len router pipeline instead of > > > gw interface MTU since ovs kernel datapath usually works on L2 frames > > > > > > Fixes: 7d42c146be ("ovn: Generate ICMPv4 packet in router pipeline for > > > larger packets") > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > --- > > > northd/ovn-northd.c | 4 ++-- > > > tests/ovn.at | 33 ++++++++++++++++++--------------- > > > 2 files changed, 20 insertions(+), 17 deletions(-) > > > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > > index b8c9e9325..53d5bf245 100644 > > > --- a/northd/ovn-northd.c > > > +++ b/northd/ovn-northd.c > > > @@ -10076,7 +10076,7 @@ build_lrouter_flows(struct hmap *datapaths, > struct > > > hmap *ports, > > > ds_clear(&actions); > > > ds_put_format(&actions, > > > REGBIT_PKT_LARGER" = check_pkt_larger(%d);" > > > - " next;", gw_mtu); > > > + " next;", gw_mtu + 18); > > > > > > > Hi Lorenzo, > > > > Thanks for the fix. Can you please add the comment why 18. May be a macro > > instead of "18" number. > > > > Maybe it's also worth modifying the comment here > https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L10103 +1. Thanks Numan > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 17 Jun, Lorenzo Bianconi wrote: > > What would be the case if there is a VLAN tag set ? > > I am not expert about how ovs manages vlan tags, but looking at [1] and [2], it seems > to me vlan header is not accounted in skb->len > > Regards, > Lorenzo > > [1] https://github.com/torvalds/linux/blob/master/net/core/dev.c#L5117 > [2] https://github.com/torvalds/linux/blob/master/net/openvswitch/vport-netdev.c#L29 I don't have the full picture of what happens either, but it would seem the vlan header is accounted From here: https://github.com/torvalds/linux/blob/master/include/linux/if_vlan.h#L342 skb_push resizes the skb adding VLAN_HLEN bytes, modifying skb->len
On 18 Jun, Gabriele Cerami wrote: > On 17 Jun, Lorenzo Bianconi wrote: > > > What would be the case if there is a VLAN tag set ? > > > > I am not expert about how ovs manages vlan tags, but looking at [1] and [2], it seems > > to me vlan header is not accounted in skb->len > > > > Regards, > > Lorenzo > > > > [1] https://github.com/torvalds/linux/blob/master/net/core/dev.c#L5117 > > [2] https://github.com/torvalds/linux/blob/master/net/openvswitch/vport-netdev.c#L29 > > I don't have the full picture of what happens either, but it would seem > the vlan header is accounted Nevermind, there's really a lot more into it, indeed I don't have the full picture of the flow.
On 6/17/20 1:08 PM, Lorenzo Bianconi wrote: >> lorenzo.bianconi@redhat.com> wrote: >> >>> Set packet length in lr_in_chk_pkt_len router pipeline instead of >>> gw interface MTU since ovs kernel datapath usually works on L2 frames >>> >>> Fixes: 7d42c146be ("ovn: Generate ICMPv4 packet in router pipeline for >>> larger packets") >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >>> --- >>> northd/ovn-northd.c | 4 ++-- >>> tests/ovn.at | 33 ++++++++++++++++++--------------- >>> 2 files changed, 20 insertions(+), 17 deletions(-) >>> >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>> index b8c9e9325..53d5bf245 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -10076,7 +10076,7 @@ build_lrouter_flows(struct hmap *datapaths, struct >>> hmap *ports, >>> ds_clear(&actions); >>> ds_put_format(&actions, >>> REGBIT_PKT_LARGER" = check_pkt_larger(%d);" >>> - " next;", gw_mtu); >>> + " next;", gw_mtu + 18); >>> >> >> Hi Lorenzo, >> >> Thanks for the fix. Can you please add the comment why 18. May be a macro >> instead of "18" number. > > Hi Numan, > > ack, will do in v2 > >> >> What would be the case if there is a VLAN tag set ? > > I am not expert about how ovs manages vlan tags, but looking at [1] and [2], it seems > to me vlan header is not accounted in skb->len Anyway, 18 seems to be an ETH_HEADER_LEN (14) + VLAN_HEADER_LEN (4). Isn't it? I don't know how this supposed to work with double tagged packets, though. > > Regards, > Lorenzo > > [1] https://github.com/torvalds/linux/blob/master/net/core/dev.c#L5117 > [2] https://github.com/torvalds/linux/blob/master/net/openvswitch/vport-netdev.c#L29 > >> >> Thanks >> Numan >> >> >> >> >>> ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_CHK_PKT_LEN, >>> 50, >>> ds_cstr(&match), ds_cstr(&actions), >>> &od->l3dgw_port->nbrp->header_); >>> @@ -10109,7 +10109,7 @@ build_lrouter_flows(struct hmap *datapaths, struct >>> hmap *ports, >>> "next(pipeline=ingress, table=0); };", >>> rp->lrp_networks.ea_s, >>> rp->lrp_networks.ipv4_addrs[0].addr_s, >>> - gw_mtu - 18); >>> + gw_mtu); >>> ovn_lflow_add_with_hint(lflows, od, >>> S_ROUTER_IN_LARGER_PKTS, >>> 50, ds_cstr(&match), >>> ds_cstr(&actions), >>> &rp->nbrp->header_); >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index 7e1ace556..1801a3151 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -14801,14 +14801,16 @@ test_ip_packet_larger() { >>> dst_mac="00000000ff01" # sw0-lr0 mac (internal router leg) >>> src_ip=`ip_to_hex 10 0 0 3` >>> dst_ip=`ip_to_hex 172 168 0 3` >>> - # Set the packet length to 100. >>> - pkt_len=0064 >>> - packet=${dst_mac}${src_mac}08004500${pkt_len}0000000040010000 >>> + # Set the packet length to 118. >>> + pkt_len=0076 >>> + packet=${dst_mac}${src_mac}08004500${pkt_len}000000004001c3d9 >>> orig_packet_l3=${src_ip}${dst_ip}0304000000000000 >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 >>> + orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 >>> + >>> packet=${packet}${orig_packet_l3} >>> >>> >>> gw_ip_garp=ffffffffffff00002020121308060001080006040001000020201213aca80064000000000000aca80064 >>> @@ -14818,32 +14820,33 @@ test_ip_packet_larger() { >>> # localnet port. >>> # If icmp_pmtu_reply_expected is 1, it means the packet is larger than >>> # the gateway mtu and ovn-controller should drop the packet and >>> instead >>> - # generate ICMPv4 Destination Unreachable message with pmtu set to >>> 42. >>> + # generate ICMPv4 Destination Unreachable message with pmtu set to >>> 100. >>> if test $icmp_pmtu_reply_expected = 0; then >>> # Packet to expect at br-phys. >>> src_mac="000020201213" >>> dst_mac="00000012af11" >>> src_ip=`ip_to_hex 10 0 0 3` >>> dst_ip=`ip_to_hex 172 168 0 3` >>> - expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f010100 >>> + expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f01c4d9 >>> expected=${expected}${src_ip}${dst_ip}0304000000000000 >>> expected=${expected}000000000000000000000000000000000000 >>> expected=${expected}000000000000000000000000000000000000 >>> expected=${expected}000000000000000000000000000000000000 >>> expected=${expected}000000000000000000000000000000000000 >>> + expected=${expected}000000000000000000000000000000000000 >>> echo $expected > br_phys_n1.expected >>> echo $gw_ip_garp >> br_phys_n1.expected >>> else >>> - # MTU would be 100 - 18 = 82 (hex 0052) >>> - mtu=0052 >>> + # MTU would be 118 - 18 = 100 (hex 0064) >>> + mtu=0064 >>> src_ip=`ip_to_hex 10 0 0 1` >>> dst_ip=`ip_to_hex 10 0 0 3` >>> - # pkt len should be 128 (28 (icmp packet) + 100 (orig ip + >>> payload)) >>> - reply_pkt_len=0080 >>> - ip_csum=bd91 >>> - >>> icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016879 >>> + # pkt len should be 146 (28 (icmp packet) + 118 (orig ip + >>> payload)) >>> + reply_pkt_len=0092 >>> + ip_csum=f993 >>> + >>> icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016867 >>> icmp_reply=${icmp_reply}${src_ip}${dst_ip}0304${ip_csum}0000${mtu} >>> - icmp_reply=${icmp_reply}4500${pkt_len}000000003f010100 >>> + icmp_reply=${icmp_reply}4500${pkt_len}000000003f01c4d9 >>> icmp_reply=${icmp_reply}${orig_packet_l3} >>> echo $icmp_reply > hv1-vif1.expected >>> fi >>> @@ -14883,12 +14886,12 @@ awk '{print $3}') >>> ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \ >>> logical_port=lr0-public mac="00\:00\:00\:12\:af\:11" >>> >>> -# Set the gateway mtu to 100. If the packet length is > 100, >>> ovn-controller >>> +# Set the gateway mtu to 100. If the packet length is > 118, >>> ovn-controller >>> # should send icmp host not reachable with pmtu set to 100. >>> ovn-nbctl --wait=hv set logical_router_port lr0-public >>> options:gateway_mtu=100 >>> as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply >>> OVS_WAIT_UNTIL([ >>> - test `as hv1 ovs-ofctl dump-flows br-int | grep >>> "check_pkt_larger(100)" | \ >>> + test `as hv1 ovs-ofctl dump-flows br-int | grep >>> "check_pkt_larger(118)" | \ >>> wc -l` -eq 1 >>> ]) >>> >>> @@ -14899,7 +14902,7 @@ test_ip_packet_larger $icmp_reply_expected >>> ovn-nbctl --wait=hv set logical_router_port lr0-public >>> options:gateway_mtu=500 >>> as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply >>> OVS_WAIT_UNTIL([ >>> - test `as hv1 ovs-ofctl dump-flows br-int | grep >>> "check_pkt_larger(500)" | \ >>> + test `as hv1 ovs-ofctl dump-flows br-int | grep >>> "check_pkt_larger(518)" | \ >>> wc -l` -eq 1 >>> ]) >>> >>> -- >>> 2.26.2 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On 6/17/20 1:08 PM, Lorenzo Bianconi wrote: > >> lorenzo.bianconi@redhat.com> wrote: > >> > >>> Set packet length in lr_in_chk_pkt_len router pipeline instead of > >>> gw interface MTU since ovs kernel datapath usually works on L2 frames > >>> > >>> Fixes: 7d42c146be ("ovn: Generate ICMPv4 packet in router pipeline for > >>> larger packets") > >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > >>> --- > >>> northd/ovn-northd.c | 4 ++-- > >>> tests/ovn.at | 33 ++++++++++++++++++--------------- > >>> 2 files changed, 20 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > >>> index b8c9e9325..53d5bf245 100644 > >>> --- a/northd/ovn-northd.c > >>> +++ b/northd/ovn-northd.c > >>> @@ -10076,7 +10076,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > >>> hmap *ports, > >>> ds_clear(&actions); > >>> ds_put_format(&actions, > >>> REGBIT_PKT_LARGER" = check_pkt_larger(%d);" > >>> - " next;", gw_mtu); > >>> + " next;", gw_mtu + 18); > >>> > >> > >> Hi Lorenzo, > >> > >> Thanks for the fix. Can you please add the comment why 18. May be a macro > >> instead of "18" number. > > > > Hi Numan, > > > > ack, will do in v2 > > > >> > >> What would be the case if there is a VLAN tag set ? > > > > I am not expert about how ovs manages vlan tags, but looking at [1] and [2], it seems > > to me vlan header is not accounted in skb->len > > Anyway, 18 seems to be an ETH_HEADER_LEN (14) + VLAN_HEADER_LEN (4). Isn't it? Nope, I guess 18 is ETH_HEADER_LEN(14) + FCS(4) > I don't know how this supposed to work with double tagged packets, though. Looking at the code it seems even the inner vlan tag is not accounted in skb->len (parse_vlan in net/openvswitch/flow.c) but I can be wrong since I am not so familiar with this code. Anyway I guess this would be true even with the previous implementation since check_pkt_larger() will check packet length and not the MTU. This patch is actually an improvement in order to not consider the L2 header in the MTU (since the field is called "gateway_mtu") Agree? Regards, Lorenzo > > > > > Regards, > > Lorenzo > > > > [1] https://github.com/torvalds/linux/blob/master/net/core/dev.c#L5117 > > [2] https://github.com/torvalds/linux/blob/master/net/openvswitch/vport-netdev.c#L29 > > > >> > >> Thanks > >> Numan > >> > >> > >> > >> > >>> ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_CHK_PKT_LEN, > >>> 50, > >>> ds_cstr(&match), ds_cstr(&actions), > >>> &od->l3dgw_port->nbrp->header_); > >>> @@ -10109,7 +10109,7 @@ build_lrouter_flows(struct hmap *datapaths, struct > >>> hmap *ports, > >>> "next(pipeline=ingress, table=0); };", > >>> rp->lrp_networks.ea_s, > >>> rp->lrp_networks.ipv4_addrs[0].addr_s, > >>> - gw_mtu - 18); > >>> + gw_mtu); > >>> ovn_lflow_add_with_hint(lflows, od, > >>> S_ROUTER_IN_LARGER_PKTS, > >>> 50, ds_cstr(&match), > >>> ds_cstr(&actions), > >>> &rp->nbrp->header_); > >>> diff --git a/tests/ovn.at b/tests/ovn.at > >>> index 7e1ace556..1801a3151 100644 > >>> --- a/tests/ovn.at > >>> +++ b/tests/ovn.at > >>> @@ -14801,14 +14801,16 @@ test_ip_packet_larger() { > >>> dst_mac="00000000ff01" # sw0-lr0 mac (internal router leg) > >>> src_ip=`ip_to_hex 10 0 0 3` > >>> dst_ip=`ip_to_hex 172 168 0 3` > >>> - # Set the packet length to 100. > >>> - pkt_len=0064 > >>> - packet=${dst_mac}${src_mac}08004500${pkt_len}0000000040010000 > >>> + # Set the packet length to 118. > >>> + pkt_len=0076 > >>> + packet=${dst_mac}${src_mac}08004500${pkt_len}000000004001c3d9 > >>> orig_packet_l3=${src_ip}${dst_ip}0304000000000000 > >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > >>> orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > >>> + orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 > >>> + > >>> packet=${packet}${orig_packet_l3} > >>> > >>> > >>> gw_ip_garp=ffffffffffff00002020121308060001080006040001000020201213aca80064000000000000aca80064 > >>> @@ -14818,32 +14820,33 @@ test_ip_packet_larger() { > >>> # localnet port. > >>> # If icmp_pmtu_reply_expected is 1, it means the packet is larger than > >>> # the gateway mtu and ovn-controller should drop the packet and > >>> instead > >>> - # generate ICMPv4 Destination Unreachable message with pmtu set to > >>> 42. > >>> + # generate ICMPv4 Destination Unreachable message with pmtu set to > >>> 100. > >>> if test $icmp_pmtu_reply_expected = 0; then > >>> # Packet to expect at br-phys. > >>> src_mac="000020201213" > >>> dst_mac="00000012af11" > >>> src_ip=`ip_to_hex 10 0 0 3` > >>> dst_ip=`ip_to_hex 172 168 0 3` > >>> - expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f010100 > >>> + expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f01c4d9 > >>> expected=${expected}${src_ip}${dst_ip}0304000000000000 > >>> expected=${expected}000000000000000000000000000000000000 > >>> expected=${expected}000000000000000000000000000000000000 > >>> expected=${expected}000000000000000000000000000000000000 > >>> expected=${expected}000000000000000000000000000000000000 > >>> + expected=${expected}000000000000000000000000000000000000 > >>> echo $expected > br_phys_n1.expected > >>> echo $gw_ip_garp >> br_phys_n1.expected > >>> else > >>> - # MTU would be 100 - 18 = 82 (hex 0052) > >>> - mtu=0052 > >>> + # MTU would be 118 - 18 = 100 (hex 0064) > >>> + mtu=0064 > >>> src_ip=`ip_to_hex 10 0 0 1` > >>> dst_ip=`ip_to_hex 10 0 0 3` > >>> - # pkt len should be 128 (28 (icmp packet) + 100 (orig ip + > >>> payload)) > >>> - reply_pkt_len=0080 > >>> - ip_csum=bd91 > >>> - > >>> icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016879 > >>> + # pkt len should be 146 (28 (icmp packet) + 118 (orig ip + > >>> payload)) > >>> + reply_pkt_len=0092 > >>> + ip_csum=f993 > >>> + > >>> icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016867 > >>> icmp_reply=${icmp_reply}${src_ip}${dst_ip}0304${ip_csum}0000${mtu} > >>> - icmp_reply=${icmp_reply}4500${pkt_len}000000003f010100 > >>> + icmp_reply=${icmp_reply}4500${pkt_len}000000003f01c4d9 > >>> icmp_reply=${icmp_reply}${orig_packet_l3} > >>> echo $icmp_reply > hv1-vif1.expected > >>> fi > >>> @@ -14883,12 +14886,12 @@ awk '{print $3}') > >>> ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \ > >>> logical_port=lr0-public mac="00\:00\:00\:12\:af\:11" > >>> > >>> -# Set the gateway mtu to 100. If the packet length is > 100, > >>> ovn-controller > >>> +# Set the gateway mtu to 100. If the packet length is > 118, > >>> ovn-controller > >>> # should send icmp host not reachable with pmtu set to 100. > >>> ovn-nbctl --wait=hv set logical_router_port lr0-public > >>> options:gateway_mtu=100 > >>> as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply > >>> OVS_WAIT_UNTIL([ > >>> - test `as hv1 ovs-ofctl dump-flows br-int | grep > >>> "check_pkt_larger(100)" | \ > >>> + test `as hv1 ovs-ofctl dump-flows br-int | grep > >>> "check_pkt_larger(118)" | \ > >>> wc -l` -eq 1 > >>> ]) > >>> > >>> @@ -14899,7 +14902,7 @@ test_ip_packet_larger $icmp_reply_expected > >>> ovn-nbctl --wait=hv set logical_router_port lr0-public > >>> options:gateway_mtu=500 > >>> as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply > >>> OVS_WAIT_UNTIL([ > >>> - test `as hv1 ovs-ofctl dump-flows br-int | grep > >>> "check_pkt_larger(500)" | \ > >>> + test `as hv1 ovs-ofctl dump-flows br-int | grep > >>> "check_pkt_larger(518)" | \ > >>> wc -l` -eq 1 > >>> ]) > >>> > >>> -- > >>> 2.26.2 > >>> > >>> _______________________________________________ > >>> dev mailing list > >>> dev@openvswitch.org > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>> > >>> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index b8c9e9325..53d5bf245 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -10076,7 +10076,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ds_clear(&actions); ds_put_format(&actions, REGBIT_PKT_LARGER" = check_pkt_larger(%d);" - " next;", gw_mtu); + " next;", gw_mtu + 18); ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_CHK_PKT_LEN, 50, ds_cstr(&match), ds_cstr(&actions), &od->l3dgw_port->nbrp->header_); @@ -10109,7 +10109,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, "next(pipeline=ingress, table=0); };", rp->lrp_networks.ea_s, rp->lrp_networks.ipv4_addrs[0].addr_s, - gw_mtu - 18); + gw_mtu); ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_LARGER_PKTS, 50, ds_cstr(&match), ds_cstr(&actions), &rp->nbrp->header_); diff --git a/tests/ovn.at b/tests/ovn.at index 7e1ace556..1801a3151 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -14801,14 +14801,16 @@ test_ip_packet_larger() { dst_mac="00000000ff01" # sw0-lr0 mac (internal router leg) src_ip=`ip_to_hex 10 0 0 3` dst_ip=`ip_to_hex 172 168 0 3` - # Set the packet length to 100. - pkt_len=0064 - packet=${dst_mac}${src_mac}08004500${pkt_len}0000000040010000 + # Set the packet length to 118. + pkt_len=0076 + packet=${dst_mac}${src_mac}08004500${pkt_len}000000004001c3d9 orig_packet_l3=${src_ip}${dst_ip}0304000000000000 orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 + orig_packet_l3=${orig_packet_l3}000000000000000000000000000000000000 + packet=${packet}${orig_packet_l3} gw_ip_garp=ffffffffffff00002020121308060001080006040001000020201213aca80064000000000000aca80064 @@ -14818,32 +14820,33 @@ test_ip_packet_larger() { # localnet port. # If icmp_pmtu_reply_expected is 1, it means the packet is larger than # the gateway mtu and ovn-controller should drop the packet and instead - # generate ICMPv4 Destination Unreachable message with pmtu set to 42. + # generate ICMPv4 Destination Unreachable message with pmtu set to 100. if test $icmp_pmtu_reply_expected = 0; then # Packet to expect at br-phys. src_mac="000020201213" dst_mac="00000012af11" src_ip=`ip_to_hex 10 0 0 3` dst_ip=`ip_to_hex 172 168 0 3` - expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f010100 + expected=${dst_mac}${src_mac}08004500${pkt_len}000000003f01c4d9 expected=${expected}${src_ip}${dst_ip}0304000000000000 expected=${expected}000000000000000000000000000000000000 expected=${expected}000000000000000000000000000000000000 expected=${expected}000000000000000000000000000000000000 expected=${expected}000000000000000000000000000000000000 + expected=${expected}000000000000000000000000000000000000 echo $expected > br_phys_n1.expected echo $gw_ip_garp >> br_phys_n1.expected else - # MTU would be 100 - 18 = 82 (hex 0052) - mtu=0052 + # MTU would be 118 - 18 = 100 (hex 0064) + mtu=0064 src_ip=`ip_to_hex 10 0 0 1` dst_ip=`ip_to_hex 10 0 0 3` - # pkt len should be 128 (28 (icmp packet) + 100 (orig ip + payload)) - reply_pkt_len=0080 - ip_csum=bd91 - icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016879 + # pkt len should be 146 (28 (icmp packet) + 118 (orig ip + payload)) + reply_pkt_len=0092 + ip_csum=f993 + icmp_reply=${src_mac}${dst_mac}08004500${reply_pkt_len}00004000fe016867 icmp_reply=${icmp_reply}${src_ip}${dst_ip}0304${ip_csum}0000${mtu} - icmp_reply=${icmp_reply}4500${pkt_len}000000003f010100 + icmp_reply=${icmp_reply}4500${pkt_len}000000003f01c4d9 icmp_reply=${icmp_reply}${orig_packet_l3} echo $icmp_reply > hv1-vif1.expected fi @@ -14883,12 +14886,12 @@ awk '{print $3}') ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \ logical_port=lr0-public mac="00\:00\:00\:12\:af\:11" -# Set the gateway mtu to 100. If the packet length is > 100, ovn-controller +# Set the gateway mtu to 100. If the packet length is > 118, ovn-controller # should send icmp host not reachable with pmtu set to 100. ovn-nbctl --wait=hv set logical_router_port lr0-public options:gateway_mtu=100 as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply OVS_WAIT_UNTIL([ - test `as hv1 ovs-ofctl dump-flows br-int | grep "check_pkt_larger(100)" | \ + test `as hv1 ovs-ofctl dump-flows br-int | grep "check_pkt_larger(118)" | \ wc -l` -eq 1 ]) @@ -14899,7 +14902,7 @@ test_ip_packet_larger $icmp_reply_expected ovn-nbctl --wait=hv set logical_router_port lr0-public options:gateway_mtu=500 as hv3 ovs-appctl netdev-dummy/receive hv3-vif1 $arp_reply OVS_WAIT_UNTIL([ - test `as hv1 ovs-ofctl dump-flows br-int | grep "check_pkt_larger(500)" | \ + test `as hv1 ovs-ofctl dump-flows br-int | grep "check_pkt_larger(518)" | \ wc -l` -eq 1 ])
Set packet length in lr_in_chk_pkt_len router pipeline instead of gw interface MTU since ovs kernel datapath usually works on L2 frames Fixes: 7d42c146be ("ovn: Generate ICMPv4 packet in router pipeline for larger packets") Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/ovn-northd.c | 4 ++-- tests/ovn.at | 33 ++++++++++++++++++--------------- 2 files changed, 20 insertions(+), 17 deletions(-)