| Message ID | 20240313172949.158842-4-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 0ce82ac45e6828c5e1531b2ada044b7abbbadea5 |
| Headers | show |
| Series | netdev-dpdk: More Tx offlaod fixes. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Wed, Mar 13, 2024 at 1:29 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > Tunnel types are not flags, but 4-bit fields, so checking them with > a simple binary 'and' is incorrect and may produce false-positive > matches. > > While the current implementation is unlikely to cause any issues today, > since both RTE_MBUF_F_TX_TUNNEL_VXLAN and RTE_MBUF_F_TX_TUNNEL_GENEVE > only have 1 bit set, it is risky to have this code and it may lead > to problems if we add support for other tunnel types in the future. > > Use proper field checks instead. Also adding a warning for unexpected > tunnel types in case something goes wrong. > > Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Mike Pattrick <mkp@redhat.com>
On 3/14/24 15:17, Mike Pattrick wrote: > On Wed, Mar 13, 2024 at 1:29 PM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> Tunnel types are not flags, but 4-bit fields, so checking them with >> a simple binary 'and' is incorrect and may produce false-positive >> matches. >> >> While the current implementation is unlikely to cause any issues today, >> since both RTE_MBUF_F_TX_TUNNEL_VXLAN and RTE_MBUF_F_TX_TUNNEL_GENEVE >> only have 1 bit set, it is risky to have this code and it may lead >> to problems if we add support for other tunnel types in the future. >> >> Use proper field checks instead. Also adding a warning for unexpected >> tunnel types in case something goes wrong. >> >> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > Acked-by: Mike Pattrick <mkp@redhat.com> > Thanks, Mike! I applied the set and backported to 3.3. best regards, Ilya Maximets.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1ae2ef398..29a6bf032 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2601,8 +2601,9 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf *mbuf) /* If packet is vxlan or geneve tunnel packet, calculate outer * l2 len and outer l3 len. Inner l2/l3/l4 len are calculated * before. */ - if (mbuf->ol_flags & - (RTE_MBUF_F_TX_TUNNEL_GENEVE | RTE_MBUF_F_TX_TUNNEL_VXLAN)) { + const uint64_t tunnel_type = mbuf->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK; + if (tunnel_type == RTE_MBUF_F_TX_TUNNEL_GENEVE || + tunnel_type == RTE_MBUF_F_TX_TUNNEL_VXLAN) { mbuf->outer_l2_len = (char *) dp_packet_l3(pkt) - (char *) dp_packet_eth(pkt); mbuf->outer_l3_len = (char *) dp_packet_l4(pkt) - @@ -2616,6 +2617,12 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf *mbuf) mbuf->ol_flags &= ~(RTE_MBUF_F_TX_IPV4 | RTE_MBUF_F_TX_IPV6); } + } else if (OVS_UNLIKELY(tunnel_type)) { + VLOG_WARN_RL(&rl, "%s: Unexpected tunnel type: %#"PRIx64, + netdev_get_name(&dev->up), tunnel_type); + netdev_dpdk_mbuf_dump(netdev_get_name(&dev->up), + "Packet with unexpected tunnel type", mbuf); + return false; } else { mbuf->l2_len = (char *) dp_packet_l3(pkt) - (char *) dp_packet_eth(pkt); @@ -2641,8 +2648,7 @@ netdev_dpdk_prep_hwol_packet(struct netdev_dpdk *dev, struct rte_mbuf *mbuf) return false; } - if (mbuf->ol_flags & (RTE_MBUF_F_TX_TUNNEL_GENEVE | - RTE_MBUF_F_TX_TUNNEL_VXLAN)) { + if (tunnel_type) { mbuf->tso_segsz = dev->mtu - mbuf->l2_len - mbuf->l3_len - mbuf->l4_len - mbuf->outer_l3_len; } else {
Tunnel types are not flags, but 4-bit fields, so checking them with a simple binary 'and' is incorrect and may produce false-positive matches. While the current implementation is unlikely to cause any issues today, since both RTE_MBUF_F_TX_TUNNEL_VXLAN and RTE_MBUF_F_TX_TUNNEL_GENEVE only have 1 bit set, it is risky to have this code and it may lead to problems if we add support for other tunnel types in the future. Use proper field checks instead. Also adding a warning for unexpected tunnel types in case something goes wrong. Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/netdev-dpdk.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)