Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 6 Oct 2014 22:53:01 +0000
From:      "Laier, Max" <max.laier@isilon.com>
To:        "freebsd-geom@FreeBSD.org" <freebsd-geom@FreeBSD.org>
Cc:        "Rice, Benno" <Benno.Rice@emc.com>, "Ferris, Scott" <scott.ferris@isilon.com>
Subject:   biodone vs geom_up
Message-ID:  <44A3D2875585FB4C857FA60A14A16C895C6ABE38@MX104CL02.corp.emc.com>
next in thread | raw e-mail | index | archive | help 
It seems to me that there is a problem with biodone (particular vs. g_disk_=
start/g_disk_done_single):

void
biodone(struct bio *bp)
{
        struct mtx *mtxp;
        void (*done)(struct bio *);
        // ...
        done =3D bp->bio_done;
        if (done =3D=3D NULL) {=20
                mtxp =3D mtx_pool_find(mtxpool_sleep, bp);
                mtx_lock(mtxp);
                bp->bio_flags |=3D BIO_DONE;   // XXX [0]
                wakeup(bp);
                mtx_unlock(mtxp);
        } else {
                bp->bio_flags |=3D BIO_DONE;   // XXX [1]
                done(bp);                              // XXX [2]
        }
}

static void
g_disk_start(struct bio *bp)
{
        // ...
        case BIO_READ:
        case BIO_WRITE:
                d_maxsize =3D (bp->bio_cmd =3D=3D BIO_DELETE) ?
                    dp->d_delmaxsize : dp->d_maxsize;
                if (bp->bio_length <=3D d_maxsize) {
                        bp->bio_disk =3D dp;
                        bp->bio_to =3D (void *)bp->bio_done;
                        bp->bio_done =3D g_disk_done_single; // XXX [3]
                        // ...
                        dp->d_strategy(bp);
                        break;
        // ...
}

static void
g_disk_done_single(struct bio *bp)
{
        struct bintime now;
        struct g_disk_softc *sc;

        bp->bio_completed =3D bp->bio_length - bp->bio_resid;
        bp->bio_done =3D (void *)bp->bio_to;
        bp->bio_to =3D LIST_FIRST(&bp->bio_disk->d_geom->provider);
        // ...
        g_io_deliver(bp, bp->bio_error);   // XXX [4]
}


And a normal caller doing something like:

void *
g_read_data(struct g_consumer *cp, off_t offset, off_t length, int *error)
{
        // ...
        bp =3D g_alloc_bio();
        bp->bio_cmd =3D BIO_READ;
        bp->bio_done =3D NULL;
        g_io_request(bp, cp);
        errorc =3D biowait(bp, "gread");
        if (error !=3D NULL)
                *error =3D errorc;
        g_destroy_bio(bp);
        // ...
}

With this code, it is possible that thread 1(the i/o initiator) only gets t=
o biowait *after* the g_io_request() has already made it back, called (the =
first) biodone, set BIO_DONE at [1], and the bio is now either queued in th=
e g_bio_run_up.bio_queue, or in direct dispatch (via g_io_deliver at [4] vi=
a done(bp) [aka bio_done set at [3] called at [2]].

biowait will just return (because BIO_DONE is set), and the initiator threa=
d will g_destroy_bio while that bio is still being worked on, on the g_up t=
hread ... use-after-free.

Does anyone see a reason why we need to set BIO_DONE at [1] at all?  It see=
ms to me that the default case, where there is no callback left [0], is eno=
ugh?

Removing the BIO_DONE setting at [1] resolves the issue I'm seeing, but I w=
onder if there is a better way to resolve this?  Should g_disk_start() clon=
e a bio for its callback instead?  Are there other places that set a bio_do=
ne callback on the way down?

Any comments, questions, or answers greatly appreciated.

Thanks,
  Max=

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?44A3D2875585FB4C857FA60A14A16C895C6ABE38>