[DPDK/ethdev Bug 1886] net/avp: tx_burst callbacks free mbufs then return short count, causing double-free

[bugzilla@dpdk.org]

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

            Bug ID: 1886
           Summary: net/avp: tx_burst callbacks free mbufs then return
                    short count, causing double-free
           Product: DPDK
           Version: 25.11
          Hardware: All
                OS: All
            Status: UNCONFIRMED
          Severity: normal
          Priority: Normal
         Component: ethdev
          Assignee: dev@dpdk.org
          Reporter: stephen@networkplumber.org
  Target Milestone: ---

Recent audit of how tx_pkt_burst is (or is not) being handled in drivers found
this.

The AVP PMD tx_burst callbacks (`avp_xmit_pkts` and `avp_xmit_scattered_pkts`)
violate the `rte_eth_tx_burst()` mbuf ownership contract.  Both functions free
all mbufs they process, but return the result of `avp_fifo_put()` which may be
smaller.  When the caller follows the standard pattern:

```c
n = rte_eth_tx_burst(port, txq, mbufs, nb_pkts);
for (i = n; i < nb_pkts; i++)
    rte_pktmbuf_free(mbufs[i]);
```

any mbufs that were freed by the driver but not reported as consumed will be
freed a second time by the caller.

There are several additional bugs in the same code paths, listed below.

### Bug 1: Double-free in avp_xmit_pkts (avp_ethdev.c ~line 1917 vs 1926)

The function frees each mbuf at line 1917 (`rte_pktmbuf_free(m)`) inside the
processing loop which runs for `count` packets.  At line 1924 it calls
`avp_fifo_put()` and returns the FIFO result `n` at line 1926.  If the FIFO
fills between the earlier free-count check and the put, `n < count`, and
`tx_pkts[n..count-1]` get double-freed.

Fix: return `count` instead of `n`, since all `count` mbufs have already been
consumed.

### Bug 2: Double-free in avp_xmit_scattered_pkts (line 1793 vs 1809)

Same pattern.  All `nb_pkts` mbufs are freed in the loop (line 1793) but
`avp_fifo_put()` result is returned (line 1809).

Fix: return `nb_pkts` instead of `n`.

### Bug 3: AVP host buffer leak on partial avp_fifo_get (lines 1862-1866)

```c
n = avp_fifo_get(alloc_q, (void **)&avp_bufs, count);
if (unlikely(n != count)) {
    txq->errors++;
    return 0;
}
```

If `avp_fifo_get()` returns a partial result (0 < n < count), those `n` AVP
buffers have been dequeued from the allocation queue but are never returned.
They are permanently leaked.  The same pattern exists in
`avp_xmit_scattered_pkts` at lines 1769-1774.

Fix: return partial buffers to alloc_q before returning:

```c
if (unlikely(n != count)) {
    if (n > 0)
        avp_fifo_put(alloc_q, (void **)&avp_bufs[0], n);
    return 0;
}
```

### Bug 4: Wrong variable in error comparison (line 1806)

```c
if (unlikely(n != orig_nb_pkts))
    txq->errors += (orig_nb_pkts - n);
```

`n` is the result of putting `nb_pkts` entries, but it is compared against
`orig_nb_pkts` which is the original caller count before clamping by
`AVP_MAX_TX_BURST` and resource limits.  These values differ whenever the
burst was clamped, so the error count is wrong.

Fix: compare against `nb_pkts`.

### Bug 5: Spurious error counting for unconsumed packets

Both TX functions increment `txq->errors` for all `nb_pkts` when returning 0
due to resource exhaustion or detach (lines 1714, 1761, 1832, 1855).  Since
the function returns 0, no mbufs were consumed — the caller still owns all of
them.  These are not transmission errors; they are backpressure.  Inflating
`oerrors` is misleading.

Fix: remove the error increments on the early-return-zero paths.

### Bug 6: Oversized packets silently truncated (lines 1886-1899)

When a packet exceeds both `guest_mbuf_size` and `host_mbuf_size`,
`avp_xmit_pkts` truncates it to the smaller buffer size and transmits a
corrupt frame.  Per the tx_burst contract, the driver should consume and
free the mbuf and count it in tx_errors, not send garbage.

Fix: set the AVP buffer length to 0 (skip the copy) and count the error.

## Steps to Reproduce

Code inspection of `drivers/net/avp/avp_ethdev.c` at tag v26.03-rc1.

The double-free (bugs 1-2) can trigger when the tx FIFO fills between the
`avp_fifo_free_count()` check and the `avp_fifo_put()` call.  This is a race
against the host consumer and is timing-dependent.

The buffer leak (bug 3) triggers when `avp_fifo_get()` from the alloc queue
returns a partial result, which can occur under host-side memory pressure.

## Suggested Patch

A patch addressing all six issues is available.  The key changes are:

- Return the number of mbufs freed (consumed) rather than the FIFO put count
- Return partially dequeued AVP buffers on allocation failure
- Remove error counting for unconsumed packets (backpressure is not an error)
- Drop oversized packets instead of truncating
- Fix the orig_nb_pkts vs nb_pkts comparison

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