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

Ilya Maximets i.maximets at ovn.org
Thu Jun 18 10:12:13 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?
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 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