svn commit: r322338 - head/sys/net

[Gleb Smirnoff]

  Hi,

On Thu, Aug 10, 2017 at 03:43:23AM +0000, Sean Bruno wrote:
S> Author: sbruno
S> Date: Thu Aug 10 03:43:23 2017
S> New Revision: 322338
S> URL: https://svnweb.freebsd.org/changeset/base/322338
S> 
S> Log:
S>   Don't leak mbufs if clusers exceeds the number of segments.  This would
S>   leak mbufs over time causing crashes.
S>   
S>   PR:		221202
S>   Submitted by:	Matt Macy <matt at mattmacy.io>
S>   Reported by:	gergely.czuczy at harmless.hu
S>   Sponsored by:	Limelight Networks
S> 
S> Modified:
S>   head/sys/net/iflib.c
S> 
S> Modified: head/sys/net/iflib.c
S> ==============================================================================
S> --- head/sys/net/iflib.c	Thu Aug 10 03:11:05 2017	(r322337)
S> +++ head/sys/net/iflib.c	Thu Aug 10 03:43:23 2017	(r322338)
S> @@ -267,6 +267,8 @@ iflib_get_sctx(if_ctx_t ctx)
S>  #define RX_SW_DESC_INUSE        (1 << 3)
S>  #define TX_SW_DESC_MAPPED       (1 << 4)
S>  
S> +#define	M_TOOBIG		M_UNUSED_8

If you DO use something, then please don't pretent it is unused. This creates
mess, when someone else will look into sys/mbuf.h and find M_UNUSED_8 there
clearly marked as available and will start using it.

However, my reading of the change is that only packets in a TX ring are
marked with this flag. And later they are checked in the same place, and
they don't travel out of iflib. So, it is a local flag, and in this case
you don't need to grab a global mbuf flag, and take any of the M_PROTO flags.

S>  typedef struct iflib_sw_rx_desc_array {
S>  	bus_dmamap_t	*ifsd_map;         /* bus_dma maps for packet */
S>  	struct mbuf	**ifsd_m;           /* pkthdr mbufs */
S> @@ -2930,8 +2932,11 @@ iflib_busdma_load_mbuf_sg(iflib_txq_t txq, bus_dma_tag
S>  			m = m->m_next;
S>  			count++;
S>  		} while (m != NULL);
S> -		if (count > *nsegs)
S> +		if (count > *nsegs) {
S> +			ifsd_m[pidx] = *m0;
S> +			ifsd_m[pidx]->m_flags |= M_TOOBIG;
S>  			return (0);
S> +		}
S>  		m = *m0;
S>  		count = 0;
S>  		do {
S> @@ -3241,8 +3246,15 @@ iflib_tx_desc_free(iflib_txq_t txq, int n)
S>  			if ((m = ifsd_m[cidx]) != NULL) {
S>  				/* XXX we don't support any drivers that batch packets yet */
S>  				MPASS(m->m_nextpkt == NULL);
S> -
S> -				m_free(m);
S> +				/* if the number of clusters exceeds the number of segments
S> +				 * there won't be space on the ring to save a pointer to each
S> +				 * cluster so we simply free the list here
S> +				 */
S> +				if (m->m_flags & M_TOOBIG) {
S> +					m_freem(m);
S> +				} else {
S> +					m_free(m);
S> +				}
S>  				ifsd_m[cidx] = NULL;
S>  #if MEMORY_LOGGING
S>  				txq->ift_dequeued++;

Can you please explain the goal of the change? AFAIK, the problem
could be fixed with one liner:

-	m_free(m);
+	m_freem(m);

n the iflib_tx_desc_free().

-- 
Totus tuus, Glebius.