On Thu, Apr 13, 2023 at 12:35 PM Dumitru Ceara <[email protected]> wrote:
>
> On 4/13/23 21:21, Han Zhou wrote:
> > 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
>
> This sounds like a relevant bug fix. Should this get backported to
> stable branches too?
>
Sound good. I will do that.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
