diff options
author | rwatson <rwatson@FreeBSD.org> | 2004-03-18 09:55:11 +0000 |
---|---|---|
committer | rwatson <rwatson@FreeBSD.org> | 2004-03-18 09:55:11 +0000 |
commit | 54eb0efc5356f41cbc74b87331bf4cdb339f5d3e (patch) | |
tree | 202f617bdd7c7deac528f79b343601786ef2b14c /sys/net/if_tap.c | |
parent | df09012184a8ffa4f548e23f416794105f9707d3 (diff) | |
download | FreeBSD-src-54eb0efc5356f41cbc74b87331bf4cdb339f5d3e.zip FreeBSD-src-54eb0efc5356f41cbc74b87331bf4cdb339f5d3e.tar.gz |
sAdd a comment indicating why there continues to be a race condition in
the tap driver, even with Giant over the cdev operation vector, due to
a non-atomic test-and-set of the si_drv1 field in the dev_t. This bug
exists with Giant under high memory pressure, as malloc() may sleep
in tapcreate(), but is less likely to occur. The resolution will
probably be to cover si_drv1 using the global tapmtx since no softc is
available, but I need to think about this problem more generally
across a range of drivers using si_drv1 in combination with SI_CHEAPCLONE
to defer expensive allocation to open().
Correct what appears to be a bug in the original if_tap implementation,
in which tapopen() will panic if a tap device instance is opened more
than once due to an incorrect assertion -- only triggered if INVARIANTS
is compiled in (i.e., when built into a kernel). Return EBUSY instead.
Expand mtx_lock() coverage using tp->tap_mtx to include tp->ether_addr.
Diffstat (limited to 'sys/net/if_tap.c')
-rw-r--r-- | sys/net/if_tap.c | 19 |
1 files changed, 14 insertions, 5 deletions
diff --git a/sys/net/if_tap.c b/sys/net/if_tap.c index 3daf8ec..17f371d 100644 --- a/sys/net/if_tap.c +++ b/sys/net/if_tap.c @@ -346,19 +346,24 @@ tapopen(dev, flag, mode, td) if ((dev2unit(dev) & CLONE_UNITMASK) > TAPMAXUNIT) return (ENXIO); + /* + * XXXRW: Non-atomic test-and-set of si_drv1. Currently protected + * by Giant, but the race actually exists under memory pressure as + * well even when running with Giant, as malloc() may sleep. + */ tp = dev->si_drv1; if (tp == NULL) { tapcreate(dev); tp = dev->si_drv1; } - /* Unlocked read. */ - KASSERT(!(tp->tap_flags & TAP_OPEN), - ("%s flags is out of sync", tp->tap_if.if_xname)); + mtx_lock(&tp->tap_mtx); + if (tp->tap_flags & TAP_OPEN) { + mtx_unlock(&tp->tap_mtx); + return (EBUSY); + } bcopy(tp->arpcom.ac_enaddr, tp->ether_addr, sizeof(tp->ether_addr)); - - mtx_lock(&tp->tap_mtx); tp->tap_pid = td->td_proc->p_pid; tp->tap_flags |= TAP_OPEN; mtx_unlock(&tp->tap_mtx); @@ -679,11 +684,15 @@ tapioctl(dev, cmd, data, flag, td) case OSIOCGIFADDR: /* get MAC address of the remote side */ case SIOCGIFADDR: + mtx_lock(&tp->tap_mtx); bcopy(tp->ether_addr, data, sizeof(tp->ether_addr)); + mtx_unlock(&tp->tap_mtx); break; case SIOCSIFADDR: /* set MAC address of the remote side */ + mtx_lock(&tp->tap_mtx); bcopy(data, tp->ether_addr, sizeof(tp->ether_addr)); + mtx_unlock(&tp->tap_mtx); break; default: |