On Mon, Jan 22, 2024 at 4:10 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 1/22/24 21:33, Mike Pattrick wrote:
> > On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> >>
> >> On 1/22/24 18:51, Mike Pattrick wrote:
> >>> The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
> >>> a double encapsulated BFD packet would trigger a seg fault. This
> >>> happened because we had assumed that if IP checksumming was marked as
> >>> offloaded, that checksum would occur in the innermost packet.
> >>>
> >>> This change will check that the inner packet has an L3 and L4 before
> >>> checksumming those parts. And if missing, will resume checksumming the
> >>> outer components.
> >>
> >> Hrm. This looks like a workaround rather than a fix. If the inner packet
> >> doesn't have L3 header, dp-packet must not have a flag for L3 checksumming
> >> set in the first place. And if it does have inner L3, the offset must be
> >> initialized. I guess, some of the offsets can be not initialized, because
> >> the packet is never parsed by either miniflow_extract() or
> >> parse_tcp_flags().
> >> And the bfd_put_packet() doesn't seem to set them.
> >
> > I think you're right, I stepped through the problem in GDB and it was
> > clear that the flags weren't getting reset properly between BFD
> > packets. I'll send an updated patch.
>
> Yeah, flags preservation is one thing, the other is that l3/l4_ofs are just
> not populated in these packets, which doesn't sound right.
>
> It's also not clear why the packet is marked for inner checksum offload if
> the inner checksum is actually fully calculated and correct. I understand
> that the flags are carried over from a previous packet, but why did that
> previous packet have this flag set?
Here's an example:
monitor_run()
\_ dp_packet_use_stub(&packet, stub, sizeof stub); <--- initializes packet once
\_ while(!heap_is_empty(&monitor_heap) <-- loop where the same packet
can be reused
\_ monitor_mport_run()
\_ dp_packet_clear() <- Data cleared, but flags are not cleared
\_ cfm_compose_ccm() / bfd_put_packet() / lldp_put_packet() <-
one or more can run
\_ Note that cfm_compose_ccm and lldp_put_packet call
eth_compose, but bfd_put_packet doesn't. bfd_put_packet doesn't reset
the offsets like eth_compose does.
\_ ofproto_dpif_send_packet()
.... non-relevant stack trace ...
\_ netdev_push_header()
\_ First run, push geneve header and toggle geneve flag
\_ Second run, detect geneve header flag, call dp_packet_ol_send_prepare()
Given the above, I think it makes sense to clear the offload flags in
dp_packet_clear().
-M
>
> >
> >> BTW, is there actually a double encapsulation in the original OVN test?
> >> Sounds strange.
> >
> > The problem was exposed in netdev_push_header() in
> > dp_packet_ol_send_prepare(packet, 0);
>
> That's true, but the packet dump in gdb showed a plain BFD packet.
> So, it was a first encapsulation, not double.
>
> >
> > -M
> >
> >>
> >>>
> >>> Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
> >>> Reported-by: Dumitru Ceara <dce...@redhat.com>
> >>> Reported-at: https://issues.redhat.com/browse/FDP-300
> >>> Signed-off-by: Mike Pattrick <m...@redhat.com>
> >>> ---
> >>> lib/dp-packet.h | 10 ++++++++--
> >>> lib/packets.c | 6 +++---
> >>> 2 files changed, 11 insertions(+), 5 deletions(-)
> >>
> >> Best regards, Ilya Maximets.
> >>
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
