fix vhci(ude) BSOD when processing partial urbr

BSOD seemed to occur sometimes while a partial urbr is processed. See #222. There was a race condition between store_urbr_partial() and urbr cancellation. This commit will unmark cancelable before processing a partial urbr in order to avoid such a race.
cezanne · Mar 2, 2021 · 2c593a5 · 2c593a5
1 parent 2c87ee4
commit 2c593a5
Show file tree

Hide file tree

Showing 12 changed files with 74 additions and 43 deletions.
diff --git a/driver/vhci_ude/vhci_dbg.c b/driver/vhci_ude/vhci_dbg.c
@@ -199,7 +199,7 @@ dbg_urbr(purb_req_t urbr)
 	else {
 		switch (urbr->type) {
 		case URBR_TYPE_URB:
-			len_dbg_urbr = libdrv_snprintf(buf_dbg_urbr, 128, "[urb,seq:%u,%s]", urbr->seq_num, dbg_urbfunc(urbr->u.urb->UrbHeader.Function));
+			len_dbg_urbr = libdrv_snprintf(buf_dbg_urbr, 128, "[urb,seq:%u,%s]", urbr->seq_num, dbg_urbfunc(urbr->u.urb.urb->UrbHeader.Function));
 			break;
 		case URBR_TYPE_UNLINK:
 			len_dbg_urbr = libdrv_snprintf(buf_dbg_urbr, 128, "[ulk,seq:%u,%u]", urbr->seq_num, urbr->u.seq_num_unlink);

diff --git a/driver/vhci_ude/vhci_read.c b/driver/vhci_ude/vhci_read.c
@@ -20,6 +20,25 @@ find_pending_urbr(pctx_vusb_t vusb)
 	return urbr;
 }
 
+static purb_req_t
+get_partial_urbr(pctx_vusb_t vusb)
+{
+	purb_req_t	urbr;
+
+	if (vusb->urbr_sent_partial == NULL)
+		return NULL;
+
+	urbr = vusb->urbr_sent_partial;
+	if (unmark_cancelable_urbr(urbr))
+		return urbr;
+	else {
+		/* There's on-going cancellation. It's enough to just clear out followings. */
+		vusb->urbr_sent_partial = NULL;
+		vusb->len_sent_partial = 0;
+		return NULL;
+	}
+}
+
 static VOID
 req_read_cancelled(WDFREQUEST req_read)
 {
@@ -54,9 +73,8 @@ read_vusb(pctx_vusb_t vusb, WDFREQUEST req)
 		WdfSpinLockRelease(vusb->spin_lock);
 		return STATUS_INVALID_DEVICE_REQUEST;
 	}
-	if (vusb->urbr_sent_partial != NULL) {
-		urbr = vusb->urbr_sent_partial;
-
+	urbr = get_partial_urbr(vusb);
+	if (urbr != NULL) {
 		WdfSpinLockRelease(vusb->spin_lock);
 
 		status = store_urbr_partial(req, urbr);
@@ -95,6 +113,7 @@ read_vusb(pctx_vusb_t vusb, WDFREQUEST req)
 		BOOLEAN	unmarked;
 		RemoveEntryListInit(&urbr->list_all);
 		unmarked = unmark_cancelable_urbr(urbr);
+		vusb->urbr_sent_partial = NULL;
 		WdfSpinLockRelease(vusb->spin_lock);
 
 		if (unmarked)

diff --git a/driver/vhci_ude/vhci_urbr.c b/driver/vhci_ude/vhci_urbr.c
@@ -123,8 +123,9 @@ create_urbr(pctx_ep_t ep, urbr_type_t type, WDFREQUEST req)
 	urbr->hmem = hmem;
 	urbr->ep = ep;
 	urbr->req = req;
-	if (req != NULL) {
-		urbr->u.urb = get_urb_from_req(req);
+	if (type == URBR_TYPE_URB) {
+		urbr->u.urb.urb = get_urb_from_req(req);
+		urbr->u.urb.cancelable = FALSE;
 		WdfRequestSetInformation(req, (ULONG_PTR)urbr);
 	}
 
@@ -185,6 +186,25 @@ urbr_cancelled(_In_ WDFREQUEST req)
 	}
 }
 
+static BOOLEAN
+mark_cancelable_urbr(purb_req_t urbr)
+{
+	NTSTATUS	status;
+
+	if (urbr->type != URBR_TYPE_URB)
+		return TRUE;
+
+	ASSERT(!urbr->u.urb.cancelable);
+
+	status = WdfRequestMarkCancelableEx(urbr->req, urbr_cancelled);
+	if (NT_ERROR(status)) {
+		TRD(URBR, "Already cancelled request?: %!URBR!, %!STATUS!", urbr, status);
+		return FALSE;
+	}
+	urbr->u.urb.cancelable = TRUE;
+	return TRUE;
+}
+
 NTSTATUS
 submit_urbr(purb_req_t urbr)
 {
@@ -201,13 +221,9 @@ submit_urbr(purb_req_t urbr)
 	}
 
 	if (vusb->urbr_sent_partial || vusb->pending_req_read == NULL) {
-		if (urbr->type == URBR_TYPE_URB) {
-			status = WdfRequestMarkCancelableEx(urbr->req, urbr_cancelled);
-			if (NT_ERROR(status)) {
-				WdfSpinLockRelease(vusb->spin_lock);
-				TRD(URBR, "failed to go pending. Already cancelled?: %!URBR!, %!STATUS!", urbr, status);
-				return STATUS_CANCELLED;
-			}
+		if (!mark_cancelable_urbr(urbr)) {
+			WdfSpinLockRelease(vusb->spin_lock);
+			return STATUS_CANCELLED;
 		}
 		InsertTailList(&vusb->head_urbr_pending, &urbr->list_state);
 		InsertTailList(&vusb->head_urbr, &urbr->list_all);
@@ -229,13 +245,9 @@ submit_urbr(purb_req_t urbr)
 	WdfSpinLockAcquire(vusb->spin_lock);
 
 	if (status == STATUS_SUCCESS) {
-		if (urbr->type == URBR_TYPE_URB) {
-			status = WdfRequestMarkCancelableEx(urbr->req, urbr_cancelled);
-			if (NT_ERROR(status)) {
-				WdfSpinLockRelease(vusb->spin_lock);
-				TRD(URBR, "Already cancelled request?: %!URBR!, %!STATUS!", urbr, status);
-				return STATUS_CANCELLED;
-			}
+		if (!mark_cancelable_urbr(urbr)) {
+			WdfSpinLockRelease(vusb->spin_lock);
+			return STATUS_CANCELLED;
 		}
 		if (vusb->len_sent_partial == 0) {
 			vusb->urbr_sent_partial = NULL;
@@ -327,9 +339,10 @@ unmark_cancelable_urbr(purb_req_t urbr)
 	req = urbr->req;
 	if (req == NULL)
 		return TRUE;
-	if (urbr->type != URBR_TYPE_URB)
+	if (urbr->type != URBR_TYPE_URB || !urbr->u.urb.cancelable)
 		return TRUE;
 	status = WdfRequestUnmarkCancelable(req);
+	urbr->u.urb.cancelable = FALSE;
 	if (status == STATUS_CANCELLED)
 		return FALSE;
 	return TRUE;
@@ -346,7 +359,7 @@ complete_urbr(purb_req_t urbr, NTSTATUS status)
 			WdfRequestComplete(req, status);
 		else {
 			if (status == STATUS_SUCCESS)
-				UdecxUrbComplete(req, urbr->u.urb->UrbHeader.Status);
+				UdecxUrbComplete(req, urbr->u.urb.urb->UrbHeader.Status);
 			else {
 				UdecxUrbCompleteWithNtStatus(req, status);
 			}

diff --git a/driver/vhci_ude/vhci_urbr.h b/driver/vhci_ude/vhci_urbr.h
@@ -22,7 +22,10 @@ typedef struct _urb_req {
 	urbr_type_t	type;
 	unsigned long	seq_num;
 	union {
-		PURB	urb;
+		struct {
+			PURB	urb;
+			BOOLEAN	cancelable;
+		} urb;
 		unsigned long	seq_num_unlink;
 		UCHAR	conf_value;
 		struct {

diff --git a/driver/vhci_ude/vhci_urbr_fetch.c b/driver/vhci_ude/vhci_urbr_fetch.c
@@ -95,7 +95,7 @@ fetch_urbr_urb(PURB urb, struct usbip_header *hdr)
 static VOID
 handle_urbr_error(purb_req_t urbr, struct usbip_header *hdr)
 {
-	PURB	urb = urbr->u.urb;
+	PURB	urb = urbr->u.urb.urb;
 
 	urb->UrbHeader.Status = to_usbd_status(hdr->u.ret_submit.status);
 	if (urb->UrbHeader.Status == USBD_STATUS_STALL_PID && urbr->ep->vusb->is_simple_ep_alloc) {
@@ -124,7 +124,7 @@ fetch_urbr(purb_req_t urbr, struct usbip_header *hdr)
 		if (hdr->u.ret_submit.status != 0)
 			handle_urbr_error(urbr, hdr);
 
-		status = fetch_urbr_urb(urbr->u.urb, hdr);
+		status = fetch_urbr_urb(urbr->u.urb.urb, hdr);
 	}
 
 	TRD(WRITE, "Leave: %!STATUS!", status);

diff --git a/driver/vhci_ude/vhci_urbr_store.c b/driver/vhci_ude/vhci_urbr_store.c
@@ -50,10 +50,6 @@ store_urbr_partial(WDFREQUEST req_read, purb_req_t urbr)
 
 	TRD(READ, "Enter: urbr: %!URBR!", urbr);
 
-	/* This request is sometimes NULL, which causes driver/system failure, if used. */
-	if (urbr->req == NULL)
-		return STATUS_SUCCESS;
-
 	WDF_REQUEST_PARAMETERS_INIT(&params);
 	WdfRequestGetParameters(urbr->req, &params);
 
@@ -99,7 +95,7 @@ store_urbr_urb(WDFREQUEST req_read, purb_req_t urbr)
 	USHORT		urb_func;
 	NTSTATUS	status;
 
-	urb_func = urbr->u.urb->UrbHeader.Function;
+	urb_func = urbr->u.urb.urb->UrbHeader.Function;
 	TRD(READ, "%!URBR!", urbr);
 
 	switch (urb_func) {

diff --git a/driver/vhci_ude/vhci_urbr_store_bulk.c b/driver/vhci_ude/vhci_urbr_store_bulk.c
@@ -7,7 +7,7 @@
 NTSTATUS
 store_urbr_bulk_partial(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_BULK_OR_INTERRUPT_TRANSFER	*urb_bi = &urbr->u.urb->UrbBulkOrInterruptTransfer;
+	struct _URB_BULK_OR_INTERRUPT_TRANSFER	*urb_bi = &urbr->u.urb.urb->UrbBulkOrInterruptTransfer;
 	PVOID	dst, src;
 	NTSTATUS	status;
 
@@ -28,7 +28,7 @@ store_urbr_bulk_partial(WDFREQUEST req_read, purb_req_t urbr)
 NTSTATUS
 store_urbr_bulk(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_BULK_OR_INTERRUPT_TRANSFER	*urb_bi = &urbr->u.urb->UrbBulkOrInterruptTransfer;
+	struct _URB_BULK_OR_INTERRUPT_TRANSFER	*urb_bi = &urbr->u.urb.urb->UrbBulkOrInterruptTransfer;
 	struct usbip_header	*hdr;
 	int	in, type;
 

diff --git a/driver/vhci_ude/vhci_urbr_store_control.c b/driver/vhci_ude/vhci_urbr_store_control.c
@@ -10,7 +10,7 @@
 NTSTATUS
 store_urbr_control_transfer_partial(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_TRANSFER	*urb_ctltrans = &urbr->u.urb->UrbControlTransfer;
+	struct _URB_CONTROL_TRANSFER	*urb_ctltrans = &urbr->u.urb.urb->UrbControlTransfer;
 	PVOID	dst;
 	char	*buf;
 
@@ -35,7 +35,7 @@ store_urbr_control_transfer_partial(WDFREQUEST req_read, purb_req_t urbr)
 NTSTATUS
 store_urbr_control_transfer_ex_partial(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_TRANSFER_EX	*urb_ctltrans_ex = &urbr->u.urb->UrbControlTransferEx;
+	struct _URB_CONTROL_TRANSFER_EX	*urb_ctltrans_ex = &urbr->u.urb.urb->UrbControlTransferEx;
 	PVOID	dst;
 	char	*buf;
 
@@ -60,7 +60,7 @@ store_urbr_control_transfer_ex_partial(WDFREQUEST req_read, purb_req_t urbr)
 NTSTATUS
 store_urbr_control_transfer(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_TRANSFER	*urb_ctltrans = &urbr->u.urb->UrbControlTransfer;
+	struct _URB_CONTROL_TRANSFER	*urb_ctltrans = &urbr->u.urb.urb->UrbControlTransfer;
 	struct usbip_header	*hdr;
 	int	in = IS_TRANSFER_FLAGS_IN(urb_ctltrans->TransferFlags);
 	ULONG	nread = 0;
@@ -133,7 +133,7 @@ NTSTATUS
 store_urbr_control_transfer_ex(WDFREQUEST req_read, purb_req_t urbr)
 {
 	pctx_vusb_t	vusb = urbr->ep->vusb;
-	struct _URB_CONTROL_TRANSFER_EX	*urb_ctltrans_ex = &urbr->u.urb->UrbControlTransferEx;
+	struct _URB_CONTROL_TRANSFER_EX	*urb_ctltrans_ex = &urbr->u.urb.urb->UrbControlTransferEx;
 	struct usbip_header	*hdr;
 	int	in = IS_TRANSFER_FLAGS_IN(urb_ctltrans_ex->TransferFlags);
 	ULONG	nread = 0;

diff --git a/driver/vhci_ude/vhci_urbr_store_dscr.c b/driver/vhci_ude/vhci_urbr_store_dscr.c
@@ -6,7 +6,7 @@
 NTSTATUS
 store_urbr_dscr_dev(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_DESCRIPTOR_REQUEST	*urb_dscr = &urbr->u.urb->UrbControlDescriptorRequest;
+	struct _URB_CONTROL_DESCRIPTOR_REQUEST	*urb_dscr = &urbr->u.urb.urb->UrbControlDescriptorRequest;
 	struct usbip_header	*hdr;
 	usb_cspkt_t	*csp;
 
@@ -46,7 +46,7 @@ store_urbr_dscr_dev(WDFREQUEST req_read, purb_req_t urbr)
 NTSTATUS
 store_urbr_dscr_intf(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_DESCRIPTOR_REQUEST	*urb_dscr = &urbr->u.urb->UrbControlDescriptorRequest;
+	struct _URB_CONTROL_DESCRIPTOR_REQUEST	*urb_dscr = &urbr->u.urb.urb->UrbControlDescriptorRequest;
 	struct usbip_header	*hdr;
 	usb_cspkt_t	*csp;
 

diff --git a/driver/vhci_ude/vhci_urbr_store_iso.c b/driver/vhci_ude/vhci_urbr_store_iso.c
@@ -57,7 +57,7 @@ get_iso_payload_len(struct _URB_ISOCH_TRANSFER *urb_iso)
 NTSTATUS
 store_urbr_iso_partial(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_ISOCH_TRANSFER	*urb_iso = &urbr->u.urb->UrbIsochronousTransfer;
+	struct _URB_ISOCH_TRANSFER	*urb_iso = &urbr->u.urb.urb->UrbIsochronousTransfer;
 	ULONG	len_iso;
 	PVOID	dst;
 
@@ -77,7 +77,7 @@ store_urbr_iso_partial(WDFREQUEST req_read, purb_req_t urbr)
 NTSTATUS
 store_urbr_iso(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_ISOCH_TRANSFER	*urb_iso = &urbr->u.urb->UrbIsochronousTransfer;
+	struct _URB_ISOCH_TRANSFER	*urb_iso = &urbr->u.urb.urb->UrbIsochronousTransfer;
 	struct usbip_header	*hdr;
 	int	in, type;
 

diff --git a/driver/vhci_ude/vhci_urbr_store_status.c b/driver/vhci_ude/vhci_urbr_store_status.c
@@ -7,7 +7,7 @@
 NTSTATUS
 store_urbr_get_status(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_GET_STATUS_REQUEST	*urb_status = &urbr->u.urb->UrbControlGetStatusRequest;
+	struct _URB_CONTROL_GET_STATUS_REQUEST	*urb_status = &urbr->u.urb.urb->UrbControlGetStatusRequest;
 	struct usbip_header	*hdr;
 	USHORT		urbfunc;
 	char		recip;

diff --git a/driver/vhci_ude/vhci_urbr_store_vendor.c b/driver/vhci_ude/vhci_urbr_store_vendor.c
@@ -7,7 +7,7 @@
 NTSTATUS
 store_urbr_vendor_class_partial(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_VENDOR_OR_CLASS_REQUEST	*urb_vendor_class = &urbr->u.urb->UrbControlVendorClassRequest;
+	struct _URB_CONTROL_VENDOR_OR_CLASS_REQUEST	*urb_vendor_class = &urbr->u.urb.urb->UrbControlVendorClassRequest;
 	PVOID	dst;
 	char	*buf;
 
@@ -33,7 +33,7 @@ store_urbr_vendor_class_partial(WDFREQUEST req_read, purb_req_t urbr)
 NTSTATUS
 store_urbr_vendor_class(WDFREQUEST req_read, purb_req_t urbr)
 {
-	struct _URB_CONTROL_VENDOR_OR_CLASS_REQUEST	*urb_vendor_class = &urbr->u.urb->UrbControlVendorClassRequest;
+	struct _URB_CONTROL_VENDOR_OR_CLASS_REQUEST	*urb_vendor_class = &urbr->u.urb.urb->UrbControlVendorClassRequest;
 	struct usbip_header	*hdr;
 	usb_cspkt_t	*csp;
 	char	type, recip;