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!
So I believe this is the reason behind the "if" statement in the RFC. Maybe
I should add this rationale in the comment, too.
>
> 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,
Han
>
> Thanks,
> Dumitru
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
