From owner-freebsd-geom@FreeBSD.ORG Mon Oct 6 22:53:16 2014 Return-Path: Delivered-To: freebsd-geom@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id AE1B2DAD for ; Mon, 6 Oct 2014 22:53:16 +0000 (UTC) Received: from mailuogwhop.emc.com (mailuogwhop.emc.com [168.159.213.141]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mailuogwprd01.lss.emc.com", Issuer "RSA Corporate Server CA v2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 5432983E for ; Mon, 6 Oct 2014 22:53:13 +0000 (UTC) Received: from maildlpprd04.lss.emc.com (maildlpprd04.lss.emc.com [10.253.24.36]) by mailuogwprd02.lss.emc.com (Sentrion-MTA-4.3.0/Sentrion-MTA-4.3.0) with ESMTP id s96MrAjJ020987 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 6 Oct 2014 18:53:10 -0400 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd02.lss.emc.com s96MrAjJ020987 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=isilon.com; s=jan2013; t=1412635990; bh=rChxoF2GK37ePl8krHQNokGITXY=; h=From:To:CC:Subject:Date:Message-ID:Content-Type: Content-Transfer-Encoding:MIME-Version; b=vC70JxPdVBX8Qi8C7NF706e7zQjjzlHkdno1H0/HWTE3AH+esT2TJIvo41ewMydlt CPmuMxNQXVyEEra8Hizzou+rmzDSIVtxaZ4SND3RIogI03EQD4wouwTuIr67ie48V+ dFI4sjgY4QNJ5wx8nOsI+qfouIu9hX54PJvByyCs= X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd02.lss.emc.com s96MrAjJ020987 Received: from mailusrhubprd52.lss.emc.com (mailusrhubprd52.lss.emc.com [10.106.48.25]) by maildlpprd04.lss.emc.com (RSA Interceptor) for ; Mon, 6 Oct 2014 18:52:58 -0400 Received: from mxhub14.corp.emc.com (mxhub14.corp.emc.com [128.222.70.235]) by mailusrhubprd52.lss.emc.com (Sentrion-MTA-4.3.0/Sentrion-MTA-4.3.0) with ESMTP id s96Mr3bM016332 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL) for ; Mon, 6 Oct 2014 18:53:04 -0400 Received: from MXHUB102.corp.emc.com (10.253.58.15) by mxhub14.corp.emc.com (128.222.70.235) with Microsoft SMTP Server (TLS) id 8.3.327.1; Mon, 6 Oct 2014 18:53:03 -0400 Received: from MX104CL02.corp.emc.com ([169.254.8.131]) by MXHUB102.corp.emc.com ([::1]) with mapi id 14.03.0195.001; Mon, 6 Oct 2014 18:53:03 -0400 From: "Laier, Max" To: "freebsd-geom@FreeBSD.org" Subject: biodone vs geom_up Thread-Topic: biodone vs geom_up Thread-Index: Ac/htbeD2cGqObefQeSVPGmnhfYE1Q== Date: Mon, 6 Oct 2014 22:53:01 +0000 Message-ID: <44A3D2875585FB4C857FA60A14A16C895C6ABE38@MX104CL02.corp.emc.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.7.176.195] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Sentrion-Hostname: mailusrhubprd52.lss.emc.com X-RSA-Classifications: public Cc: "Rice, Benno" , "Ferris, Scott" X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Oct 2014 22:53:16 -0000 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= From owner-freebsd-geom@FreeBSD.ORG Tue Oct 7 18:53:11 2014 Return-Path: Delivered-To: freebsd-geom@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5AA53D4 for ; Tue, 7 Oct 2014 18:53:11 +0000 (UTC) Received: from mail-la0-x230.google.com (mail-la0-x230.google.com [IPv6:2a00:1450:4010:c03::230]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id D4DE5E41 for ; Tue, 7 Oct 2014 18:53:10 +0000 (UTC) Received: by mail-la0-f48.google.com with SMTP id gi9so6851205lab.7 for ; Tue, 07 Oct 2014 11:53:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :content-type:content-transfer-encoding; bh=+utLiGuQQvrZ8xSXHCaktV0O7Mqwffmp+FSlbUx3u+k=; b=mnY9wO3NMNOcanUXL9CjggLFxOrT368ZdM0eNqZxlqBtd8oGRuMeGXstcgXADjBaR7 99Lcw9EZxoNLuj8McwvE/DB2fLrCBMzpzcNm6lQUamzRPjaR41C2SMZftW8jwR1MGvZb f7DOyFLJ2oQRSYFep/roduhEcEZ0x/M6YHgbVd0MzA6haW8aVeyiwFOEaSgI+iuM+uJZ MQWul9W0qFGV0NEVPmscRxsfr1Q9+4f3xU0RGNR+a28NWaBnytswuoTHtP8/ZP+5KhY+ UYz/lEP/EhAofKxh+Xefv6NMkEG47xdjiow3g6gvIVXSQX9oYebjbkyy3Z5sAONfTjxh Iydw== X-Received: by 10.152.5.9 with SMTP id o9mr5361919lao.95.1412707988766; Tue, 07 Oct 2014 11:53:08 -0700 (PDT) Received: from mavbook.mavhome.dp.ua ([134.249.139.101]) by mx.google.com with ESMTPSA id l13sm6962936lbh.32.2014.10.07.11.53.06 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 07 Oct 2014 11:53:07 -0700 (PDT) Sender: Alexander Motin Message-ID: <54343691.1030600@FreeBSD.org> Date: Tue, 07 Oct 2014 21:53:05 +0300 From: Alexander Motin User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: "Laier, Max" Subject: Re: biodone vs geom_up X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: "Rice, Benno" , "Ferris, Scott" , "freebsd-geom@FreeBSD.org" X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Oct 2014 18:53:11 -0000 Hi, Max. > Removing the BIO_DONE setting at [1] resolves the issue I'm seeing, but I wonder if there is a better way to resolve this? Should g_disk_start() clone a bio for its callback instead? Are there other places that set a bio_done callback on the way down? Originally requests were always cloned in g_disk_start(). Skipping BIO cloning there was my change to reduce I/O processing overhead. It is indeed kind of hack, but supposed to be a small one. I think I see the problem there too now. Removing BIO_DONE setting at [1] indeed should help. Grepping through the tree I don't see much other checks for BIO_DONE, so I would guess it should not be a problem. Any way, unlocked BIO_DONE setting there should be pointless or at least unreliable, except may be for some debugging. -- Alexander Motin