DF-5109 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 | --- a/sys/dev/raid/vinum/vinumlock.c +++ b/sys/dev/raid/vinum/vinumlock.c @@ -55,24 +55,16 @@ { int error; - /* XXX get rid of drive->flags |= VF_LOCKING; */ - if ((drive->flags & VF_LOCKED) /* it's locked */ - &&(drive->pid == curproc->p_pid)) { /* by us! */ -#ifdef VINUMDEBUG - log(LOG_WARNING, - "vinum lockdrive: already locking %s from %s:%d, called from %s:%d\n", - drive->label.name, - drive->lockfilename, - drive->lockline, - basename(file), - line); -#else - log(LOG_WARNING, - "vinum lockdrive: already locking %s\n", - drive->label.name); -#endif - return 0; - } + /* + * The old pid-based "recursion" early-return was wrong: p_pid is + * shared by every thread of a process (including the vinum daemon, + * which runs inside the VINUM_DAEMON issuer's process), so a second + * thread of the same process was admitted into the "locked" section + * while the first slept in vn_close() inside close_locked_drive(). + * Both then executed close_locked_drive() and Free(drive->freelist), + * producing double vn_close() and BADFREE2 double frees. Callers + * must simply wait; there is no legitimate recursion. + */ while ((drive->flags & VF_LOCKED) != 0) { /* * There are problems sleeping on a unique identifier, @@ -84,8 +76,10 @@ if ((error = tsleep(&lockdrive, 0, "vindrv", 0)) != 0) return error; } + crit_enter(); drive->flags |= VF_LOCKED; drive->pid = curproc->p_pid; /* it's a panic error if curproc is null */ + crit_exit(); #ifdef VINUMDEBUG bcopy(basename(file), drive->lockfilename, 15); drive->lockfilename[15] = '\0'; /* truncate if necessary */ @@ -98,8 +92,10 @@ void unlockdrive(struct drive *drive) { + crit_enter(); drive->flags &= ~VF_LOCKED; /* we don't reset pid: it's of hysterical interest */ + crit_exit(); wakeup(&lockdrive); } @@ -135,8 +131,9 @@ */ crit_enter(); +restart: /* Wait here if the table is full */ - while (plex->usedlocks == PLEX_LOCKS) /* all in use */ + while (plex->usedlocks >= PLEX_LOCKS) /* all in use (>=: drift-proof) */ tsleep(&plex->usedlocks, 0, "vlock", 0); #ifdef DIAGNOSTIC @@ -165,6 +162,13 @@ #endif plex->lockwaits++; /* waited one more time */ tsleep(lock, 0, "vrlock", 0); + /* + * The table can have filled while we slept (tsleep + * drops the MP token); a rescan finding no free slot + * would otherwise run pos off the end of the table. + */ + if (plex->usedlocks >= PLEX_LOCKS) + goto restart; lock = &plex->lock[-1]; /* start again */ foundlocks = 0; pos = NULL; @@ -180,6 +184,12 @@ */ if (pos == NULL) /* didn't find one on the way, */ pos = lock; /* use the one we're pointing to */ + if (pos >= &plex->lock[PLEX_LOCKS]) { /* defensive: filled under us */ + crit_exit(); + tsleep(&plex->usedlocks, 0, "vlock", 0); + crit_enter(); + goto restart; + } /* * The address range is free, and we're pointing @@ -203,6 +213,7 @@ struct plex *plex; plex = &PLEX[plexno]; + crit_enter(); #ifdef DIAGNOSTIC if (lock < &plex->lock[0] || lock >= &plex->lock[PLEX_LOCKS]) panic("vinum: rangelock %p on plex %d invalid, not between %p and %p", @@ -217,9 +228,10 @@ #endif lock->stripe = 0; /* no longer used */ plex->usedlocks--; /* one less lock */ - if (plex->usedlocks == PLEX_LOCKS - 1) /* we were full, */ + if (plex->usedlocks >= PLEX_LOCKS - 1) /* we were full, */ wakeup(&plex->usedlocks); /* get a waiter if one's there */ wakeup((void *) lock); + crit_exit(); } /* Get a lock for the global config, wait if it's not available */ |