svn commit: r220062 - head/sys/geom/gate
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 fjwc...@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);
if (sc-sc_timeout 0) {
callout_reset(sc-sc_callout, sc-sc_timeout * hz,
Modified: head/sys/geom/gate/g_gate.h
==
--- head/sys/geom/gate/g_gate.h Sun Mar 27 19:29:18 2011(r220061)
+++ head/sys/geom/gate/g_gate.h Sun Mar 27 19:56:55 2011(r220062)
@@ -76,6 +76,7 @@
* 'P:' means 'Protected by'.
*/
struct g_gate_softc {
+ char*sc_name; /* P: (read-only) */
int sc_unit; /* P: (read-only) */
int sc_ref;/* P: g_gate_list_mtx */
struct g_provider *sc_provider; /* P: (read-only) */
@@ -96,7 +97,6 @@ struct g_gate_softc {
LIST_ENTRY(g_gate_softc) sc_next; /* P: g_gate_list_mtx */
char sc_info[G_GATE_INFOSIZE]; /* P: (read-only) */
};
-#definesc_name sc_provider-geom-name
#defineG_GATE_DEBUG(lvl, ...) do {
\
if (g_gate_debug = (lvl)) {\
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r220062 - head/sys/geom/gate
On Sun, Mar 27, 2011 at 07:56:55PM +, 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 fjwc...@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);
I think you do not need a mutex locked around the single assignment.
As I understand, sc_provider-name is constant ?
pgpWiGCrhAZuT.pgp
Description: PGP signature
Re: svn commit: r220062 - head/sys/geom/gate
On Sun, 27 Mar 2011 23:08:04 +0300 Kostik Belousov wrote:
KB On Sun, Mar 27, 2011 at 07:56:55PM +, 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 fjwc...@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.cSun Mar 27 19:29:18 2011
(r220061)
+++ head/sys/geom/gate/g_gate.cSun 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
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r220062 - head/sys/geom/gate
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 +, 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 fjwc...@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.cSun Mar 27 19:29:18 2011
(r220061)
+++ head/sys/geom/gate/g_gate.cSun 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.
pgp0Ao5ItIhA4.pgp
Description: PGP signature
