On 4/13/23 17:34, Han Zhou wrote: > On Thu, Apr 13, 2023 at 12:54 AM Dumitru Ceara <[email protected]> wrote: >> >> On 4/13/23 07:07, Han Zhou wrote: >>> In RFC1812 section 5.3.1, it is mentioned that: >>> >>> If the TTL is reduced to zero (or less), the packet MUST be >>> discarded, and if the destination is not a multicast address the >>> router MUST send an ICMP Time Exceeded message ... >>> >> >> The code itself looks OK but I wonder a bit about the rationale. Do you >> have an example in which OVN replies with Time Exceeded for multicast >> destinations and that causes issues? >> >>> So if the destionation is a multicast address the route shouldn't send >>> ICMP Time Exceeded, but the current OVN implementation didn't check >>> multicast and tries to send ICMP regardless. This patch fixes it. >> >> The statement "if destination is not a multicast address the router MUST >> send an ICMP Time Exceeded message" implies that "if destination is a >> multicast address the router MAY or MAY NOT send an ICMP Time Exceeded >> message". So the fact that OVN sends one is not necessarily wrong. > > For my limited understanding of multicast, sending ICMP time exceeded is > not a good idea. In multicast TTL has special meanings, for example: > TTL 0: Restricted to the same host, not transmitted by the router. > TTL 1: Restricted to the same subnet, not forwarded by the router. > If we send ICMP for such packets, it means for a very common use case of > multicast (ttl = 1, same subnet), we will end up sending ICMP for every > normal packet. > In production we saw this happening with a rate higher than 10k pps! >
Makes sense, thanks for the details! > So I believe this is the reason behind the "if" statement in the RFC. Maybe > I should add this rationale in the comment, too. > If you could add some of the details above to the commit log too then: Acked-by: Dumitru Ceara <[email protected]> >> >> I think I'd like to better understand the use case that's broken by the >> OVN behavior before accepting this change. >> >>> >>> Signed-off-by: Han Zhou <[email protected]> >>> --- >>> northd/northd.c | 10 ++++++++-- >>> northd/ovn-northd.8.xml | 10 +++++++++- >>> tests/ovn-northd.at | 9 +++++---- >>> tests/ovn.at | 37 ++++++++++++++++++++++++++++--------- >>> 4 files changed, 50 insertions(+), 16 deletions(-) >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index c4cb7232e0a1..cedddbc99d2c 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -13107,6 +13107,12 @@ > build_misc_local_traffic_drop_flows_for_lrouter( >>> ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 50, >>> "eth.bcast", debug_drop_action()); >>> >>> + /* Avoid ICMP time exceeded for multicast, silent drop instead. >>> + * (priority-31 flows will send ICMP time exceeded) */ >> >> If we go ahead with this patch, can you please add in the comment a >> reference to RFC1812 section 5.3.1? > > Sure. > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
