[ofw] [Patch][Core] Fix at IRP cancellation mechanism
Smith, Stan
stan.smith at intel.com
Mon Jun 21 10:38:03 PDT 2010
Thank you!
Will integrate and let you know how the testing goes.
stan.
________________________________
From: Leonid Keller [mailto:leonid at mellanox.co.il]
Sent: Sunday, June 20, 2010 1:18 AM
To: Smith, Stan; Alex Naslednikov; Fab Tillier
Cc: ofw at lists.openfabrics.org; Uri Habusha
Subject: RE: [ofw] [Patch][Core] Fix at IRP cancellation mechanism
Alex went on vacation.
Beforehand he tried Fab's idea and for some reason it didn't work well.
Maybe he made some mistake in the implementation ...
I took the problem over from him and made another variant of the patch, using IoCsqXXX functions as Microsoft recommends.
It seems like working good.
A short description of the patch:
1. Removed 'h_cm_ioctl' field as it seems to be unused.
2. Added 'al_csq_t' object, containing CSQ for pending IOCTLs and a pointer to dev_open_context.
3. Added callbacks for using this object with IoCsqXXX functions;
4. Replaced 'h_comp_ioctl' and 'h_misc_ioctl' pointers with 'al_csq_t' objects.
5. Removed 'al_dev_cancel_ioctl()' and 'al_dev_cancel_io()' as unnecessary;
To pend an ioctl IBAL now puts it on a CSQ; to complete - removes it from the queue and completes.
Index: core/al/al_dev.h
===================================================================
--- core/al/al_dev.h (revision 5593)
+++ core/al/al_dev.h (working copy)
@@ -82,15 +82,6 @@
al_dev_ioctl(
IN cl_ioctl_handle_t h_ioctl );
-void
-al_dev_cancel_ioctl(
- IN cl_ioctl_handle_t h_ioctl );
-
-void
-al_dev_cancel_io(
- IN DEVICE_OBJECT *p_dev_obj,
- IN IRP *p_irp );
-
cl_status_t
bus_add_pkey(
IN cl_ioctl_handle_t h_ioctl );
Index: core/al/al_proxy.h
===================================================================
--- core/al/al_proxy.h (revision 5593)
+++ core/al/al_proxy.h (working copy)
@@ -83,8 +83,21 @@
} proxy_pnp_recs_t;
+#ifdef CL_KERNEL
+typedef struct _al_dev_open_context al_dev_open_context_t;
+typedef struct _al_csq
+{
+ IO_CSQ csq;
+ KSPIN_LOCK lock;
+ LIST_ENTRY queue;
+ al_dev_open_context_t *dev_ctx;
+} al_csq_t;
+
+#endif
+
+
/**********************************************************
*
* Per-process device context.
@@ -110,9 +123,10 @@
cl_mutex_t pnp_mutex;
/* Pending IOCTLs. */
- cl_ioctl_handle_t h_cm_ioctl;
- cl_ioctl_handle_t h_comp_ioctl;
- cl_ioctl_handle_t h_misc_ioctl;
+#ifdef CL_KERNEL
+ al_csq_t al_csq_comp;
+ al_csq_t al_csq_misc;
+#endif
/* Per-process AL handle. */
ib_al_handle_t h_al;
@@ -191,7 +205,143 @@
} al_proxy_cb_info_t;
+#ifdef CL_KERNEL
+#pragma warning(disable:4706)
+static inline void al_csq_flush_que(
+ IN al_csq_t* p_al_csq,
+ IN NTSTATUS completion_code
+ )
+{
+ PIRP Irp;
+ while( Irp = IoCsqRemoveNextIrp( &p_al_csq->csq, NULL ) )
+ {
+ cl_ioctl_complete( Irp, completion_code, 0 );
+ proxy_context_deref( p_al_csq->dev_ctx );
+ }
+}
+#pragma warning(default:4706)
+
+static VOID __insert_irp(
+ IN PIO_CSQ Csq,
+ IN PIRP Irp
+ )
+{
+ al_csq_t *p_al_csq = (al_csq_t*)Csq;
+ InsertTailList( &p_al_csq->queue, &Irp->Tail.Overlay.ListEntry );
+}
+
+static VOID __remove_irp(
+ IN PIO_CSQ Csq,
+ IN PIRP Irp
+ )
+{
+ UNUSED_PARAM( Csq );
+ RemoveEntryList( &Irp->Tail.Overlay.ListEntry );
+}
+
+static PIRP __peek_next_irp(
+ IN PIO_CSQ Csq,
+ IN PIRP Irp,
+ IN PVOID PeekContext
+ )
+{
+ PIRP nextIrp = NULL;
+ PLIST_ENTRY nextEntry;
+ PLIST_ENTRY listHead;
+ al_csq_t *p_al_csq = (al_csq_t*)Csq;
+
+ listHead = &p_al_csq->queue;
+
+ //
+ // If the IRP is NULL, we will start peeking from the listhead, else
+ // we will start from that IRP onwards. This is done under the
+ // assumption that new IRPs are always inserted at the tail.
+ //
+
+ if(Irp == NULL)
+ nextEntry = listHead->Flink;
+ else
+ nextEntry = Irp->Tail.Overlay.ListEntry.Flink;
+
+ while(nextEntry != listHead) {
+ nextIrp = CONTAINING_RECORD(nextEntry, IRP, Tail.Overlay.ListEntry);
+
+ //
+ // If context is present, continue until you find a matching one.
+ // Else you break out as you got next one.
+ //
+
+ if(PeekContext)
+ {
+ /* for now PeekContext is unused */
+ }
+ else
+ {
+ break;
+ }
+
+ nextIrp = NULL;
+ nextEntry = nextEntry->Flink;
+ }
+
+ return nextIrp;
+}
+
+static VOID __acquire_lock(
+ IN PIO_CSQ Csq,
+ OUT PKIRQL Irql
+ )
+{
+ al_csq_t *p_al_csq = (al_csq_t*)Csq;
+ KeAcquireSpinLock( &p_al_csq->lock, Irql );
+}
+
+static VOID __release_lock(
+ IN PIO_CSQ Csq,
+ IN KIRQL Irql
+ )
+{
+ al_csq_t *p_al_csq = (al_csq_t*)Csq;
+ KeReleaseSpinLock( &p_al_csq->lock, Irql );
+}
+
+static VOID __complete_cancelled_irp(
+ IN PIO_CSQ Csq,
+ IN PIRP Irp
+ )
+{
+ al_csq_t *p_al_csq = (al_csq_t*)Csq;
+
+ cl_ioctl_complete( Irp, CL_CANCELED, 0 );
+ proxy_context_deref( p_al_csq->dev_ctx );
+}
+
+static inline NTSTATUS
+al_csq_init(
+ IN al_dev_open_context_t * dev_ctx,
+ IN al_csq_t * p_al_csq)
+{
+ NTSTATUS status;
+
+ status = IoCsqInitialize( &p_al_csq->csq,
+ __insert_irp, __remove_irp,
+ __peek_next_irp, __acquire_lock,
+ __release_lock, __complete_cancelled_irp );
+ if ( !NT_SUCCESS( status ) )
+ goto exit;
+
+ InitializeListHead( &p_al_csq->queue );
+ KeInitializeSpinLock( &p_al_csq->lock );
+ p_al_csq->dev_ctx = dev_ctx;
+ status = STATUS_SUCCESS;
+
+exit:
+ return status;
+}
+
+#endif
+
al_proxy_cb_info_t*
proxy_cb_get(
IN al_dev_open_context_t *p_context );
Index: core/al/kernel/al_dev.c
===================================================================
--- core/al/kernel/al_dev.c (revision 5593)
+++ core/al/kernel/al_dev.c (working copy)
@@ -94,6 +94,7 @@
IN al_dev_open_context_t *p_context )
{
cl_status_t cl_status;
+ NTSTATUS status;
cl_status = cl_event_init( &p_context->close_event, FALSE );
if( cl_status != CL_SUCCESS )
@@ -118,6 +119,15 @@
if( cl_status != CL_SUCCESS )
return cl_status;
+ status = al_csq_init( p_context, &p_context->al_csq_comp );
+ if ( !NT_SUCCESS( status ) )
+ return (cl_status_t)status;
+
+
+ status = al_csq_init( p_context, &p_context->al_csq_misc );
+ if ( !NT_SUCCESS( status ) )
+ return (cl_status_t)status;
+
return CL_SUCCESS;
}
@@ -325,6 +335,12 @@
p_io_stack = IoGetCurrentIrpStackLocation( h_ioctl );
/* Determine if the client closed the al_handle. */
+ if (!p_io_stack->FileObject)
+ {
+ AL_PRINT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
+ ("Client closed with a null open context .\n") );
+ return CL_SUCCESS;
+ }
p_context = (al_dev_open_context_t*)p_io_stack->FileObject->FsContext;
if( !p_context )
{
@@ -343,12 +359,8 @@
p_context->closing = TRUE;
/* Flush any pending IOCTLs in case user-mode threads died on us. */
- if( p_context->h_cm_ioctl )
- al_dev_cancel_ioctl( p_context->h_cm_ioctl );
- if( p_context->h_comp_ioctl )
- al_dev_cancel_ioctl( p_context->h_comp_ioctl );
- if( p_context->h_misc_ioctl )
- al_dev_cancel_ioctl( p_context->h_misc_ioctl );
+ al_csq_flush_que( &p_context->al_csq_comp, STATUS_CANCELLED );
+ al_csq_flush_que( &p_context->al_csq_misc, STATUS_CANCELLED );
while( p_context->ref_cnt )
{
@@ -491,78 +503,3 @@
AL_EXIT( AL_DBG_DEV );
return cl_status;
}
-
-
-
-/*
- * Cancel any pending IOCTL calls for the specified type.
- * This routine is also called when closing the device.
- */
-void
-al_dev_cancel_ioctl(
- IN cl_ioctl_handle_t h_ioctl )
-{
- al_dev_open_context_t *p_context;
- cl_ioctl_handle_t *ph_ioctl;
- PIO_STACK_LOCATION p_io_stack;
-
- /*
- * Search the ioctl buffer in the process specific queue
- * Dequeue it, if found
- */
- AL_ENTER( AL_DBG_DEV );
-
- /* Get the stack location. */
- p_io_stack = IoGetCurrentIrpStackLocation( h_ioctl );
-
- p_context = (al_dev_open_context_t *)p_io_stack->FileObject->FsContext;
- ASSERT( p_context );
-
- /* Clear the IOCTL. */
- cl_spinlock_acquire( &p_context->cb_lock );
- switch( cl_ioctl_ctl_code( h_ioctl ) )
- {
- case UAL_GET_COMP_CB_INFO:
- ph_ioctl = &p_context->h_comp_ioctl;
- break;
- case UAL_GET_MISC_CB_INFO:
- ph_ioctl = &p_context->h_misc_ioctl;
- break;
- default:
- AL_PRINT( TRACE_LEVEL_ERROR, AL_DBG_ERROR, ("Invalid CB type\n") );
- ph_ioctl = NULL;
- break;
- }
-
- if( ph_ioctl && *ph_ioctl == h_ioctl )
- {
- *ph_ioctl = NULL;
-#pragma warning(push, 3)
- IoSetCancelRoutine( h_ioctl, NULL );
-#pragma warning(pop)
-
- /* Complete the IOCTL. */
- cl_ioctl_complete( h_ioctl, CL_CANCELED, 0 );
- proxy_context_deref( p_context );
- }
- cl_spinlock_release( &p_context->cb_lock );
-
- AL_EXIT( AL_DBG_DEV );
-}
-
-
-void
-al_dev_cancel_io(
- IN DEVICE_OBJECT *p_dev_obj,
- IN IRP *p_irp )
-{
- AL_ENTER( AL_DBG_DEV );
-
- UNUSED_PARAM( p_dev_obj );
-
- al_dev_cancel_ioctl( p_irp );
-
- IoReleaseCancelSpinLock( p_irp->CancelIrql );
-
- AL_EXIT( AL_DBG_DEV );
-}
Index: core/al/kernel/al_proxy.c
===================================================================
--- core/al/kernel/al_proxy.c (revision 5593)
+++ core/al/kernel/al_proxy.c (working copy)
@@ -199,7 +199,7 @@
{
cl_qlist_t *p_cb_list;
al_proxy_cb_info_t *p_cb_info;
- cl_ioctl_handle_t *ph_ioctl;
+ al_csq_t *p_al_csq;
uintn_t ioctl_size;
AL_ENTER( AL_DBG_DEV );
@@ -209,14 +209,14 @@
{
case UAL_GET_COMP_CB_INFO:
p_cb_list = &p_context->comp_cb_list;
- ph_ioctl = &p_context->h_comp_ioctl;
+ p_al_csq = &p_context->al_csq_comp;
/* TODO: Use output size only. */
ioctl_size = sizeof( comp_cb_ioctl_info_t );
break;
case UAL_GET_MISC_CB_INFO:
p_cb_list = &p_context->misc_cb_list;
- ph_ioctl = &p_context->h_misc_ioctl;
+ p_al_csq = &p_context->al_csq_misc;
/* TODO: Use output size only. */
ioctl_size = sizeof( misc_cb_ioctl_info_t );
break;
@@ -254,7 +254,6 @@
}
/* There are no callbacks to report. Mark this IOCTL as pending. */
- CL_ASSERT( !(*ph_ioctl) );
/* If we're closing down, complete the IOCTL with a canceled status. */
if( p_context->closing )
@@ -264,13 +263,8 @@
return CL_CANCELED;
}
- *ph_ioctl = h_ioctl;
- /* Set the cancel routine for this IRP so the app can abort. */
-#pragma warning(push, 3)
- IoSetCancelRoutine( h_ioctl, al_dev_cancel_io );
-#pragma warning(pop)
- /* If returning pending, the IRP must be marked as such. */
- IoMarkIrpPending( h_ioctl );
+ /* put ioctl on the cancel-safe queue (it makes it pending) */
+ IoCsqInsertIrp( (PIO_CSQ)p_al_csq, h_ioctl, NULL );
/* Ref the context until the IOCTL is either completed or cancelled. */
proxy_context_ref( p_context );
Index: core/al/kernel/al_proxy_verbs.c
===================================================================
--- core/al/kernel/al_proxy_verbs.c (revision 5593)
+++ core/al/kernel/al_proxy_verbs.c (working copy)
@@ -254,7 +254,8 @@
{
cl_qlist_t *p_cb_list;
al_proxy_cb_info_t *p_cb_info;
- cl_ioctl_handle_t *ph_ioctl, h_ioctl;
+ cl_ioctl_handle_t h_ioctl;
+ al_csq_t *p_al_csq;
uintn_t ioctl_size;
AL_ENTER( AL_DBG_DEV );
@@ -264,13 +265,13 @@
{
case UAL_GET_COMP_CB_INFO:
p_cb_list = &p_context->comp_cb_list;
- ph_ioctl = &p_context->h_comp_ioctl;
+ p_al_csq = &p_context->al_csq_comp;
ioctl_size = sizeof( comp_cb_ioctl_info_t );
break;
case UAL_GET_MISC_CB_INFO:
p_cb_list = &p_context->misc_cb_list;
- ph_ioctl = &p_context->h_misc_ioctl;
+ p_al_csq = &p_context->al_csq_misc;
ioctl_size = sizeof( misc_cb_ioctl_info_t );
break;
@@ -299,17 +300,11 @@
cl_spinlock_acquire( &p_context->cb_lock );
cl_qlist_insert_tail( p_cb_list, &p_cb_info->pool_item.list_item );
- /* See if there is a pending IOCTL ready to receive the callback. */
- if( *ph_ioctl )
+ /* remove ioctl from the cancel-safe queue (it makes it unpending) */
+ h_ioctl = IoCsqRemoveNextIrp( (PIO_CSQ)p_al_csq, NULL );
+ if (h_ioctl)
{
- h_ioctl = *ph_ioctl;
- *ph_ioctl = NULL;
-#pragma warning(push, 3)
- IoSetCancelRoutine( h_ioctl, NULL );
-#pragma warning(pop)
-
p_cb_info->reported = TRUE;
-
/* Complete the IOCTL to return the callback information. */
CL_ASSERT( cl_ioctl_out_size( h_ioctl ) >= ioctl_size );
cl_memcpy( cl_ioctl_out_buf( h_ioctl ), p_cb_data, ioctl_size );
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Smith, Stan
Sent: Saturday, June 19, 2010 10:35 PM
To: Alex Naslednikov; Fab Tillier
Cc: ofw at lists.openfabrics.org; Uri Habusha
Subject: Re: [ofw] [Patch][Core] Fix at IRP cancellation mechanism
Hi Alex,
When were you going to commit this patch?
thanks,
stan.
________________________________
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Wednesday, June 09, 2010 2:38 AM
To: Fab Tillier
Cc: ofw at lists.openfabrics.org; Uri Habusha
Subject: Re: [ofw] [Patch][Core] Fix at IRP cancellation mechanism
Hi Fab,
Looks excellent !
________________________________
From: Fab Tillier [mailto:ftillier at microsoft.com]
Sent: Tuesday, June 08, 2010 6:34 PM
To: Alex Naslednikov; ofw at lists.openfabrics.org; Leonid Keller; Tzachi Dar
Cc: Uri Habusha
Subject: RE: [ofw] [Patch][Core] Fix at IRP cancellation mechanism
Hi Alex,
Why not move the call to IoSetCancelRoutine up in the function, and check the return value? If the return is NULL, then someone else already cancelled the IRP. If non-NULL, then the routine cancels it. This also will close any potential race with completing the IRP (where the cancel routine should also be cleared.)
IoSetCancelRoutine is atomic, so should provide the correct level of synchronization.
Thoughts?
-Fab
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Tuesday, June 08, 2010 12:56 AM
To: ofw at lists.openfabrics.org; Leonid Keller; Tzachi Dar
Cc: Uri Habusha
Subject: [ofw] [Patch][Core] Fix at IRP cancellation mechanism
This patch fixes the race (and BSOD) within IRP cancellation mechanism.
The original problem:
1.Function al_dev_cancel_ioctl() called from the 2 places:
directly from IBAL and by OS (as a callback)
2. This is because I/O request can be cancelled both by user-level mechanism and
by OS.
3. Function al_dev_cancel_ioctl() calls to IoGetCurrentIrpStackLocation() in order
to receive context and take a spinlock (p_context->cb_lock)
4. But IoGetCurrentIrpStackLocation() as well further access to stack pointer
should be protected !!!
5. BSOD happened when context or IRP itself were already cancelled by concurrent
calls to al_dev_cancel_ioctl()
Signed-off by: Alexander Naslednikov (xalex at mellanox.co.il)
Index: B:/users/xalex/2_1_0/core/al/kernel/al_dev.c
===================================================================
--- B:/users/xalex/2_1_0/core/al/kernel/al_dev.c (revision 5929)
+++ B:/users/xalex/2_1_0/core/al/kernel/al_dev.c (revision 5930)
@@ -65,9 +65,16 @@
__proxy_cancel_cblists(
IN al_dev_open_context_t *p_context );
+CL_INLINE void
+al_dev_cancel_ioctl_unlocked(
+ IN al_dev_open_context_t *p_context,
+ IN OUT cl_ioctl_handle_t *p_ioctl );
static void
__construct_open_context(
IN al_dev_open_context_t *p_context )
@@ -343,12 +350,19 @@
p_context->closing = TRUE;
/* Flush any pending IOCTLs in case user-mode threads died on us. */
+ /* Protect IOCTL cancellation by spinlock to avoid race
+ */
+
+ cl_spinlock_acquire( &p_context->cb_lock );
+
if( p_context->h_cm_ioctl )
- al_dev_cancel_ioctl( p_context->h_cm_ioctl );
- if( p_context->h_comp_ioctl )
- al_dev_cancel_ioctl( p_context->h_comp_ioctl );
+ al_dev_cancel_ioctl_unlocked( p_context, &p_context->h_cm_ioctl );
+ if( p_context->h_comp_ioctl )
+ al_dev_cancel_ioctl_unlocked( p_context, &p_context->h_comp_ioctl );
if( p_context->h_misc_ioctl )
- al_dev_cancel_ioctl( p_context->h_misc_ioctl );
+ al_dev_cancel_ioctl_unlocked( p_context, &p_context->h_misc_ioctl );
+
+ cl_spinlock_release ( &p_context->cb_lock );
while( p_context->ref_cnt )
{
@@ -514,20 +528,31 @@
/* Get the stack location. */
p_io_stack = IoGetCurrentIrpStackLocation( h_ioctl );
+ if (!p_io_stack) {
+ AL_PRINT( TRACE_LEVEL_WARNING, AL_DBG_DEV, ("This IOCTL was previosly destroyed \n") );
+ return;
+ }
p_context = (al_dev_open_context_t *)p_io_stack->FileObject->FsContext;
- ASSERT( p_context );
-
+ if (!p_context) {
+ AL_PRINT( TRACE_LEVEL_ERROR, AL_DBG_ERROR, ("This IOCTL was previosly destroyed \n") );
+ return;
+ }
/* Clear the IOCTL. */
cl_spinlock_acquire( &p_context->cb_lock );
switch( cl_ioctl_ctl_code( h_ioctl ) )
{
case UAL_GET_COMP_CB_INFO:
ph_ioctl = &p_context->h_comp_ioctl;
+ ASSERT(ph_ioctl);
break;
case UAL_GET_MISC_CB_INFO:
ph_ioctl = &p_context->h_misc_ioctl;
break;
+ case NULL:
+ AL_PRINT( TRACE_LEVEL_WARNING, AL_DBG_DEV, ("This IOCTL was previosly destroyed \n") );
+ cl_spinlock_release( &p_context->cb_lock );
+ return;
default:
AL_PRINT( TRACE_LEVEL_ERROR, AL_DBG_ERROR, ("Invalid CB type\n") );
ph_ioctl = NULL;
@@ -550,7 +575,33 @@
AL_EXIT( AL_DBG_DEV );
}
+/*
+ * Cancel any pending IOCTL calls for the specified type.
+ * This routine is also called when closing the device.
+ * The lock should already be taken by the caller
+ */
+CL_INLINE void
+al_dev_cancel_ioctl_unlocked(
+ IN al_dev_open_context_t *p_context,
+ IN OUT cl_ioctl_handle_t *p_ioctl )
+{
+
+ AL_ENTER( AL_DBG_DEV );
+#pragma warning(push, 3)
+ IoSetCancelRoutine( *p_ioctl, NULL );
+#pragma warning(pop)
+
+ /* Complete the IOCTL. */
+ cl_ioctl_complete( *p_ioctl, CL_CANCELED, 0 );
+ proxy_context_deref( p_context );
+ *p_ioctl = NULL;
+
+ AL_EXIT( AL_DBG_DEV );
+}
+
+
+
void
al_dev_cancel_io(
IN DEVICE_OBJECT *p_dev_obj,
Index: B:/users/xalex/2_1_0/inc/kernel/complib/cl_ioctl_osd.h
===================================================================
--- B:/users/xalex/2_1_0/inc/kernel/complib/cl_ioctl_osd.h (revision 5929)
+++ B:/users/xalex/2_1_0/inc/kernel/complib/cl_ioctl_osd.h (revision 5930)
@@ -49,7 +49,9 @@
#define IOCTL_CODE( type, cmd ) \
CTL_CODE( type, (cmd & 0x0FFF), METHOD_BUFFERED, FILE_ANY_ACCESS)
+#define BAD_IRP_STACK 0
+
typedef PIRP cl_ioctl_handle_t;
@@ -101,6 +103,10 @@
IO_STACK_LOCATION *p_io_stack;
p_io_stack = IoGetCurrentIrpStackLocation( h_ioctl );
+ if (!p_io_stack) {
+ /* "This IOCTL was previosly destroyed ! */
+ return BAD_IRP_STACK ;
+ }
return p_io_stack->Parameters.DeviceIoControl.IoControlCode;
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20100621/3df0967e/attachment.html>
More information about the ofw
mailing list