From rea-fbsd@codelabs.ru Wed Nov 11 17:40:18 2009 Return-Path: Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7CC27106573F; Wed, 11 Nov 2009 17:40:18 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from 0.mx.codelabs.ru (0.mx.codelabs.ru [144.206.177.45]) by mx1.freebsd.org (Postfix) with ESMTP id 09D348FC17; Wed, 11 Nov 2009 17:40:17 +0000 (UTC) Received: from shadow.codelabs.ru (cdma-92-36-76-113.msk.skylink.ru [92.36.76.113]) by 0.mx.codelabs.ru with esmtps (TLSv1:CAMELLIA256-SHA:256) id 1N8HB1-0005iy-GM; Wed, 11 Nov 2009 20:40:17 +0300 Message-Id: <20091111174011.3E7FF17182@shadow.codelabs.ru> Date: Wed, 11 Nov 2009 20:40:11 +0300 (MSK) From: Eygene Ryabinkin Reply-To: Eygene Ryabinkin To: FreeBSD-gnats-submit@freebsd.org Cc: thompsa@freebsd.org, hps@freebsd.org, scottl@freebsd.org, pjd@freebsd.org, freebsd-current@freebsd.org Subject: [patch] allow boot-time attachment of daX devices to GEOM_ELI X-Send-Pr-Version: 3.113 X-GNATS-Notify: >Number: 140477 >Category: usb >Synopsis: [umass] [usb8] [patch] allow boot-time attachment of daX devices to GEOM_ELI >Confidential: no >Severity: serious >Priority: medium >Responsible: freebsd-usb >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Wed Nov 11 17:50:01 UTC 2009 >Closed-Date: >Last-Modified: Mon Dec 14 02:48:09 UTC 2009 >Originator: Eygene Ryabinkin >Release: FreeBSD 9.0-CURRENT amd64 >Organization: Code Labs >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 --- >Release-Note: >Audit-Trail: Responsible-Changed-From-To: freebsd-bugs->freebsd-usb Responsible-Changed-By: linimon Responsible-Changed-When: Wed Nov 11 17:59:57 UTC 2009 Responsible-Changed-Why: The patch affects both usb and scsi, so it will need to be reviewed by multiple people, but just pick usb to start with. http://www.freebsd.org/cgi/query-pr.cgi?pr=140477 From: Scott Long To: FreeBSD-gnats-submit@freebsd.org Cc: Subject: Re: usb/140477: [patch] allow boot-time attachment of daX devices to GEOM_ELI Date: Wed, 11 Nov 2009 11:37:55 -0700 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 From: "Bjoern A. Zeeb" To: bug-followup@FreeBSD.org, rea-fbsd@codelabs.ru Cc: Subject: Re: usb/140477: [umass] [patch] allow boot-time attachment of daX devices to GEOM_ELI Date: Thu, 12 Nov 2009 07:28:03 +0000 (UTC) Hi, has this changed recently, that it no longer works? I seem to remember that it had perfectly worked before this year. /bz -- Bjoern A. Zeeb It will not break if you know what you are doing. From: Eygene Ryabinkin To: "Bjoern A. Zeeb" Cc: bug-followup@FreeBSD.org Subject: Re: usb/140477: [umass] [patch] allow boot-time attachment of daX devices to GEOM_ELI Date: Thu, 12 Nov 2009 12:11:17 +0300 Bjoern, good day. Thu, Nov 12, 2009 at 07:28:03AM +0000, Bjoern A. Zeeb wrote: > has this changed recently, that it no longer works? I seem to > remember that it had perfectly worked before this year. Yes, it used to work with up to 7.. But it seems that with the new USB stack we have asynchronous discovery and attachment, so other subsystems are started when this process isn't yet fully completed, so root mount is getting "closer" and, for my case, root mount typically waits only for the completion of USB tasks. My gut feeling is that the device discovery prior to the USBv2 was done synchronously, but with USBv2 kernel use async callbacks. Though, I may be wrong in this judgement. You can try it yourself -- plugged USB stick with geli volume that is marked as attach-on-boot should show the current behaviour. -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ # From: "Bjoern A. Zeeb" To: Eygene Ryabinkin Cc: bug-followup@FreeBSD.org Subject: Re: usb/140477: [umass] [patch] allow boot-time attachment of daX devices to GEOM_ELI Date: Thu, 12 Nov 2009 10:43:56 +0000 (UTC) On Thu, 12 Nov 2009, Eygene Ryabinkin wrote: Hi, > Bjoern, good day. > > Thu, Nov 12, 2009 at 07:28:03AM +0000, Bjoern A. Zeeb wrote: >> has this changed recently, that it no longer works? I seem to >> remember that it had perfectly worked before this year. > > Yes, it used to work with up to 7.. But it seems that with > the new USB stack we have asynchronous discovery and attachment, so > other subsystems are started when this process isn't yet fully > completed, so root mount is getting "closer" and, for my case, root > mount typically waits only for the completion of USB tasks. > > My gut feeling is that the device discovery prior to the USBv2 was > done synchronously, but with USBv2 kernel use async callbacks. Though, > I may be wrong in this judgement. > > You can try it yourself -- plugged USB stick with geli volume that is > marked as attach-on-boot should show the current behaviour. I am doing that regularly, on HEAD (last updated in October). But I see I lacked coffee this morning, wanted to say "earlier this year". -- Bjoern A. Zeeb It will not break if you know what you are doing. From: Eygene Ryabinkin To: "Bjoern A. Zeeb" Cc: bug-followup@FreeBSD.org Subject: Re: usb/140477: [umass] [patch] allow boot-time attachment of daX devices to GEOM_ELI Date: Thu, 12 Nov 2009 16:29:54 +0300 Thu, Nov 12, 2009 at 10:43:56AM +0000, Bjoern A. Zeeb wrote: > I am doing that regularly, on HEAD (last updated in October). But I > see I lacked coffee this morning, wanted to say "earlier this year". Then it will be interesting to see on which point your kernel is attaching the daX devices. Could you, please, show your dmesg? -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ # >Unformatted: