Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 27 Mar 2011 23:49:15 +0300
From:      Mikolaj Golub <trociny@freebsd.org>
To:        Kostik Belousov <kostikbel@gmail.com>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r220062 - head/sys/geom/gate
Message-ID:  <86fwq8e1bo.fsf@kopusha.home.net>
In-Reply-To: <20110327200804.GM78089@deviant.kiev.zoral.com.ua> (Kostik Belousov's message of "Sun, 27 Mar 2011 23:08:04 %2B0300")
References:  <201103271956.p2RJutha067490@svn.freebsd.org> <20110327200804.GM78089@deviant.kiev.zoral.com.ua>

next in thread | previous in thread | raw e-mail | index | archive | help

On Sun, 27 Mar 2011 23:08:04 +0300 Kostik Belousov wrote:

 KB> On Sun, Mar 27, 2011 at 07:56:55PM +0000, Mikolaj Golub wrote:
 >> Author: trociny
 >> Date: Sun Mar 27 19:56:55 2011
 >> New Revision: 220062
 >> URL: http://svn.freebsd.org/changeset/base/220062
 >> 
 >> Log:
 >>   In g_gate_create() there is a window between when g_gate_softc is
 >>   registered in g_gate_units array and when its sc_provider field is
 >>   filled. If during this period g_gate_units is accessed by another
 >>   thread that is checking for provider name collision the crash is
 >>   possible.
 >>   
 >>   Fix this by adding sc_name field to struct g_gate_softc. In
 >>   g_gate_create() when g_gate_softc is created but sc_provider is still
 >>   not sc_name points to provider name stored in the local array.
 >>   
 >>   Approved by:        pjd (mentor)
 >>   Reported by:        Freddie Cash <fjwcash@gmail.com>
 >>   MFC after:        1 week
 >> 
 >> Modified:
 >>   head/sys/geom/gate/g_gate.c
 >>   head/sys/geom/gate/g_gate.h
 >> 
 >> Modified: head/sys/geom/gate/g_gate.c
 >> ==============================================================================
 >> --- head/sys/geom/gate/g_gate.c        Sun Mar 27 19:29:18 2011        (r220061)
 >> +++ head/sys/geom/gate/g_gate.c        Sun Mar 27 19:56:55 2011        (r220062)
 >> @@ -409,13 +409,14 @@ g_gate_create(struct g_gate_ctl_create *
 >>          for (unit = 0; unit < g_gate_maxunits; unit++) {
 >>                  if (g_gate_units[unit] == NULL)
 >>                          continue;
 >> -                if (strcmp(name, g_gate_units[unit]->sc_provider->name) != 0)
 >> +                if (strcmp(name, g_gate_units[unit]->sc_name) != 0)
 >>                          continue;
 >>                  mtx_unlock(&g_gate_units_lock);
 >>                  mtx_destroy(&sc->sc_queue_mtx);
 >>                  free(sc, M_GATE);
 >>                  return (EEXIST);
 >>          }
 >> +        sc->sc_name = name;
 >>          g_gate_units[sc->sc_unit] = sc;
 >>          g_gate_nunits++;
 >>          mtx_unlock(&g_gate_units_lock);
 >> @@ -434,6 +435,9 @@ g_gate_create(struct g_gate_ctl_create *
 >>          sc->sc_provider = pp;
 >>          g_error_provider(pp, 0);
 >>          g_topology_unlock();
 >> +        mtx_lock(&g_gate_units_lock);
 >> +        sc->sc_name = sc->sc_provider->name;
 >> +        mtx_unlock(&g_gate_units_lock);
 KB> I think you do not need a mutex locked around the single assignment.
 KB> As I understand, sc_provider->name is constant ?

Is the following scenario impossible?

Thread A is looking for name collision and is accessing
g_gate_units[unit]->sc_name of the unit that is being created by a thread B,
so sc_name is pointing to thread B local buffer. At this time the thread B
creates provider, does sc->sc_name = sc->sc_provider->name and returns from
g_gate_create(). Thread A, if it is still working with
g_gate_units[unit]->sc_name, is accessing invalid memory.

-- 
Mikolaj Golub



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86fwq8e1bo.fsf>