Skip to content

Commit

Permalink
fix use after free issue in mbuf free
Browse files Browse the repository at this point in the history
Two kinds of mbuf are used in f-stack: freebsd mbuf and dpdk mbuf.

freebsd mbufs are metadata used in freebsd stack, and their data
pointers (m_data) point to dpdk mbuf's data (buf_addr). And they have
their own chain, like this:

  bsd_mbuf1 -> bsd_mbuf2 -> bsd_mbuf3
      \            \            \
    dpdk_mbuf1 -> dpdk_mbuf2 -> dpdk_mbuf3

Considering the map relationship,

- m_freem() is corresponding to rte_pktmbuf_free(), is to free the whole
  chain of mbufs.
- m_free() is corresponding to rte_pktmbuf_free_seg(), is to free the
  specified mbuf segment.

The current implementation in f-stack uses rte_pktmbuf_free() for
m_free(). This leads to mbufs, which are still in use, be freed
unexpectedly. For example, if the bsd_mbuf1 is trimed into zero length,
bsd will invoke m_free() to free the specified segment, however, the
whole mbuf chain is freed by calling rte_pktmbuf_free().

  #0 rte_pktmbuf_free (m=0x22006fb480)
  #1 in ff_dpdk_pktmbuf_free (m=0x22006fb480)
  #2 in ff_mbuf_ext_free (m=0x7ffff7f82800, arg1=0x22006fb480, arg2=0x0)
  #3 in mb_free_ext (m=0x7ffff7f82800)
  #4 in m_free (m=0x7ffff7f82800)
  #5 in sbcompress (sb=, m=0x7ffff7f82800, n=)
  #6 in sbappendstream_locked (sb=, m=0x7ffff7f82800, flags=0)

The fix is straightforward. Use the correct API for segment free.

Reported-by: Yong-Hao Zou <yonghaoz1994@gmail.com>
Signed-off-by: Jianfeng Tan <henry.tjf@antgroup.com>
  • Loading branch information
tanjianfeng authored and jfb8856606 committed Mar 6, 2021
1 parent c0c877b commit 06c3e57
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 6 deletions.
4 changes: 2 additions & 2 deletions lib/ff_dpdk_if.c
Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ ff_veth_input(const struct ff_dpdk_if_context *ctx, struct rte_mbuf *pkt)
data = rte_pktmbuf_mtod(pn, void*);
len = rte_pktmbuf_data_len(pn);

void *mb = ff_mbuf_get(prev, data, len);
void *mb = ff_mbuf_get(prev, pn, data, len);
if (mb == NULL) {
ff_mbuf_free(hdr);
rte_pktmbuf_free(pkt);
Expand Down Expand Up @@ -1967,7 +1967,7 @@ ff_dpdk_run(loop_func_t loop, void *arg) {
void
ff_dpdk_pktmbuf_free(void *m)
{
rte_pktmbuf_free((struct rte_mbuf *)m);
rte_pktmbuf_free_seg((struct rte_mbuf *)m);
}

static uint32_t
Expand Down
6 changes: 3 additions & 3 deletions lib/ff_veth.c
Original file line number Diff line number Diff line change
Expand Up @@ -209,16 +209,16 @@ ff_mbuf_gethdr(void *pkt, uint16_t total, void *data,
}

void *
ff_mbuf_get(void *m, void *data, uint16_t len)
ff_mbuf_get(void *p, void *m, void *data, uint16_t len)
{
struct mbuf *prev = (struct mbuf *)m;
struct mbuf *prev = (struct mbuf *)p;
struct mbuf *mb = m_get(M_NOWAIT, MT_DATA);

if (mb == NULL) {
return NULL;
}

m_extadd(mb, data, len, NULL, NULL, NULL, 0, 0);
m_extadd(mb, data, len, ff_mbuf_ext_free, m, NULL, 0, EXT_DISPOSABLE);

mb->m_next = NULL;
mb->m_nextpkt = NULL;
Expand Down
2 changes: 1 addition & 1 deletion lib/ff_veth.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ int ff_veth_detach(void *arg);

void *ff_mbuf_gethdr(void *pkt, uint16_t total, void *data,
uint16_t len, uint8_t rx_csum);
void *ff_mbuf_get(void *m, void *data, uint16_t len);
void *ff_mbuf_get(void *p, void *m, void *data, uint16_t len);
void ff_mbuf_free(void *m);

int ff_mbuf_copydata(void *m, void *data, int off, int len);
Expand Down

0 comments on commit 06c3e57

Please sign in to comment.