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;
>  		/*
>