svn commit: r220062 - head/sys/geom/gate
Mikolaj Golub
trociny at freebsd.org
Sun Mar 27 20:49:19 UTC 2011
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.
--
Mikolaj Golub
More information about the svn-src-head
mailing list