From owner-freebsd-net Mon Jun 3 16:15:23 2002 Delivered-To: freebsd-net@freebsd.org Received: from InterJet.dellroad.org (adsl-63-194-81-26.dsl.snfc21.pacbell.net [63.194.81.26]) by hub.freebsd.org (Postfix) with ESMTP id D1D9D37B401 for ; Mon, 3 Jun 2002 16:15:04 -0700 (PDT) Received: from arch20m.dellroad.org (arch20m.dellroad.org [10.1.1.20]) by InterJet.dellroad.org (8.9.1a/8.9.1) with ESMTP id PAA81290; Mon, 3 Jun 2002 15:55:08 -0700 (PDT) Received: (from archie@localhost) by arch20m.dellroad.org (8.11.6/8.11.6) id g53Ms2O48132; Mon, 3 Jun 2002 15:54:02 -0700 (PDT) (envelope-from archie) From: Archie Cobbs Message-Id: <200206032254.g53Ms2O48132@arch20m.dellroad.org> Subject: Re: Race condition with M_EXT ref count? In-Reply-To: "from Julian Elischer at Jun 3, 2002 03:08:01 pm" To: Julian Elischer Date: Mon, 3 Jun 2002 15:54:02 -0700 (PDT) Cc: Archie Cobbs , Alfred Perlstein , freebsd-net@freebsd.org X-Mailer: ELM [version 2.4ME+ PL88 (25)] MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org Julian Elischer writes: > well, it's a valid problem form the point of view of some interrupt code > changing the ext reference at teh same time that some other code is > planning on incrementing it, but while Giant is in place it's not > likely to affect anything.. > > I presume you are talking about 4.x however right? Yes (see original email). > we can presume that each manipulator has a reference so it's not going to > GA AWAY due to a zero reference being reached, so the link can be followed > safely, which just elaves the atomicity of the addition operation. > > who are the contenders? > 1/ network mid-level code? (protected by: splnet and the BGL. > Only one cpu at a time.. > > 2/ Interrupt code running at splimp > probably freeing stuff after transmit. (receivers should have > pre-allocated buffers) > > it IS possible that there could need to be an splimp() added but I am > not clear on what part the 4.4 BGL (Big Giant Lock) plays in this.. > it may make it safe... If you don't mind, I'll let you deal with the -current case, since I'm not familiar enough with -current locking. Below is my proposed patch for -stable. It should be clear that in -stable at least, there can be a race between an interrupt handler that free's a cluster mbuf and mid-level code that is free'ing the same mbuf, for example. -Archie __________________________________________________________________________ Archie Cobbs * Packet Design * http://www.packetdesign.com --- sys/kern/uipc_mbuf.c.orig Mon Jun 3 15:43:25 2002 +++ sys/kern/uipc_mbuf.c Mon Jun 3 15:52:28 2002 @@ -707,11 +707,16 @@ n->m_len = min(len, m->m_len - off); if (m->m_flags & M_EXT) { n->m_data = m->m_data + off; - if(!m->m_ext.ext_ref) - mclrefcnt[mtocl(m->m_ext.ext_buf)]++; - else - (*(m->m_ext.ext_ref))(m->m_ext.ext_buf, - m->m_ext.ext_size); + if (m->m_ext.ext_ref == NULL) { + atomic_add_char( + &mclrefcnt[mtocl(m->m_ext.ext_buf)], 1); + } else { + int s = splimp(); + + (*m->m_ext.ext_ref)(m->m_ext.ext_buf, + m->m_ext.ext_size); + splx(s); + } n->m_ext = m->m_ext; n->m_flags |= M_EXT; } else @@ -754,11 +759,15 @@ n->m_len = m->m_len; if (m->m_flags & M_EXT) { n->m_data = m->m_data; - if(!m->m_ext.ext_ref) - mclrefcnt[mtocl(m->m_ext.ext_buf)]++; - else - (*(m->m_ext.ext_ref))(m->m_ext.ext_buf, - m->m_ext.ext_size); + if (m->m_ext.ext_ref == NULL) + atomic_add_char(&mclrefcnt[mtocl(m->m_ext.ext_buf)], 1); + else { + int s = splimp(); + + (*m->m_ext.ext_ref)(m->m_ext.ext_buf, + m->m_ext.ext_size); + splx(s); + } n->m_ext = m->m_ext; n->m_flags |= M_EXT; } else { @@ -777,11 +786,16 @@ n->m_len = m->m_len; if (m->m_flags & M_EXT) { n->m_data = m->m_data; - if(!m->m_ext.ext_ref) - mclrefcnt[mtocl(m->m_ext.ext_buf)]++; - else - (*(m->m_ext.ext_ref))(m->m_ext.ext_buf, - m->m_ext.ext_size); + if (m->m_ext.ext_ref == NULL) { + atomic_add_char( + &mclrefcnt[mtocl(m->m_ext.ext_buf)], 1); + } else { + int s = splimp(); + + (*m->m_ext.ext_ref)(m->m_ext.ext_buf, + m->m_ext.ext_size); + splx(s); + } n->m_ext = m->m_ext; n->m_flags |= M_EXT; } else { @@ -1125,11 +1139,15 @@ if (m->m_flags & M_EXT) { n->m_flags |= M_EXT; n->m_ext = m->m_ext; - if(!m->m_ext.ext_ref) - mclrefcnt[mtocl(m->m_ext.ext_buf)]++; - else - (*(m->m_ext.ext_ref))(m->m_ext.ext_buf, - m->m_ext.ext_size); + if (m->m_ext.ext_ref == NULL) + atomic_add_char(&mclrefcnt[mtocl(m->m_ext.ext_buf)], 1); + else { + int s = splimp(); + + (*m->m_ext.ext_ref)(m->m_ext.ext_buf, + m->m_ext.ext_size); + splx(s); + } m->m_ext.ext_size = 0; /* For Accounting XXXXXX danger */ n->m_data = m->m_data + len; } else { To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message