From owner-svn-src-head@freebsd.org Wed Aug 16 20:43:29 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6428DDD01F1; Wed, 16 Aug 2017 20:43:29 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (glebi.us [96.95.210.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 3A33B6F331; Wed, 16 Aug 2017 20:43:28 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.15.2/8.15.2) with ESMTPS id v7GKhRfT017397 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 16 Aug 2017 13:43:27 -0700 (PDT) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebi.us (8.15.2/8.15.2/Submit) id v7GKhRLj017396; Wed, 16 Aug 2017 13:43:27 -0700 (PDT) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@FreeBSD.org using -f Date: Wed, 16 Aug 2017 13:43:27 -0700 From: Gleb Smirnoff To: Sean Bruno , Matt Macy Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r322338 - head/sys/net Message-ID: <20170816204327.GK1113@FreeBSD.org> References: <201708100343.v7A3hNwR068837@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201708100343.v7A3hNwR068837@repo.freebsd.org> User-Agent: Mutt/1.8.3 (2017-05-23) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Aug 2017 20:43:29 -0000 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 S> Reported by: gergely.czuczy@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.