| Message ID | 20240503102156.7699-1-amitprakashs@marvell.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] lib: Fix segfault for tunnel packet. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/3/24 12:21, Amit Prakash Shukla wrote: > Add NULL check to UDP, TCP and SCTP checksum functions. This patch > also adds changes to populate inner_l3_ofs and inner_l4_ofs for the > tunneled packets received from ports other than vport which are > required by the protocol specific checksum function to parse the > headers. > > Thread 22 "pmd-c07/id:15" received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0xffff6e70dc00 (LWP 1061)] > 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 > 2061 if (!udp->udp_csum) { > > 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 > 0x13e5126c in dp_packet_ol_send_prepare at lib/dp-packet.c:638 > 0x13eb7d4c in netdev_push_header at lib/netdev.c:1035 > 0x13e69830 in push_tnl_action at lib/dpif-netdev.c:9067 > 0x13e69dac in dp_execute_cb at lib/dpif-netdev.c:9226 > 0x13ec72c4 in odp_execute_actions at lib/odp-execute.c:1008 > 0x13e6a7bc in dp_netdev_execute_actions at lib/dpif-netdev.c:9524 > 0x13e673d0 in packet_batch_per_flow_execute at lib/dpif-netdev.c:8271 > 0x13e69188 in dp_netdev_input__ at lib/dpif-netdev.c:8899 > 0x13e691f8 in dp_netdev_input at lib/dpif-netdev.c:8908 > 0x13e600e4 in dp_netdev_process_rxq_port at lib/dpif-netdev.c:5660 > 0x13e649a8 in pmd_thread_main at lib/dpif-netdev.c:7295 > 0x13f44b2c in ovsthread_wrapper at lib/ovs-thread.c:423 > Hi, Amit. Thanks for the patch! Nit: Please, send new versions as separate emails, do not send them as a reply. Could you, please, explain how exactly we end up in this situation? The stack trace is fine, but it doesn't tell the whole story, e.g. what other actions being executed on this packet? We will likely need a test case for this scenario. We can help writing one, if you can describe steps to reproduce the issue. > CC: Mike Pattrick <mkp@redhat.com> > Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.") > > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com> > --- > > v2: > - Added Fixes tag and updated commit message. > > lib/netdev.c | 7 +++++++ > lib/packets.c | 10 +++++++++- > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev.c b/lib/netdev.c > index f2d921ed6..19bd87ef7 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -1032,6 +1032,13 @@ netdev_push_header(const struct netdev *netdev, > netdev_get_name(netdev)); > continue; > } > + if (packet->l3_ofs != UINT16_MAX) { > + packet->inner_l3_ofs = packet->l3_ofs + data->header_len; > + } > + if (packet->l4_ofs != UINT16_MAX) { > + packet->inner_l4_ofs = packet->l4_ofs + data->header_len; > + } > + This is confusing. We should not end up here, unless it is a double encapsulation scenario. And in this case the offsets should already be initialized by the same code in netdev_tnl_push_udp_header(). > dp_packet_ol_send_prepare(packet, 0); > } > netdev->netdev_class->push_header(netdev, packet, data); > diff --git a/lib/packets.c b/lib/packets.c > index 5803d26f4..988c0e41f 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > @@ -2011,6 +2011,10 @@ packet_tcp_complete_csum(struct dp_packet *p, bool inner) > tcp_sz = dp_packet_l4_size(p); > } > > + if (!tcp || !ip_hdr) { > + return; > + } > + > if (!inner && dp_packet_hwol_is_outer_ipv6(p)) { > is_v4 = false; > } else if (!inner && dp_packet_hwol_is_outer_ipv4(p)) { > @@ -2058,7 +2062,7 @@ packet_udp_complete_csum(struct dp_packet *p, bool inner) > } > > /* Skip csum calculation if the udp_csum is zero. */ > - if (!udp->udp_csum) { > + if (!udp || !ip_hdr || !udp->udp_csum) { > return; > } > > @@ -2109,6 +2113,10 @@ packet_sctp_complete_csum(struct dp_packet *p, bool inner) > tp_len = dp_packet_l4_size(p); > } > > + if (!sh) { > + return; > + } > + > put_16aligned_be32(&sh->sctp_csum, 0); > csum = crc32c((void *) sh, tp_len); > put_16aligned_be32(&sh->sctp_csum, csum); Please, don't add these checks. If these functions are called for incorrect packet it's a bug in a code that calls them. We may add assertions instead, e.g. ovs_assert(sh); Best regards, Ilya Maximets.
On Fri, May 3, 2024 at 6:22 AM Amit Prakash Shukla <amitprakashs@marvell.com> wrote: > > Add NULL check to UDP, TCP and SCTP checksum functions. This patch > also adds changes to populate inner_l3_ofs and inner_l4_ofs for the > tunneled packets received from ports other than vport which are > required by the protocol specific checksum function to parse the > headers. > > Thread 22 "pmd-c07/id:15" received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0xffff6e70dc00 (LWP 1061)] > 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 > 2061 if (!udp->udp_csum) { > > 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 > 0x13e5126c in dp_packet_ol_send_prepare at lib/dp-packet.c:638 > 0x13eb7d4c in netdev_push_header at lib/netdev.c:1035 > 0x13e69830 in push_tnl_action at lib/dpif-netdev.c:9067 > 0x13e69dac in dp_execute_cb at lib/dpif-netdev.c:9226 > 0x13ec72c4 in odp_execute_actions at lib/odp-execute.c:1008 > 0x13e6a7bc in dp_netdev_execute_actions at lib/dpif-netdev.c:9524 > 0x13e673d0 in packet_batch_per_flow_execute at lib/dpif-netdev.c:8271 > 0x13e69188 in dp_netdev_input__ at lib/dpif-netdev.c:8899 > 0x13e691f8 in dp_netdev_input at lib/dpif-netdev.c:8908 > 0x13e600e4 in dp_netdev_process_rxq_port at lib/dpif-netdev.c:5660 > 0x13e649a8 in pmd_thread_main at lib/dpif-netdev.c:7295 > 0x13f44b2c in ovsthread_wrapper at lib/ovs-thread.c:423 > > CC: Mike Pattrick <mkp@redhat.com> > Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.") > > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com> > --- > > v2: > - Added Fixes tag and updated commit message. > > lib/netdev.c | 7 +++++++ > lib/packets.c | 10 +++++++++- > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev.c b/lib/netdev.c > index f2d921ed6..19bd87ef7 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -1032,6 +1032,13 @@ netdev_push_header(const struct netdev *netdev, > netdev_get_name(netdev)); > continue; > } > + if (packet->l3_ofs != UINT16_MAX) { > + packet->inner_l3_ofs = packet->l3_ofs + data->header_len; > + } > + if (packet->l4_ofs != UINT16_MAX) { > + packet->inner_l4_ofs = packet->l4_ofs + data->header_len; > + } > + > dp_packet_ol_send_prepare(packet, 0); > } > netdev->netdev_class->push_header(netdev, packet, data); > diff --git a/lib/packets.c b/lib/packets.c > index 5803d26f4..988c0e41f 100644 > --- a/lib/packets.c > +++ b/lib/packets.c > @@ -2011,6 +2011,10 @@ packet_tcp_complete_csum(struct dp_packet *p, bool inner) > tcp_sz = dp_packet_l4_size(p); > } > > + if (!tcp || !ip_hdr) { > + return; > + } This suggests a packet has NETDEV_TX_OFFLOAD_TCP_CKSUM set but no TCP header or the offsets are set incorrectly. If that's the case then there will be additional issues in netdev-linux, the avx512 code, and potentially in other DPDK drivers. As Ilya mentioned, an assert here would be preferable. -M > + > if (!inner && dp_packet_hwol_is_outer_ipv6(p)) { > is_v4 = false; > } else if (!inner && dp_packet_hwol_is_outer_ipv4(p)) { > @@ -2058,7 +2062,7 @@ packet_udp_complete_csum(struct dp_packet *p, bool inner) > } > > /* Skip csum calculation if the udp_csum is zero. */ > - if (!udp->udp_csum) { > + if (!udp || !ip_hdr || !udp->udp_csum) { > return; > } > > @@ -2109,6 +2113,10 @@ packet_sctp_complete_csum(struct dp_packet *p, bool inner) > tp_len = dp_packet_l4_size(p); > } > > + if (!sh) { > + return; > + } > + > put_16aligned_be32(&sh->sctp_csum, 0); > csum = crc32c((void *) sh, tp_len); > put_16aligned_be32(&sh->sctp_csum, csum); > -- > 2.34.1 >
Hi Ilya, Thank you for the review and feedback. Please find my reply in-line. Thanks, Amit Shukla > -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Friday, May 3, 2024 4:41 PM > To: Amit Prakash Shukla <amitprakashs@marvell.com>; ovs- > dev@openvswitch.org > Cc: Harman Kalra <hkalra@marvell.com>; Jerin Jacob <jerinj@marvell.com>; > i.maximets@ovn.org; Michael Pattrick <mkp@redhat.com> > Subject: [EXTERNAL] Re: [ovs-dev] [PATCH v2] lib: Fix segfault for tunnel > packet. > > Prioritize security for external emails: Confirm sender and content safety > before clicking links or opening attachments > > ---------------------------------------------------------------------- > On 5/3/24 12:21, Amit Prakash Shukla wrote: > > Add NULL check to UDP, TCP and SCTP checksum functions. This patch > > also adds changes to populate inner_l3_ofs and inner_l4_ofs for the > > tunneled packets received from ports other than vport which are > > required by the protocol specific checksum function to parse the > > headers. > > > > Thread 22 "pmd-c07/id:15" received signal SIGSEGV, Segmentation fault. > > [Switching to Thread 0xffff6e70dc00 (LWP 1061)] > > 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 > > 2061 if (!udp->udp_csum) { > > > > 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 > > 0x13e5126c in dp_packet_ol_send_prepare at lib/dp-packet.c:638 > > 0x13eb7d4c in netdev_push_header at lib/netdev.c:1035 > > 0x13e69830 in push_tnl_action at lib/dpif-netdev.c:9067 0x13e69dac in > > dp_execute_cb at lib/dpif-netdev.c:9226 > > 0x13ec72c4 in odp_execute_actions at lib/odp-execute.c:1008 0x13e6a7bc > > in dp_netdev_execute_actions at lib/dpif-netdev.c:9524 > > 0x13e673d0 in packet_batch_per_flow_execute at lib/dpif-netdev.c:8271 > > 0x13e69188 in dp_netdev_input__ at lib/dpif-netdev.c:8899 > > 0x13e691f8 in dp_netdev_input at lib/dpif-netdev.c:8908 > > 0x13e600e4 in dp_netdev_process_rxq_port at lib/dpif-netdev.c:5660 > > 0x13e649a8 in pmd_thread_main at lib/dpif-netdev.c:7295 0x13f44b2c in > > ovsthread_wrapper at lib/ovs-thread.c:423 > > > > Hi, Amit. Thanks for the patch! > > Nit: Please, send new versions as separate emails, do not send them > as a reply. [Amit] : Sure, I will take care next time. > > Could you, please, explain how exactly we end up in this situation? > The stack trace is fine, but it doesn't tell the whole story, e.g. > what other actions being executed on this packet? [Amit]: On debugging further, observed that it was stray packet without encapsulation and offload flag set. Will fix it in driver code. > > We will likely need a test case for this scenario. We can help writing one, if you > can describe steps to reproduce the issue. > > > > CC: Mike Pattrick <mkp@redhat.com> > > Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.") > > > > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com> > > --- > > > > v2: > > - Added Fixes tag and updated commit message. > > > > lib/netdev.c | 7 +++++++ > > lib/packets.c | 10 +++++++++- > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/lib/netdev.c b/lib/netdev.c index f2d921ed6..19bd87ef7 > > 100644 > > --- a/lib/netdev.c > > +++ b/lib/netdev.c > > @@ -1032,6 +1032,13 @@ netdev_push_header(const struct netdev > *netdev, > > netdev_get_name(netdev)); > > continue; > > } > > + if (packet->l3_ofs != UINT16_MAX) { > > + packet->inner_l3_ofs = packet->l3_ofs + data->header_len; > > + } > > + if (packet->l4_ofs != UINT16_MAX) { > > + packet->inner_l4_ofs = packet->l4_ofs + data->header_len; > > + } > > + > > This is confusing. We should not end up here, unless it is a double > encapsulation scenario. And in this case the offsets should already be > initialized by the same code in netdev_tnl_push_udp_header(). > > > dp_packet_ol_send_prepare(packet, 0); > > } > > netdev->netdev_class->push_header(netdev, packet, data); > > diff --git a/lib/packets.c b/lib/packets.c index 5803d26f4..988c0e41f > > 100644 > > --- a/lib/packets.c > > +++ b/lib/packets.c > > @@ -2011,6 +2011,10 @@ packet_tcp_complete_csum(struct dp_packet > *p, bool inner) > > tcp_sz = dp_packet_l4_size(p); > > } > > > > + if (!tcp || !ip_hdr) { > > + return; > > + } > > + > > if (!inner && dp_packet_hwol_is_outer_ipv6(p)) { > > is_v4 = false; > > } else if (!inner && dp_packet_hwol_is_outer_ipv4(p)) { @@ > > -2058,7 +2062,7 @@ packet_udp_complete_csum(struct dp_packet *p, > bool inner) > > } > > > > /* Skip csum calculation if the udp_csum is zero. */ > > - if (!udp->udp_csum) { > > + if (!udp || !ip_hdr || !udp->udp_csum) { > > return; > > } > > > > @@ -2109,6 +2113,10 @@ packet_sctp_complete_csum(struct dp_packet > *p, bool inner) > > tp_len = dp_packet_l4_size(p); > > } > > > > + if (!sh) { > > + return; > > + } > > + > > put_16aligned_be32(&sh->sctp_csum, 0); > > csum = crc32c((void *) sh, tp_len); > > put_16aligned_be32(&sh->sctp_csum, csum); > > Please, don't add these checks. If these functions are called for incorrect > packet it's a bug in a code that calls them. We may add assertions instead, e.g. > ovs_assert(sh); [Amit]: Sure, can I send next version of patch for ovs_assert to csum function for TCP, UDP and SCTP? Thank you for the pointers! > > Best regards, Ilya Maximets.
diff --git a/lib/netdev.c b/lib/netdev.c index f2d921ed6..19bd87ef7 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -1032,6 +1032,13 @@ netdev_push_header(const struct netdev *netdev, netdev_get_name(netdev)); continue; } + if (packet->l3_ofs != UINT16_MAX) { + packet->inner_l3_ofs = packet->l3_ofs + data->header_len; + } + if (packet->l4_ofs != UINT16_MAX) { + packet->inner_l4_ofs = packet->l4_ofs + data->header_len; + } + dp_packet_ol_send_prepare(packet, 0); } netdev->netdev_class->push_header(netdev, packet, data); diff --git a/lib/packets.c b/lib/packets.c index 5803d26f4..988c0e41f 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -2011,6 +2011,10 @@ packet_tcp_complete_csum(struct dp_packet *p, bool inner) tcp_sz = dp_packet_l4_size(p); } + if (!tcp || !ip_hdr) { + return; + } + if (!inner && dp_packet_hwol_is_outer_ipv6(p)) { is_v4 = false; } else if (!inner && dp_packet_hwol_is_outer_ipv4(p)) { @@ -2058,7 +2062,7 @@ packet_udp_complete_csum(struct dp_packet *p, bool inner) } /* Skip csum calculation if the udp_csum is zero. */ - if (!udp->udp_csum) { + if (!udp || !ip_hdr || !udp->udp_csum) { return; } @@ -2109,6 +2113,10 @@ packet_sctp_complete_csum(struct dp_packet *p, bool inner) tp_len = dp_packet_l4_size(p); } + if (!sh) { + return; + } + put_16aligned_be32(&sh->sctp_csum, 0); csum = crc32c((void *) sh, tp_len); put_16aligned_be32(&sh->sctp_csum, csum);
Add NULL check to UDP, TCP and SCTP checksum functions. This patch also adds changes to populate inner_l3_ofs and inner_l4_ofs for the tunneled packets received from ports other than vport which are required by the protocol specific checksum function to parse the headers. Thread 22 "pmd-c07/id:15" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0xffff6e70dc00 (LWP 1061)] 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 2061 if (!udp->udp_csum) { 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061 0x13e5126c in dp_packet_ol_send_prepare at lib/dp-packet.c:638 0x13eb7d4c in netdev_push_header at lib/netdev.c:1035 0x13e69830 in push_tnl_action at lib/dpif-netdev.c:9067 0x13e69dac in dp_execute_cb at lib/dpif-netdev.c:9226 0x13ec72c4 in odp_execute_actions at lib/odp-execute.c:1008 0x13e6a7bc in dp_netdev_execute_actions at lib/dpif-netdev.c:9524 0x13e673d0 in packet_batch_per_flow_execute at lib/dpif-netdev.c:8271 0x13e69188 in dp_netdev_input__ at lib/dpif-netdev.c:8899 0x13e691f8 in dp_netdev_input at lib/dpif-netdev.c:8908 0x13e600e4 in dp_netdev_process_rxq_port at lib/dpif-netdev.c:5660 0x13e649a8 in pmd_thread_main at lib/dpif-netdev.c:7295 0x13f44b2c in ovsthread_wrapper at lib/ovs-thread.c:423 CC: Mike Pattrick <mkp@redhat.com> Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.") Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com> --- v2: - Added Fixes tag and updated commit message. lib/netdev.c | 7 +++++++ lib/packets.c | 10 +++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)