On Thu, Apr 13, 2023 at 8:37 AM Dumitru Ceara <[email protected]> wrote: > > 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]>
Thanks Dumitru. I applied the series to main. Han > > >> > >> 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
