[DPDK/vhost/virtio Bug 1884] vhost: fix tx_burst return value after VLAN insertion failure

[DPDK/vhost/virtio Bug 1884] vhost: fix tx_burst return value after VLAN insertion failure

[bugzilla]

http://bugs.dpdk.org/show_bug.cgi?id=1884

            Bug ID: 1884
           Summary: vhost: fix tx_burst return value after VLAN insertion
                    failure
           Product: DPDK
           Version: 25.11
          Hardware: All
                OS: All
            Status: UNCONFIRMED
          Severity: normal
          Priority: Normal
         Component: vhost/virtio
          Assignee: dev@dpdk.org
          Reporter: stephen@networkplumber.org
  Target Milestone: ---

Found while auditing tx_burst semantics of drivers.

The eth_vhost_tx() function violates the rte_eth_tx_burst() ownership
contract when VLAN insertion fails for some packets.

The VLAN pre-processing loop compacts surviving packets into bufs[]
starting at index 0:

    for (i = 0; i < nb_bufs; i++) {
        struct rte_mbuf *m = bufs[i];

        if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
            int error = rte_vlan_insert(&m);
            if (unlikely(error)) {
                rte_pktmbuf_free(m);
                continue;           /* <-- skip, don't store */
            }
        }
        bufs[nb_send] = m;          /* <-- compacts in-place */
        ++nb_send;
    }

When VLAN insertion fails, the packet is freed and skipped, so
nb_send < nb_bufs and the bufs[] array is rewritten as a compacted
version. After rte_vhost_enqueue_burst() the function returns nb_tx
(the number enqueued to the guest).

This causes two problems:

1. The return value does not correspond to a prefix of the original
   array. The caller expects bufs[0..n-1] to be consumed and
   bufs[n..nb_bufs-1] to be untouched. But the compaction has
   shuffled entries so bufs[n..nb_bufs-1] now contains stale
   pointers from before compaction.

   For example, with nb_bufs=4 where bufs[1] fails VLAN insertion:

     Original: bufs[] = {A, B, C, D}
     After compaction: bufs[] = {A, C, D, D}
                                          ^
                                    stale duplicate

   If vhost enqueues 2 (nb_tx=2), the driver frees A and C (which
   the driver enqueued - those are freed at line 515-516). The
   caller sees nb_tx=2, then tries to free bufs[2] and bufs[3],
   which are D and D -- double free.

2. Packets that passed VLAN pre-processing but were not enqueued
   by rte_vhost_enqueue_burst() are not freed by the driver.
   The caller cannot reliably free them either because the array
   has been rewritten.

The missed_pkts statistic (nb_bufs - nb_tx) is also inflated
because it counts VLAN failures (already freed) as missed.

-- 
You are receiving this mail because:
You are the assignee for the bug.