svn commit: r220062 - head/sys/geom/gate

[Kostik Belousov]

On Sun, Mar 27, 2011 at 11:49:15PM +0300, Mikolaj Golub wrote:
> 
> 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 at 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.

Ok, name is local variable.
Apparently, what you need is a barrier. It would be enough to do
        sc->sc_name = sc->sc_provider->name;
        mtx_lock(&g_gate_units_lock);
        mtx_unlock(&g_gate_units_lock);
The change is fine as is.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-head/attachments/20110327/6401401c/attachment.pgp