[ovs-dev] [PATCH ovn] northd: set packet length in check_pkt_larger()

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Thu Jun 18 11:59:04 UTC 2020


> On 6/17/20 1:08 PM, Lorenzo Bianconi wrote:
> >> lorenzo.bianconi at 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 at 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 at openvswitch.org
> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>>
> >>>
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > 
> 


More information about the dev mailing list