Re: svn commit: r322338 - head/sys/net

[Gleb Smirnoff]

On Wed, Aug 16, 2017 at 01:43:27PM -0700, Gleb Smirnoff wrote:
T> S>   MPASS(m->m_nextpkt == NULL);
T> S> -
T> S> - m_free(m);
T> S> + /* if the number of clusters exceeds the number 
of segments
T> S> +  * there won't be space on the ring to save a 
pointer to each
T> S> +  * cluster so we simply free the list here
T> S> +  */
T> S> + if (m->m_flags & M_TOOBIG) {
T> S> + m_freem(m);
T> S> + } else {
T> S> + m_free(m);
T> S> + }
T> S>   ifsd_m[cidx] = NULL;
T> S>  #if MEMORY_LOGGING
T> S>   txq->ift_dequeued++;
T> 
T> Can you please explain the goal of the change? AFAIK, the problem
T> could be fixed with one liner:
T> 
T> -m_free(m);
T> +m_freem(m);
T> 
T> n the iflib_tx_desc_free().

I'm probably wrong on one liner, but still I don't see reason to have flag.
Since we clear m_next in normal case, doing m_freem() always is fine.

Suggested patch attached.

-- 
Totus tuus, Glebius.
Index: sys/net/iflib.c
===
--- sys/net/iflib.c	(revision 322587)
+++ sys/net/iflib.c	(working copy)
@@ -267,8 +267,6 @@ iflib_get_sctx(if_ctx_t ctx)
 #define RX_SW_DESC_INUSE(1 << 3)
 #define TX_SW_DESC_MAPPED   (1 << 4)
 
-#define	M_TOOBIG		M_UNUSED_8
-
 typedef struct iflib_sw_rx_desc_array {
 	bus_dmamap_t	*ifsd_map; /* bus_dma maps for packet */
 	struct mbuf	**ifsd_m;   /* pkthdr mbufs */
@@ -2934,7 +2932,6 @@ iflib_busdma_load_mbuf_sg(iflib_txq_t txq, bus_dma
 		} while (m != NULL);
 		if (count > *nsegs) {
 			ifsd_m[pidx] = *m0;
-			ifsd_m[pidx]->m_flags |= M_TOOBIG;
 			return (0);
 		}
 		m = *m0;
@@ -3246,15 +3243,7 @@ iflib_tx_desc_free(iflib_txq_t txq, int n)
 			if ((m = ifsd_m[cidx]) != NULL) {
 /* XXX we don't support any drivers that batch packets yet */
 MPASS(m->m_nextpkt == NULL);
-/* if the number of clusters exceeds the number of segments
- * there won't be space on the ring to save a pointer to each
- * cluster so we simply free the list here
- */
-if (m->m_flags & M_TOOBIG) {
-	m_freem(m);
-} else {
-	m_free(m);
-}
+m_freem(m);
 ifsd_m[cidx] = NULL;
 #if MEMORY_LOGGING
 txq->ift_dequeued++;
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r322338 - head/sys/net

[Gleb Smirnoff]

  Hi,

On Thu, Aug 10, 2017 at 03:43:23AM +, 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 
S>   Reported by:   gergely.czu...@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_TOOBIGM_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.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r322338 - head/sys/net

[Sean Bruno]

Author: sbruno
Date: Thu Aug 10 03:43:23 2017
New Revision: 322338
URL: https://svnweb.freebsd.org/changeset/base/322338

Log:
  Don't leak mbufs if clusers exceeds the number of segments.  This would
  leak mbufs over time causing crashes.
  
  PR:   221202
  Submitted by: Matt Macy 
  Reported by:  gergely.czu...@harmless.hu
  Sponsored by: Limelight Networks

Modified:
  head/sys/net/iflib.c

Modified: head/sys/net/iflib.c
==
--- head/sys/net/iflib.cThu Aug 10 03:11:05 2017(r322337)
+++ head/sys/net/iflib.cThu Aug 10 03:43:23 2017(r322338)
@@ -267,6 +267,8 @@ iflib_get_sctx(if_ctx_t ctx)
 #define RX_SW_DESC_INUSE(1 << 3)
 #define TX_SW_DESC_MAPPED   (1 << 4)
 
+#defineM_TOOBIGM_UNUSED_8
+
 typedef struct iflib_sw_rx_desc_array {
bus_dmamap_t*ifsd_map; /* bus_dma maps for packet */
struct mbuf **ifsd_m;   /* pkthdr mbufs */
@@ -2930,8 +2932,11 @@ iflib_busdma_load_mbuf_sg(iflib_txq_t txq, bus_dma_tag
m = m->m_next;
count++;
} while (m != NULL);
-   if (count > *nsegs)
+   if (count > *nsegs) {
+   ifsd_m[pidx] = *m0;
+   ifsd_m[pidx]->m_flags |= M_TOOBIG;
return (0);
+   }
m = *m0;
count = 0;
do {
@@ -3241,8 +3246,15 @@ iflib_tx_desc_free(iflib_txq_t txq, int n)
if ((m = ifsd_m[cidx]) != NULL) {
/* XXX we don't support any drivers that batch 
packets yet */
MPASS(m->m_nextpkt == NULL);
-
-   m_free(m);
+   /* if the number of clusters exceeds the number 
of segments
+* there won't be space on the ring to save a 
pointer to each
+* cluster so we simply free the list here
+*/
+   if (m->m_flags & M_TOOBIG) {
+   m_freem(m);
+   } else {
+   m_free(m);
+   }
ifsd_m[cidx] = NULL;
 #if MEMORY_LOGGING
txq->ift_dequeued++;
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"