Re: [patch] allow boot-time attachment of daX devices to GEOM_ELI

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<something>) 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 <rea-fbsd_at_codelabs.ru>
> 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 <rea-fbsd_at_codelabs.ru>
> ---
> 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
> _at__at_ -130,6 +130,7 _at__at_ 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 {
> _at__at_ -1166,6 +1167,8 _at__at_ 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);
> _at__at_ -1754,6 +1757,10 _at__at_ 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
> _at__at_ -1034,6 +1034,8 _at__at_ 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 {
> _at__at_ -1043,6 +1045,15 _at__at_ 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;
> _at__at_ -1502,6 +1513,13 _at__at_ 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));
>
> _at__at_ -1646,6 +1664,10 _at__at_ 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 */
> }
> _at__at_ -2745,12 +2767,18 _at__at_ 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,
> _at__at_ -2761,6 +2789,11 _at__at_ 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);
> }
> _at__at_ -2769,6 +2802,7 _at__at_ 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",
> _at__at_ -2776,11 +2810,15 _at__at_ 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
> _at__at_ -2791,7 +2829,7 _at__at_ 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 ---