From: jan@openbsd.org Subject: Re: ix mbuf double free To: Alexander Bluhm , tech@openbsd.org Date: Thu, 7 Mar 2024 13:37:34 +0100 On Wed, Mar 06, 2024 at 03:43:27PM +0100, Claudio Jeker wrote: > On Wed, Mar 06, 2024 at 03:36:51PM +0100, Alexander Bluhm wrote: > > When ix(4) rx links an mbuf into a chain, it is also referenced by > > nxbuf->buf and may be freed later. As m_freem(rxbuf->fmp) frees > > this mbuf as part of the chain it is a double free. > > > > As claudio@ mentioned that linking an mbuf with M_PKTHDR as chain > > member is a bug, we can us this flag as indication whether we need > > an individual free of the nxbuf->buf. > > > > Unfortunatelly the error case with these free calls is not triggered, > > so I cannot test it. > > The below is my version of the same fix. This uses the fact that if fmp is > set then the mbuf in buf is part of that chain. No need for the M_PKTHDR > check. I clear the M_PKTHDR at a slightly different spot but that should > also not matter that much. The diff looks good to me and works well in my setup. ok jan@ > As bluhm@ said this code path is not hit so the problem is somewhere else. > -- > :wq Claudio > > Index: if_ix.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/if_ix.c,v > diff -u -p -r1.209 if_ix.c > --- if_ix.c 15 Feb 2024 10:56:53 -0000 1.209 > +++ if_ix.c 6 Mar 2024 10:31:25 -0000 > @@ -3172,12 +3188,12 @@ ixgbe_rxeof(struct rx_ring *rxr) > rsccnt >>= IXGBE_RXDADV_RSCCNT_SHIFT; > > if (staterr & IXGBE_RXDADV_ERR_FRAME_ERR_MASK) { > if (rxbuf->fmp) { > m_freem(rxbuf->fmp); > - rxbuf->fmp = NULL; > + } else { > + m_freem(mp); > } > - > - m_freem(mp); > + rxbuf->fmp = NULL; > rxbuf->buf = NULL; > goto next_desc; > } > @@ -3224,6 +3241,8 @@ ixgbe_rxeof(struct rx_ring *rxr) > sendmp = mp; > sendmp->m_pkthdr.len = 0; > sendmp->m_pkthdr.ph_mss = 0; > + } else { > + mp->m_flags &= ~M_PKTHDR; > } > sendmp->m_pkthdr.len += mp->m_len; > /* >