From owner-freebsd-current@FreeBSD.ORG Wed Nov 11 18:38:22 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E292C106566B; Wed, 11 Nov 2009 18:38:22 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.freebsd.org (Postfix) with ESMTP id 7EC488FC0A; Wed, 11 Nov 2009 18:38:22 +0000 (UTC) Received: from [IPv6:::1] (pooker.samsco.org [168.103.85.57]) (authenticated bits=0) by pooker.samsco.org (8.14.2/8.14.2) with ESMTP id nABIbuSU091118; Wed, 11 Nov 2009 11:37:56 -0700 (MST) (envelope-from scottl@samsco.org) Mime-Version: 1.0 (Apple Message framework v1076) Content-Type: text/plain; charset=us-ascii; format=flowed; delsp=yes From: Scott Long In-Reply-To: <20091111174011.3E7FF17182@shadow.codelabs.ru> Date: Wed, 11 Nov 2009 11:37:55 -0700 Content-Transfer-Encoding: 7bit Message-Id: <017A1314-C5F2-4A71-857A-A381F029540B@samsco.org> References: <20091111174011.3E7FF17182@shadow.codelabs.ru> To: Eygene Ryabinkin X-Mailer: Apple Mail (2.1076) X-Spam-Status: No, score=-4.3 required=3.8 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.8 X-Spam-Checker-Version: SpamAssassin 3.1.8 (2007-02-13) on pooker.samsco.org Cc: pjd@freebsd.org, scottl Long , hps@freebsd.org, FreeBSD-Current Mailing List , thompsa@freebsd.org, FreeBSD-gnats-submit@freebsd.org Subject: Re: [patch] allow boot-time attachment of daX devices to GEOM_ELI X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Nov 2009 18:38:23 -0000 I understand your concern about G_ELI. I'm not a fan of root_mount_hold, and I'd really like it to go away in favor of the SYSINIT and INTRHOOK mechanisms that already existed before root_mount_hold was introduced. It's really a hack, and a messy one that requires extensive modification to the system to work; i.e. root_mount_hold calls need to be added to just about every storage driver, not just 'ad' and 'da', while the existing SYSINIT based ordering does not. I'll look into this and see if I can come up with an alternate solution. Scott On Nov 11, 2009, at 10:40 AM, Eygene Ryabinkin wrote: > >> Submitter-Id: current-users >> Originator: Eygene Ryabinkin >> Organization: Code Labs >> Confidential: no >> Synopsis: [patch] allow boot-time attachment of daX devices to >> GEOM_ELI >> Severity: serious >> Priority: medium >> Category: kern >> Class: sw-bug >> Release: FreeBSD 9.0-CURRENT amd64 >> Environment: > > System: FreeBSD 9.0-CURRENT amd64 > >> Description: > > Currently, one can not make GEOM_ELI to attach encrypted providers > at a > boot time if these providers are backed by the da(4) devices (USB > disks > or sticks). This happens because umass(4) only pushes CAM layer to > attach the created device, but the actual attachment is done > asynchronously and on my machines it is done after the root mount. > > This makes me unable to boot my machines whose disks are removable USB > ones and all partitions (with the boot one) are lying inside GEOM_ELI > volume. > >> How-To-Repeat: > > Create GEOM_ELI volume on the removable USB stick, set boot flag on it > (geli configure -b /dev/da) and boot the machine. You > won't > be asked for the password to attach the encrypted volume on boot (at > least, this won't happen on the machines where daX will be attached > after the root mount and at least on my notebook it is true). > >> Fix: > > The following patch fixes the things both for 9-CURRENT and 8-RC2. It > uses a hacky way to pass the softc to the CAM callback, but I found no > other ways to do so and daX should drop the root mount hold only after > it will be completely attached or failed to do so. > > --- attach-umass-and-da-before-root-mount.diff begins here --- > From ced3079c3de1b07654ebd35ea80347d9f39b105e Mon Sep 17 00:00:00 2001 > From: Eygene Ryabinkin > Date: Wed, 11 Nov 2009 20:21:12 +0300 > > This allows attaching of external USB disks that carry volumes > encrypted > by GEOM_ELI, otherwise daX are probed and attached only after root > mount > and this makes impossible to use geli-backed device as the container > for > the root partition. > > Signed-off-by: Eygene Ryabinkin > --- > sys/cam/scsi/scsi_da.c | 7 ++++++ > sys/dev/usb/storage/umass.c | 48 ++++++++++++++++++++++++++++++++++ > ++++---- > 2 files changed, 50 insertions(+), 5 deletions(-) > > diff --git a/sys/cam/scsi/scsi_da.c b/sys/cam/scsi/scsi_da.c > index d05376e..0f766ed 100644 > --- a/sys/cam/scsi/scsi_da.c > +++ b/sys/cam/scsi/scsi_da.c > @@ -130,6 +130,7 @@ struct da_softc { > struct sysctl_ctx_list sysctl_ctx; > struct sysctl_oid *sysctl_tree; > struct callout sendordered_c; > + struct root_hold_token *sc_rootmount; > }; > > struct da_quirk_entry { > @@ -1166,6 +1167,8 @@ daregister(struct cam_periph *periph, void *arg) > return(CAM_REQ_CMP_ERR); > } > > + softc->sc_rootmount = root_mount_hold("scsi_da"); > + > LIST_INIT(&softc->pending_ccbs); > softc->state = DA_STATE_PROBE; > bioq_init(&softc->bio_queue); > @@ -1754,6 +1757,10 @@ dadone(struct cam_periph *periph, union ccb > *done_ccb) > * operation. > */ > xpt_release_ccb(done_ccb); > + if (softc->sc_rootmount != NULL) { > + root_mount_rel(softc->sc_rootmount); > + softc->sc_rootmount = NULL; > + } > cam_periph_unhold(periph); > return; > } > diff --git a/sys/dev/usb/storage/umass.c b/sys/dev/usb/storage/umass.c > index 18756c9..a3a973e 100644 > --- a/sys/dev/usb/storage/umass.c > +++ b/sys/dev/usb/storage/umass.c > @@ -1034,6 +1034,8 @@ struct umass_softc { > uint8_t sc_maxlun; /* maximum LUN number, inclusive */ > uint8_t sc_last_xfer_index; > uint8_t sc_status_try; > + > + struct root_hold_token *sc_rootmount; > }; > > struct umass_probe_proto { > @@ -1043,6 +1045,15 @@ struct umass_probe_proto { > int32_t error; > }; > > +/* > + * Wrapped 'union ccb *' to "pass" 'struct umass_softc *' > + * into the CAM callback. > + */ > +struct wrapped_ccb_ptr { > + union ccb ccb; > + struct umass_softc *sc; > +}; > + > /* prototypes */ > > static device_probe_t umass_probe; > @@ -1502,6 +1513,13 @@ umass_attach(device_t dev) > sc->sc_quirks = temp.quirks; > sc->sc_unit = device_get_unit(dev); > > + /* > + * We will release rootmount for this device only when > + * it will be attached to the SCSI bus or will be > + * failed to do so. This is done inside rescan_callback. > + */ > + sc->sc_rootmount = root_mount_hold(device_get_nameunit(dev)); > + > snprintf(sc->sc_name, sizeof(sc->sc_name), > "%s", device_get_nameunit(dev)); > > @@ -1646,6 +1664,10 @@ umass_attach(device_t dev) > return (0); /* success */ > > detach: > + if (sc->sc_rootmount != NULL) { > + root_mount_rel(sc->sc_rootmount); > + sc->sc_rootmount = NULL; > + } > umass_detach(dev); > return (ENXIO); /* failure */ > } > @@ -2745,12 +2767,18 @@ umass_cam_attach_sim(struct umass_softc *sc) > return (0); > } > > +/* > + * "Wrapped" ccb will be passed to this callback: second argument > + * will really point to 'struct wrapped_ccb_ptr *', so we can > + * cast it. > + */ > static void > umass_cam_rescan_callback(struct cam_periph *periph, union ccb *ccb) > { > -#if USB_DEBUG > - struct umass_softc *sc = NULL; > + struct wrapped_ccb_ptr *wccb = (struct wrapped_ccb_ptr *)ccb; > + struct umass_softc *sc = wccb->sc; > > +#if USB_DEBUG > if (ccb->ccb_h.status != CAM_REQ_CMP) { > DPRINTF(sc, UDMASS_SCSI, "%s:%d Rescan failed, 0x%04x\n", > periph->periph_name, periph->unit_number, > @@ -2761,6 +2789,11 @@ umass_cam_rescan_callback(struct cam_periph > *periph, union ccb *ccb) > } > #endif > > + if (sc->sc_rootmount != NULL) { > + root_mount_rel(sc->sc_rootmount); > + sc->sc_rootmount = NULL; > + } > + > xpt_free_path(ccb->ccb_h.path); > free(ccb, M_USBDEV); > } > @@ -2769,6 +2802,7 @@ static void > umass_cam_rescan(struct umass_softc *sc) > { > struct cam_path *path; > + struct wrapped_ccb_ptr *wccb; > union ccb *ccb; > > DPRINTF(sc, UDMASS_SCSI, "scbus%d: scanning for %d:%d:%d\n", > @@ -2776,11 +2810,15 @@ umass_cam_rescan(struct umass_softc *sc) > cam_sim_path(sc->sc_sim), > sc->sc_unit, CAM_LUN_WILDCARD); > > - ccb = malloc(sizeof(*ccb), M_USBDEV, M_WAITOK | M_ZERO); > + wccb = malloc(sizeof(*wccb), M_USBDEV, M_WAITOK | M_ZERO); > > - if (ccb == NULL) { > + if (wccb == NULL) { > return; > } > + > + ccb = &wccb->ccb; > + wccb->sc = sc; > + > #if (__FreeBSD_version >= 700037) > mtx_lock(&sc->sc_mtx); > #endif > @@ -2791,7 +2829,7 @@ umass_cam_rescan(struct umass_softc *sc) > #if (__FreeBSD_version >= 700037) > mtx_unlock(&sc->sc_mtx); > #endif > - free(ccb, M_USBDEV); > + free(wccb, M_USBDEV); > return; > } > xpt_setup_ccb(&ccb->ccb_h, path, 5 /* priority (low) */ ); > -- > 1.6.4.3 > --- attach-umass-and-da-before-root-mount.diff ends here ---