On Thu, Sep 03, 2020 at 06:34:38AM +0200, Bjorn Ketelaars wrote:
> On Tue 25/08/2020 08:42, Martin Pieuchot wrote:
> > Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
> > https://marc.info/?l=netbsd-tech-net&m=159827988018641&w=2
> >
> > It seems to be that such leak is present in other uses of m_defrag() in
> > the tree. I won't take the time to go though all of them, but an audit
> > would be welcome.
>
> Similar issue seems to exist in:
>
> sys/arch/octeon/dev/if_cnmac.c
> sys/arch/octeon/dev/if_ogx.c
> sys/dev/fdt/if_dwge.c
> sys/dev/fdt/if_dwxe.c
> sys/dev/ic/bcmgenet.c
> sys/dev/ic/gem.c
> sys/dev/ic/re.c
> sys/dev/ic/rt2860.c
> sys/dev/pci/if_bge.c
> sys/dev/pci/if_bnx.c
> sys/dev/pci/if_bnxt.c
> sys/dev/pci/if_bwfm_pci.c
> sys/dev/pci/if_cas.c
> sys/dev/pci/if_em.c
> sys/dev/pci/if_iavf.c
> sys/dev/pci/if_ix.c
> sys/dev/pci/if_ixl.c
> sys/dev/pci/if_mcx.c
> sys/dev/pci/if_msk.c
> sys/dev/pci/if_myx.c
> sys/dev/pci/if_rge.c
> sys/dev/pci/if_se.c
> sys/dev/pci/if_sis.c
> sys/dev/pci/if_sk.c
> sys/dev/pci/if_vge.c
> sys/dev/pci/if_vic.c
> sys/dev/pci/if_vmx.c
> sys/dev/pci/if_vr.c
> sys/dev/pv/if_hvn.c
> sys/dev/pv/if_vio.c
> sys/dev/pv/if_xnf.c
>
> The diff below should fix this. At least I'm able to build a kernel
> (amd64 only), which doesn't explode when actually using it.
This is probably not correct. m_defrag does not free the mbuf on failure
because in the drivers the mbuf is most probably still sitting on the
interace output queue and will be retried later on.
This is by design. wg(4) abused m_defrag for something else and its error
handling was just wrong.
> diff --git sys/arch/octeon/dev/if_cnmac.c sys/arch/octeon/dev/if_cnmac.c
> index c45503c3d7f..c6a12d20a9a 100644
> --- sys/arch/octeon/dev/if_cnmac.c
> +++ sys/arch/octeon/dev/if_cnmac.c
> @@ -755,8 +755,10 @@ cnmac_send_makecmd_gbuf(struct cnmac_softc *sc, struct
> mbuf *m0,
> return 0;
>
> defrag:
> - if (m_defrag(m0, M_DONTWAIT) != 0)
> + if (m_defrag(m0, M_DONTWAIT) != 0) {
> + m_freem(m0);
> return 1;
> + }
> gbuf[0] = cnmac_send_makecmd_w1(m0->m_len, KVTOPHYS(m0->m_data));
> *rsegs = 1;
> return 0;
> diff --git sys/arch/octeon/dev/if_ogx.c sys/arch/octeon/dev/if_ogx.c
> index 13a9cd32a7d..67695fabb23 100644
> --- sys/arch/octeon/dev/if_ogx.c
> +++ sys/arch/octeon/dev/if_ogx.c
> @@ -1243,8 +1243,10 @@ ogx_send_mbuf(struct ogx_softc *sc, struct mbuf *m0)
> }
>
> if (m != NULL) {
> - if (m_defrag(m0, M_DONTWAIT) != 0)
> + if (m_defrag(m0, M_DONTWAIT) != 0) {
> + m_freem(m0);
> return ENOMEM;
> + }
>
> /* Discard previously set fragments. */
> scroff -= sizeof(word) * nfrags;
> diff --git sys/dev/fdt/if_dwge.c sys/dev/fdt/if_dwge.c
> index 58114bf3e38..e9bef31fcdc 100644
> --- sys/dev/fdt/if_dwge.c
> +++ sys/dev/fdt/if_dwge.c
> @@ -1136,8 +1136,10 @@ dwge_encap(struct dwge_softc *sc, struct mbuf *m, int
> *idx)
> map = sc->sc_txbuf[cur].tb_map;
>
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return (EFBIG);
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
> return (EFBIG);
> }
> diff --git sys/dev/fdt/if_dwxe.c sys/dev/fdt/if_dwxe.c
> index e4895a9a044..a3c6122840a 100644
> --- sys/dev/fdt/if_dwxe.c
> +++ sys/dev/fdt/if_dwxe.c
> @@ -1198,8 +1198,10 @@ dwxe_encap(struct dwxe_softc *sc, struct mbuf *m, int
> *idx)
> map = sc->sc_txbuf[cur].tb_map;
>
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return (EFBIG);
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
> return (EFBIG);
> }
> diff --git sys/dev/ic/bcmgenet.c sys/dev/ic/bcmgenet.c
> index c9b21f7768b..2444b71a68a 100644
> --- sys/dev/ic/bcmgenet.c
> +++ sys/dev/ic/bcmgenet.c
> @@ -215,15 +215,19 @@ genet_setup_txbuf(struct genet_softc *sc, int index,
> struct mbuf *m)
> * than the minimum Ethernet packet size.
> */
> if (m->m_len < ETHER_MIN_LEN - ETHER_CRC_LEN) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return 0;
> + }
> }
>
> error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
> sc->sc_tx.buf_map[index].map, m, BUS_DMA_WRITE | BUS_DMA_NOWAIT);
> if (error == EFBIG) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return 0;
> + }
> error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
> sc->sc_tx.buf_map[index].map, m,
> BUS_DMA_WRITE | BUS_DMA_NOWAIT);
> diff --git sys/dev/ic/gem.c sys/dev/ic/gem.c
> index 0cd66dbfd63..b5ef3eca8da 100644
> --- sys/dev/ic/gem.c
> +++ sys/dev/ic/gem.c
> @@ -1698,6 +1698,7 @@ gem_load_mbuf(struct gem_softc *sc, struct gem_sxd *sd,
> struct mbuf *m)
> break;
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (1);
> }
>
> diff --git sys/dev/ic/re.c sys/dev/ic/re.c
> index 78eb8e07843..c6522d656b5 100644
> --- sys/dev/ic/re.c
> +++ sys/dev/ic/re.c
> @@ -1650,6 +1650,7 @@ re_encap(struct rl_softc *sc, unsigned int idx, struct
> mbuf *m)
>
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (0);
> }
>
> diff --git sys/dev/ic/rt2860.c sys/dev/ic/rt2860.c
> index fa9b12e7275..2d0d7fe99ee 100644
> --- sys/dev/ic/rt2860.c
> +++ sys/dev/ic/rt2860.c
> @@ -1660,8 +1660,10 @@ rt2860_tx(struct rt2860_softc *sc, struct mbuf *m,
> struct ieee80211_node *ni)
> KASSERT (ring->queued <= RT2860_TX_RING_ONEMORE); /* <1> */
>
> if (bus_dmamap_load_mbuf(sc->sc_dmat, data->map, m, BUS_DMA_NOWAIT)) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return (ENOBUFS);
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat,
> data->map, m, BUS_DMA_NOWAIT))
> return (EFBIG);
> diff --git sys/dev/pci/if_bge.c sys/dev/pci/if_bge.c
> index 7cc69d902b9..a8558a841c9 100644
> --- sys/dev/pci/if_bge.c
> +++ sys/dev/pci/if_bge.c
> @@ -4087,6 +4087,7 @@ doit:
>
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (ENOBUFS);
> }
>
> diff --git sys/dev/pci/if_bnx.c sys/dev/pci/if_bnx.c
> index 771735cb308..7c85321e65d 100644
> --- sys/dev/pci/if_bnx.c
> +++ sys/dev/pci/if_bnx.c
> @@ -4791,6 +4791,7 @@ bnx_tx_encap(struct bnx_softc *sc, struct mbuf *m, int
> *used)
> /* FALLTHROUGH */
> default:
> sc->tx_dma_map_failures++;
> + m_freem(m);
> return (ENOBUFS);
> }
>
> diff --git sys/dev/pci/if_bnxt.c sys/dev/pci/if_bnxt.c
> index 3bbff64ee14..09700cf74e4 100644
> --- sys/dev/pci/if_bnxt.c
> +++ sys/dev/pci/if_bnxt.c
> @@ -1088,6 +1088,7 @@ bnxt_load_mbuf(struct bnxt_softc *sc, struct bnxt_slot
> *bs, struct mbuf *m)
> break;
>
> default:
> + m_freem(m);
> return (1);
> }
>
> diff --git sys/dev/pci/if_bwfm_pci.c sys/dev/pci/if_bwfm_pci.c
> index b930011656c..c6a25cc5b91 100644
> --- sys/dev/pci/if_bwfm_pci.c
> +++ sys/dev/pci/if_bwfm_pci.c
> @@ -899,8 +899,10 @@ bwfm_pci_pktid_new(struct bwfm_pci_softc *sc, struct
> bwfm_pci_pkts *pkts,
> if (pkts->pkts[idx].bb_m == NULL) {
> if (bus_dmamap_load_mbuf(sc->sc_dmat,
> pkts->pkts[idx].bb_map, m, BUS_DMA_NOWAIT) != 0) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return EFBIG;
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat,
> pkts->pkts[idx].bb_map, m, BUS_DMA_NOWAIT)
> != 0)
> return EFBIG;
> diff --git sys/dev/pci/if_cas.c sys/dev/pci/if_cas.c
> index 8172e35f5f0..b11cfb7e410 100644
> --- sys/dev/pci/if_cas.c
> +++ sys/dev/pci/if_cas.c
> @@ -1789,6 +1789,7 @@ cas_encap(struct cas_softc *sc, struct mbuf *m, int
> *used)
> break;
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (ENOBUFS);
> }
>
> diff --git sys/dev/pci/if_em.c sys/dev/pci/if_em.c
> index ae0524756bf..8afa7f15d20 100644
> --- sys/dev/pci/if_em.c
> +++ sys/dev/pci/if_em.c
> @@ -1182,6 +1182,7 @@ em_encap(struct em_queue *que, struct mbuf *m)
> /* FALLTHROUGH */
> default:
> sc->no_tx_dma_setup++;
> + m_freem(m);
> return (0);
> }
>
> diff --git sys/dev/pci/if_iavf.c sys/dev/pci/if_iavf.c
> index fc2fb49a6ae..dc1f810c1c5 100644
> --- sys/dev/pci/if_iavf.c
> +++ sys/dev/pci/if_iavf.c
> @@ -1651,8 +1651,10 @@ iavf_load_mbuf(bus_dma_tag_t dmat, bus_dmamap_t map,
> struct mbuf *m)
> return (error);
>
> error = m_defrag(m, M_DONTWAIT);
> - if (error != 0)
> + if (error != 0) {
> + m_freem(m);
> return (error);
> + }
>
> return (bus_dmamap_load_mbuf(dmat, map, m,
> BUS_DMA_STREAMING | BUS_DMA_NOWAIT));
> diff --git sys/dev/pci/if_ix.c sys/dev/pci/if_ix.c
> index c40dbcdb83b..f25fd48974c 100644
> --- sys/dev/pci/if_ix.c
> +++ sys/dev/pci/if_ix.c
> @@ -1405,6 +1405,7 @@ ixgbe_encap(struct tx_ring *txr, struct mbuf *m_head)
> break;
> /* FALLTHROUGH */
> default:
> + m_freem(m_head);
> return (0);
> }
>
> diff --git sys/dev/pci/if_ixl.c sys/dev/pci/if_ixl.c
> index 35c5545f8d9..e908f5bb50e 100644
> --- sys/dev/pci/if_ixl.c
> +++ sys/dev/pci/if_ixl.c
> @@ -2740,8 +2740,10 @@ ixl_load_mbuf(bus_dma_tag_t dmat, bus_dmamap_t map,
> struct mbuf *m)
> return (error);
>
> error = m_defrag(m, M_DONTWAIT);
> - if (error != 0)
> + if (error != 0) {
> + m_freem(m);
> return (error);
> + }
>
> return (bus_dmamap_load_mbuf(dmat, map, m,
> BUS_DMA_STREAMING | BUS_DMA_NOWAIT));
> diff --git sys/dev/pci/if_mcx.c sys/dev/pci/if_mcx.c
> index 8a00f532f64..b2428102f9a 100644
> --- sys/dev/pci/if_mcx.c
> +++ sys/dev/pci/if_mcx.c
> @@ -7393,6 +7393,7 @@ mcx_load_mbuf(struct mcx_softc *sc, struct mcx_slot
> *ms, struct mbuf *m)
> break;
>
> default:
> + m_freem(m);
> return (1);
> }
>
> diff --git sys/dev/pci/if_msk.c sys/dev/pci/if_msk.c
> index 371d3b5b920..9e585155cbc 100644
> --- sys/dev/pci/if_msk.c
> +++ sys/dev/pci/if_msk.c
> @@ -1451,6 +1451,7 @@ msk_encap(struct sk_if_softc *sc_if, struct mbuf *m,
> uint32_t prod)
> break;
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (0);
> }
>
> diff --git sys/dev/pci/if_myx.c sys/dev/pci/if_myx.c
> index 8c486140ed8..22f0cc6c262 100644
> --- sys/dev/pci/if_myx.c
> +++ sys/dev/pci/if_myx.c
> @@ -1630,6 +1630,7 @@ myx_load_mbuf(struct myx_softc *sc, struct myx_slot
> *ms, struct mbuf *m)
> BUS_DMA_STREAMING | BUS_DMA_NOWAIT) == 0)
> break;
> default:
> + m_freem(m);
> return (1);
> }
>
> diff --git sys/dev/pci/if_rge.c sys/dev/pci/if_rge.c
> index e89f7bf52ab..2198a569c72 100644
> --- sys/dev/pci/if_rge.c
> +++ sys/dev/pci/if_rge.c
> @@ -403,6 +403,7 @@ rge_encap(struct rge_softc *sc, struct mbuf *m, int idx)
>
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (0);
> }
>
> diff --git sys/dev/pci/if_se.c sys/dev/pci/if_se.c
> index 73b94d1934b..17229bbfa32 100644
> --- sys/dev/pci/if_se.c
> +++ sys/dev/pci/if_se.c
> @@ -1129,6 +1129,7 @@ se_encap(struct se_softc *sc, struct mbuf *m_head,
> uint32_t *txidx)
> if (ifp->if_flags & IFF_DEBUG)
> printf("%s: m_defrag failed\n", ifp->if_xname);
> #endif
> + m_freem(m_head);
> return ENOBUFS; /* XXX should not be fatal */
> }
>
> diff --git sys/dev/pci/if_sis.c sys/dev/pci/if_sis.c
> index 0a5bf238870..bcfd65bc4b4 100644
> --- sys/dev/pci/if_sis.c
> +++ sys/dev/pci/if_sis.c
> @@ -1619,6 +1619,7 @@ sis_encap(struct sis_softc *sc, struct mbuf *m_head,
> u_int32_t *txidx)
>
> /* FALLTHROUGH */
> default:
> + m_freem(m_head);
> return (ENOBUFS);
> }
>
> diff --git sys/dev/pci/if_sk.c sys/dev/pci/if_sk.c
> index e7992d296ec..a513b9ae545 100644
> --- sys/dev/pci/if_sk.c
> +++ sys/dev/pci/if_sk.c
> @@ -1421,6 +1421,7 @@ sk_encap(struct sk_if_softc *sc_if, struct mbuf
> *m_head, u_int32_t *txidx)
> BUS_DMA_STREAMING | BUS_DMA_NOWAIT) == 0)
> break;
> default:
> + m_free(m_head);
> return (1);
> }
>
> diff --git sys/dev/pci/if_vge.c sys/dev/pci/if_vge.c
> index c728877cff9..fc334e9799f 100644
> --- sys/dev/pci/if_vge.c
> +++ sys/dev/pci/if_vge.c
> @@ -1340,6 +1340,7 @@ vge_encap(struct vge_softc *sc, struct mbuf *m_head,
> int idx)
> BUS_DMA_NOWAIT)) == 0)
> break;
> default:
> + m_freem(m_head);
> return (error);
> }
>
> diff --git sys/dev/pci/if_vic.c sys/dev/pci/if_vic.c
> index 80d6a1df298..89d6f775886 100644
> --- sys/dev/pci/if_vic.c
> +++ sys/dev/pci/if_vic.c
> @@ -1139,6 +1139,7 @@ vic_load_txb(struct vic_softc *sc, struct vic_txbuf
> *txb, struct mbuf *m)
>
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (ENOBUFS);
> }
>
> diff --git sys/dev/pci/if_vmx.c sys/dev/pci/if_vmx.c
> index f2dc04193b3..cbb91f14c5e 100644
> --- sys/dev/pci/if_vmx.c
> +++ sys/dev/pci/if_vmx.c
> @@ -1298,8 +1298,10 @@ vmx_load_mbuf(bus_dma_tag_t dmat, bus_dmamap_t map,
> struct mbuf *m)
> return (error);
>
> error = m_defrag(m, M_DONTWAIT);
> - if (error != 0)
> + if (error != 0) {
> + m_freem(m);
> return (error);
> + }
>
> return (bus_dmamap_load_mbuf(dmat, map, m,
> BUS_DMA_STREAMING | BUS_DMA_NOWAIT));
> diff --git sys/dev/pci/if_vr.c sys/dev/pci/if_vr.c
> index c9aa6b9bce5..48cce0543b8 100644
> --- sys/dev/pci/if_vr.c
> +++ sys/dev/pci/if_vr.c
> @@ -1215,6 +1215,7 @@ vr_encap(struct vr_softc *sc, struct vr_chain **cp,
> struct mbuf *m)
>
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return (ENOBUFS);
> }
>
> diff --git sys/dev/pv/if_hvn.c sys/dev/pv/if_hvn.c
> index 0081eac5c1f..dc2245187d2 100644
> --- sys/dev/pv/if_hvn.c
> +++ sys/dev/pv/if_hvn.c
> @@ -557,6 +557,7 @@ hvn_encap(struct hvn_softc *sc, struct mbuf *m, struct
> hvn_tx_desc **txd0)
> /* FALLTHROUGH */
> default:
> DPRINTF("%s: failed to load mbuf\n", sc->sc_dev.dv_xname);
> + m_freem(m);
> return (-1);
> }
> txd->txd_buf = m;
> diff --git sys/dev/pv/if_vio.c sys/dev/pv/if_vio.c
> index 4b5e36a6eb3..8b46f51d076 100644
> --- sys/dev/pv/if_vio.c
> +++ sys/dev/pv/if_vio.c
> @@ -1182,6 +1182,7 @@ vio_encap(struct vio_softc *sc, int slot, struct mbuf
> *m)
>
> /* FALLTHROUGH */
> default:
> + m_freem(m);
> return ENOBUFS;
> }
> sc->sc_tx_mbufs[slot] = m;
> diff --git sys/dev/pv/if_xnf.c sys/dev/pv/if_xnf.c
> index c7c07193a3f..82bb0eb8435 100644
> --- sys/dev/pv/if_xnf.c
> +++ sys/dev/pv/if_xnf.c
> @@ -578,8 +578,10 @@ xnf_encap(struct xnf_softc *sc, struct mbuf *m_head,
> uint32_t *prod)
> int i, flags, n, used = 0;
>
> if ((xnf_fragcount(m_head) > sc->sc_tx_frags) &&
> - m_defrag(m_head, M_DONTWAIT))
> + m_defrag(m_head, M_DONTWAIT)) {
> + m_freem(m_head);
> return (ENOBUFS);
> + }
>
> flags = (sc->sc_domid << 16) | BUS_DMA_WRITE | BUS_DMA_NOWAIT;
>
>
--
:wq Claudio
