Date: Mon, 3 Jun 2002 19:39:22 -0400 From: Bosko Milekic <bmilekic@unixdaemons.com> To: Archie Cobbs <archie@dellroad.org> Cc: Julian Elischer <julian@elischer.org>, Alfred Perlstein <bright@mu.org>, freebsd-net@FreeBSD.ORG Subject: Re: Race condition with M_EXT ref count? Message-ID: <20020603193922.B29296@unixdaemons.com> In-Reply-To: <200206032254.g53Ms2O48132@arch20m.dellroad.org>; from archie@dellroad.org on Mon, Jun 03, 2002 at 03:54:02PM -0700 References: <Pine.BSF.4.21.0206031500400.43857-100000@InterJet.elischer.org> <200206032254.g53Ms2O48132@arch20m.dellroad.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jun 03, 2002 at 03:54:02PM -0700, Archie Cobbs wrote: [...] > 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. -current does not have this "problem." > 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 { It would be better if the thing was in a macro to allow for consistent ref. count manipulation, like in -CURRENT. But whatever, I don't really care. -- Bosko Milekic bmilekic@unixdaemons.com bmilekic@FreeBSD.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020603193922.B29296>