biodone panics

[Peter Edwards]

There's a few people complaining about double-frees (and other
crashes) in biodone(), and I just banged my head off it too: They
generally look a little like this:

>  panic(c0870cfb,c1b0c4a4,c143f000,c0850ced,c0870cdf) at panic+0x127
    (where c0870cfb == "duplicate free of item %p from zone %p(%s)"
>  uma_dbg_free(c143f000,0,c1b0c4a4) at uma_dbg_free+0x110
>  uma_zfree_arg(c143f000,c1b0c4a4,0) at uma_zfree_arg+0x66
>  g_destroy_bio(c1b0c4a4) at g_destroy_bio+0x13
>  g_vfs_done(c1b0c4a4) at g_vfs_done+0x5a
>  biodone(c1b0c4a4,ca0bccc4,0,c0850cb0,1e4) at biodone+0x57
>  g_io_schedule_up(c1833300) at g_io_schedule_up+0xb5
>  g_up_procbody(0,ca0bcd38,0,c05fed08,0) at g_up_procbody+0x5a
>  fork_exit(c05fed08,0,ca0bcd38) at fork_exit+0xa0
>  fork_trampoline() at fork_trampoline+0x8

Am I just being dense, or is there a race condition in biodone():

> void
> biodone(struct bio *bp)
> {
>
>         mtx_lock(&bdonelock);
>         bp->bio_flags |= BIO_DONE;
>         if (bp->bio_done == NULL)
>                 wakeup(bp);
>         mtx_unlock(&bdonelock);
>         if (bp->bio_done != NULL)
>                 bp->bio_done(bp);
> }

the call to wakeup may set in motion some events that cause the bio to
be freed. By the time the mtx_unlock completes, "bp" could point to an
invalid bio, and the "if (bp->bio_done != NULL)" is bogus. True?
Shouldn't it be

> biodone(struct bio *bp)
> {
>         void (*done)(struct bio *);
>
>         mtx_lock(&bdonelock);
>         bp->bio_flags |= BIO_DONE;
>         done = bp->bio_done
>         if (done == NULL)
>                 wakeup(bp);
>         mtx_unlock(&bdonelock);
>         if (done != NULL)
>                 bp->bio_done(bp);
> }

Anyone agree?