| Message ID | 20240122175110.548125-1-mkp@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] userspace: Check for inner L3 while preparing encapsulated packets. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | success | test: success |
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. BTW, is there actually a double encapsulation in the original OVN test? Sounds strange. > > Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.") > Reported-by: Dumitru Ceara <dceara@redhat.com> > Reported-at: https://issues.redhat.com/browse/FDP-300 > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/dp-packet.h | 10 ++++++++-- > lib/packets.c | 6 +++--- > 2 files changed, 11 insertions(+), 5 deletions(-) Best regards, Ilya Maximets.
On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets <i.maximets@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. > 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); -M > > > > > Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.") > > Reported-by: Dumitru Ceara <dceara@redhat.com> > > Reported-at: https://issues.redhat.com/browse/FDP-300 > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > --- > > lib/dp-packet.h | 10 ++++++++-- > > lib/packets.c | 6 +++--- > > 2 files changed, 11 insertions(+), 5 deletions(-) > > Best regards, Ilya Maximets. >
On 1/22/24 21:33, Mike Pattrick wrote: > On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets <i.maximets@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? > >> 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 <dceara@redhat.com> >>> Reported-at: https://issues.redhat.com/browse/FDP-300 >>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >>> --- >>> lib/dp-packet.h | 10 ++++++++-- >>> lib/packets.c | 6 +++--- >>> 2 files changed, 11 insertions(+), 5 deletions(-) >> >> Best regards, Ilya Maximets. >> >
On Mon, Jan 22, 2024 at 4:10 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 1/22/24 21:33, Mike Pattrick wrote: > > On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets <i.maximets@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 <dceara@redhat.com> > >>> Reported-at: https://issues.redhat.com/browse/FDP-300 > >>> Signed-off-by: Mike Pattrick <mkp@redhat.com> > >>> --- > >>> lib/dp-packet.h | 10 ++++++++-- > >>> lib/packets.c | 6 +++--- > >>> 2 files changed, 11 insertions(+), 5 deletions(-) > >> > >> Best regards, Ilya Maximets. > >> > > >
On 1/22/24 23:24, Mike Pattrick wrote: > On Mon, Jan 22, 2024 at 4:10 PM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 1/22/24 21:33, Mike Pattrick wrote: >>> On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets <i.maximets@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. Sounds like a bug to me. bfd_put_packet() should set correct offsets in the packet, otherwise we'll get random failures with different actions that may depend on these offsets to be populated. > \_ 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(). I agree with that. But it still doesn't explain why the DP_PACKET_OL_TX_IP_CKSUM is set after the first run. The inner checksum is fully calculated and correct. There should be no Tx offloading for it set. Only the DP_PACKET_OL_TX_OUTER_IP_CKSUM should be set in this packet. Or am I missing something? > > -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 <dceara@redhat.com> >>>>> Reported-at: https://issues.redhat.com/browse/FDP-300 >>>>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >>>>> --- >>>>> lib/dp-packet.h | 10 ++++++++-- >>>>> lib/packets.c | 6 +++--- >>>>> 2 files changed, 11 insertions(+), 5 deletions(-) >>>> >>>> Best regards, Ilya Maximets. >>>> >>> >> >
On Tue, Jan 23, 2024 at 7:41 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 1/22/24 23:24, Mike Pattrick wrote: > > On Mon, Jan 22, 2024 at 4:10 PM Ilya Maximets <i.maximets@ovn.org> wrote: > >> > >> On 1/22/24 21:33, Mike Pattrick wrote: > >>> On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets <i.maximets@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. > > Sounds like a bug to me. bfd_put_packet() should set correct offsets > in the packet, otherwise we'll get random failures with different > actions that may depend on these offsets to be populated. Agreed. Do you want me to homologate this as part of this series? > > > \_ 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(). > > I agree with that. But it still doesn't explain why the DP_PACKET_OL_TX_IP_CKSUM > is set after the first run. The inner checksum is fully calculated and correct. > There should be no Tx offloading for it set. Only the DP_PACKET_OL_TX_OUTER_IP_CKSUM > should be set in this packet. Or am I missing something? bfd_put_packet() calculates the checksum, so it will always be correct, miniflow_extract() sets DP_PACKET_OL_TX_IP_CKSUM if the previous packet had DP_PACKET_OL_RX_IP_CKSUM_GOOD, which it would have gotten from dp_packet_ol_send_prepare(). > > > > > -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 <dceara@redhat.com> > >>>>> Reported-at: https://issues.redhat.com/browse/FDP-300 > >>>>> Signed-off-by: Mike Pattrick <mkp@redhat.com> > >>>>> --- > >>>>> lib/dp-packet.h | 10 ++++++++-- > >>>>> lib/packets.c | 6 +++--- > >>>>> 2 files changed, 11 insertions(+), 5 deletions(-) > >>>> > >>>> Best regards, Ilya Maximets. > >>>> > >>> > >> > > >
On 1/23/24 14:31, Mike Pattrick wrote: > On Tue, Jan 23, 2024 at 7:41 AM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 1/22/24 23:24, Mike Pattrick wrote: >>> On Mon, Jan 22, 2024 at 4:10 PM Ilya Maximets <i.maximets@ovn.org> wrote: >>>> >>>> On 1/22/24 21:33, Mike Pattrick wrote: >>>>> On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets <i.maximets@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. >> >> Sounds like a bug to me. bfd_put_packet() should set correct offsets >> in the packet, otherwise we'll get random failures with different >> actions that may depend on these offsets to be populated. > > Agreed. Do you want me to homologate this as part of this series? Yeah, a separate patch for this would be great. > >> >>> \_ 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(). >> >> I agree with that. But it still doesn't explain why the DP_PACKET_OL_TX_IP_CKSUM >> is set after the first run. The inner checksum is fully calculated and correct. >> There should be no Tx offloading for it set. Only the DP_PACKET_OL_TX_OUTER_IP_CKSUM >> should be set in this packet. Or am I missing something? > > bfd_put_packet() calculates the checksum, so it will always be > correct, miniflow_extract() sets DP_PACKET_OL_TX_IP_CKSUM if the > previous packet had DP_PACKET_OL_RX_IP_CKSUM_GOOD, which it would have > gotten from dp_packet_ol_send_prepare(). OK. That makes sense. I understand now. Thanks! > > >> >>> >>> -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 <dceara@redhat.com> >>>>>>> Reported-at: https://issues.redhat.com/browse/FDP-300 >>>>>>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >>>>>>> --- >>>>>>> lib/dp-packet.h | 10 ++++++++-- >>>>>>> lib/packets.c | 6 +++--- >>>>>>> 2 files changed, 11 insertions(+), 5 deletions(-) >>>>>> >>>>>> Best regards, Ilya Maximets. >>>>>> >>>>> >>>> >>> >> >
diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 939bec5c8..81b01f331 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -537,7 +537,7 @@ dp_packet_inner_l4(const struct dp_packet *b) static inline size_t dp_packet_inner_l4_size(const struct dp_packet *b) { - return OVS_LIKELY(b->l4_ofs != UINT16_MAX) + return OVS_LIKELY(b->inner_l4_ofs != UINT16_MAX) ? (const char *) dp_packet_tail(b) - (const char *) dp_packet_inner_l4(b) - dp_packet_l2_pad_size(b) @@ -1385,7 +1385,13 @@ dp_packet_ip_checksum_bad(const struct dp_packet *p) static inline void dp_packet_ip_set_header_csum(struct dp_packet *p, bool inner) { - struct ip_header *ip = (inner) ? dp_packet_inner_l3(p) : dp_packet_l3(p); + struct ip_header *ip; + + if (inner && dp_packet_inner_l3(p)) { + ip = dp_packet_inner_l3(p); + } else { + ip = dp_packet_l3(p); + } ovs_assert(ip); ip->ip_csum = 0; diff --git a/lib/packets.c b/lib/packets.c index f23d25420..921862b0d 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -2004,7 +2004,7 @@ packet_tcp_complete_csum(struct dp_packet *p, bool inner) void *ip_hdr; bool is_v4; - if (inner) { + if (inner && dp_packet_inner_l4(p)) { tcp = dp_packet_inner_l4(p); ip_hdr = dp_packet_inner_l3(p); tcp_sz = dp_packet_inner_l4_size(p); @@ -2050,7 +2050,7 @@ packet_udp_complete_csum(struct dp_packet *p, bool inner) void *ip_hdr; bool is_v4; - if (inner) { + if (inner && dp_packet_inner_l4(p)) { udp = dp_packet_inner_l4(p); ip_hdr = dp_packet_inner_l3(p); udp_sz = dp_packet_inner_l4_size(p); @@ -2104,7 +2104,7 @@ packet_sctp_complete_csum(struct dp_packet *p, bool inner) uint16_t tp_len; ovs_be32 csum; - if (inner) { + if (inner && dp_packet_inner_l4(p)) { sh = dp_packet_inner_l4(p); tp_len = dp_packet_inner_l4_size(p); } else {
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. Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.") Reported-by: Dumitru Ceara <dceara@redhat.com> Reported-at: https://issues.redhat.com/browse/FDP-300 Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/dp-packet.h | 10 ++++++++-- lib/packets.c | 6 +++--- 2 files changed, 11 insertions(+), 5 deletions(-)