Skip to contentSkip to conversation

net/vhost: preserve Tx ownership on VLAN failure

PATCH Applied Weijun Pan 2 participants 4 messages Jul 26 – Oct 15 Network drivers

lore
Share & tools
lore

Weijun Pan

author
#0 lore raw

PATCH 1/1 net/vhost: preserve Tx ownership on VLAN failure

The Tx path compacts the input mbuf array after software VLAN insertion fails. Later packets are moved into the failed packet's slot while the failed mbuf is freed.

The returned count then no longer identifies a prefix of the original burst. An application freeing the unsent suffix can therefore free duplicate or stale mbuf pointers.

Stop preprocessing at the first VLAN insertion failure and leave the failed mbuf and the remaining suffix owned by the caller. Count only packets prepared for vhost enqueue but not accepted as missed.

Bugzilla ID: 1884

Fixes f63d356ee993 ("net/vhost: insert/strip VLAN header in software")

---
Cc: Jan Blunck <[email protected]>

Tested:
- Full x86_64 debug build with AddressSanitizer.
- Two-process net_vhost and virtio-user data-path test.
- Forced VLAN insertion failure on packet 1 by removing its headroom.
- rte_eth_tx_burst() returned 1 and preserved the unsent suffix.
- testpmd received 1 packet with no RX errors or missed packets.

 drivers/net/vhost/rte_eth_vhost.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/net/vhost/rte_eth_vhost.c +6 -8
index 05940f2461..9a85f1e26d 100644
@@ -474,19 +474,17 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 		/* Do VLAN tag insertion */
 		if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
 			int error = rte_vlan_insert(&m);
-			if (unlikely(error)) {
-				rte_pktmbuf_free(m);
-				continue;
-			}
+
+			if (unlikely(error))
+				break;
 		}
 
 		if (r->internal->tx_sw_csum)
 			vhost_dev_tx_sw_csum(m);
 
-
-		bufs[nb_send] = m;
-		++nb_send;
+		bufs[i] = m;
 	}
+	nb_send = i;
 
 	/* Enqueue packets to guest RX queue */
 	while (nb_send) {
@@ -506,7 +504,7 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 	for (i = 0; likely(i < nb_tx); i++)
 		nb_bytes += bufs[i]->pkt_len;
 
-	nb_missed = nb_bufs - nb_tx;
+	nb_missed = nb_send;
 
 	r->stats.pkts += nb_tx;
 	r->stats.bytes += nb_bytes;
Signature · 1 line
2.34.1
author
#1 lore raw

Recheck-request: iol-unit-arm64-testing

Quoted 72 lines · Weijun Pan

The Tx path compacts the input mbuf array after software VLAN insertion fails. Later packets are moved into the failed packet's slot while the failed mbuf is freed.

The returned count then no longer identifies a prefix of the original burst. An application freeing the unsent suffix can therefore free duplicate or stale mbuf pointers.

Stop preprocessing at the first VLAN insertion failure and leave the failed mbuf and the remaining suffix owned by the caller. Count only packets prepared for vhost enqueue but not accepted as missed.

Bugzilla ID: 1884

Fixes f63d356ee993 ("net/vhost: insert/strip VLAN header in software")

---
Cc: Jan Blunck <[email protected]>

Tested:
- Full x86_64 debug build with AddressSanitizer.
- Two-process net_vhost and virtio-user data-path test.
- Forced VLAN insertion failure on packet 1 by removing its headroom.
- rte_eth_tx_burst() returned 1 and preserved the unsent suffix.
- testpmd received 1 packet with no RX errors or missed packets.

 drivers/net/vhost/rte_eth_vhost.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/net/vhost/rte_eth_vhost.c +6 -9
index 05940f2461..9a85f1e26d 100644
@@ -474,19 +474,17 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
                /* Do VLAN tag insertion */
                if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
                        int error = rte_vlan_insert(&m);
-                       if (unlikely(error)) {
-                               rte_pktmbuf_free(m);
-                               continue;
-                       }
+
+                       if (unlikely(error))
+                               break;
                }
 
                if (r->internal->tx_sw_csum)
                        vhost_dev_tx_sw_csum(m);
 
-
-               bufs[nb_send] = m;
-               ++nb_send;
+               bufs[i] = m;
        }
+       nb_send = i;
 
        /* Enqueue packets to guest RX queue */
        while (nb_send) {
@@ -506,7 +504,7 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
        for (i = 0; likely(i < nb_tx); i++)
                nb_bytes += bufs[i]->pkt_len;
 
-       nb_missed = nb_bufs - nb_tx;
+       nb_missed = nb_send;
 
        r->stats.pkts += nb_tx;
        r->stats.bytes += nb_bytes;
--

2.34.1

Reviewed-by on v1 1/1
#2 lore raw
In reply to Weijun Pan
Quoted 28 lines · Weijun Pan

The Tx path compacts the input mbuf array after software VLAN insertion fails. Later packets are moved into the failed packet's slot while the failed mbuf is freed.

The returned count then no longer identifies a prefix of the original burst. An application freeing the unsent suffix can therefore free duplicate or stale mbuf pointers.

Stop preprocessing at the first VLAN insertion failure and leave the failed mbuf and the remaining suffix owned by the caller. Count only packets prepared for vhost enqueue but not accepted as missed.

Bugzilla ID: 1884

Fixes f63d356ee993 ("net/vhost: insert/strip VLAN header in software")

---
Cc: Jan Blunck <[email protected]>

Tested:
- Full x86_64 debug build with AddressSanitizer.
- Two-process net_vhost and virtio-user data-path test.
- Forced VLAN insertion failure on packet 1 by removing its headroom.
- rte_eth_tx_burst() returned 1 and preserved the unsent suffix.
- testpmd received 1 packet with no RX errors or missed packets.

Quoted 3 lines
 drivers/net/vhost/rte_eth_vhost.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Thanks,
Maxime

#3 lore raw
In reply to Weijun Pan
Quoted 27 lines · Weijun Pan

The Tx path compacts the input mbuf array after software VLAN insertion fails. Later packets are moved into the failed packet's slot while the failed mbuf is freed.

The returned count then no longer identifies a prefix of the original burst. An application freeing the unsent suffix can therefore free duplicate or stale mbuf pointers.

Stop preprocessing at the first VLAN insertion failure and leave the failed mbuf and the remaining suffix owned by the caller. Count only packets prepared for vhost enqueue but not accepted as missed.

Bugzilla ID: 1884

Fixes f63d356ee993 ("net/vhost: insert/strip VLAN header in software")

---
Cc: Jan Blunck<[email protected]>

Tested:
- Full x86_64 debug build with AddressSanitizer.
- Two-process net_vhost and virtio-user data-path test.
- Forced VLAN insertion failure on packet 1 by removing its headroom.
- rte_eth_tx_burst() returned 1 and preserved the unsent suffix.
- testpmd received 1 packet with no RX errors or missed packets.

Quoted 2 lines
  drivers/net/vhost/rte_eth_vhost.c | 14 ++++++--------
  1 file changed, 6 insertions(+), 8 deletions(-)

Applied to next-virtio/for-next-net.

Thanks,
Maxime