<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML xmlns="http://www.w3.org/TR/REC-html40" xmlns:m =
"http://schemas.microsoft.com/office/2004/12/omml" xmlns:w =
"urn:schemas-microsoft-com:office:word" xmlns:o =
"urn:schemas-microsoft-com:office:office" xmlns:v =
"urn:schemas-microsoft-com:vml"><HEAD>
<META content="text/html; charset=us-ascii" http-equiv=Content-Type>
<STYLE>@font-face {
font-family: Cambria Math;
}
@font-face {
font-family: Calibri;
}
@font-face {
font-family: Tahoma;
}
@page Section1 {size: 8.5in 11.0in; margin: 1.0in 1.0in 1.0in 1.0in; }
P.MsoNormal {
MARGIN: 0in 0in 0pt; FONT-FAMILY: "Times New Roman","serif"; FONT-SIZE: 12pt
}
LI.MsoNormal {
MARGIN: 0in 0in 0pt; FONT-FAMILY: "Times New Roman","serif"; FONT-SIZE: 12pt
}
DIV.MsoNormal {
MARGIN: 0in 0in 0pt; FONT-FAMILY: "Times New Roman","serif"; FONT-SIZE: 12pt
}
A:link {
COLOR: blue; TEXT-DECORATION: underline; mso-style-priority: 99
}
SPAN.MsoHyperlink {
COLOR: blue; TEXT-DECORATION: underline; mso-style-priority: 99
}
A:visited {
COLOR: purple; TEXT-DECORATION: underline; mso-style-priority: 99
}
SPAN.MsoHyperlinkFollowed {
COLOR: purple; TEXT-DECORATION: underline; mso-style-priority: 99
}
SPAN.EmailStyle17 {
FONT-FAMILY: "Calibri","sans-serif"; COLOR: #1f497d; mso-style-type: personal-reply
}
.MsoChpDefault {
FONT-SIZE: 10pt; mso-style-type: export-only
}
DIV.Section1 {
page: Section1
}
</STYLE>
<!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<META name=GENERATOR content="MSHTML 8.00.6001.18876"></HEAD>
<BODY lang=EN-US link=blue vLink=purple>
<DIV><SPAN class=873581508-09062010><FONT color=#0000ff size=2 face=Arial>Hi
Fab,</FONT></SPAN></DIV>
<DIV><SPAN class=873581508-09062010><FONT color=#0000ff size=2 face=Arial>Looks
excellent !</FONT></SPAN></DIV>
<DIV><SPAN class=873581508-09062010></SPAN><SPAN
class=873581508-09062010> </DIV></SPAN><BR>
<DIV dir=ltr lang=en-us class=OutlookMessageHeader align=left>
<HR tabIndex=-1>
<FONT size=2 face=Tahoma><B>From:</B> Fab Tillier
[mailto:ftillier@microsoft.com] <BR><B>Sent:</B> Tuesday, June 08, 2010 6:34
PM<BR><B>To:</B> Alex Naslednikov; ofw@lists.openfabrics.org; Leonid Keller;
Tzachi Dar<BR><B>Cc:</B> Uri Habusha<BR><B>Subject:</B> RE: [ofw] [Patch][Core]
Fix at IRP cancellation mechanism<BR></FONT><BR></DIV>
<DIV></DIV>
<DIV class=Section1>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;>Hi Alex,<o:p></o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;><o:p> </o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;>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.)<o:p></o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;><o:p> </o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;>IoSetCancelRoutine is atomic, so should provide the
correct level of synchronization.<o:p></o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;><o:p> </o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;>Thoughts?<o:p></o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;>-Fab<o:p></o:p></SPAN></P>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 11pt" color:#1F497D?
Calibri?,?sans-serif?;><o:p> </o:p></SPAN></P>
<DIV
style="BORDER-BOTTOM: medium none; BORDER-LEFT: blue 1.5pt solid; PADDING-BOTTOM: 0in; PADDING-LEFT: 4pt; PADDING-RIGHT: 0in; BORDER-TOP: medium none; BORDER-RIGHT: medium none; PADDING-TOP: 0in">
<DIV>
<DIV
style="BORDER-BOTTOM: medium none; BORDER-LEFT: medium none; PADDING-BOTTOM: 0in; PADDING-LEFT: 0in; PADDING-RIGHT: 0in; BORDER-TOP: #b5c4df 1pt solid; BORDER-RIGHT: medium none; PADDING-TOP: 3pt">
<P class=MsoNormal><B><SPAN style="FONT-FAMILY: ; FONT-SIZE: 10pt"
Tahoma?,?sans-serif??>From:</SPAN></B><SPAN
style="FONT-FAMILY: ; FONT-SIZE: 10pt" Tahoma?,?sans-serif??>
ofw-bounces@lists.openfabrics.org [mailto:ofw-bounces@lists.openfabrics.org]
<B>On Behalf Of </B>Alex Naslednikov<BR><B>Sent:</B> Tuesday, June 08, 2010
12:56 AM<BR><B>To:</B> ofw@lists.openfabrics.org; Leonid Keller; Tzachi
Dar<BR><B>Cc:</B> Uri Habusha<BR><B>Subject:</B> [ofw] [Patch][Core] Fix at IRP
cancellation mechanism<o:p></o:p></SPAN></P></DIV></DIV>
<P class=MsoNormal><o:p> </o:p></P>
<DIV>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 10pt"
Arial?,?sans-serif??>This patch fixes the race (and BSOD) within IRP
cancellation mechanism.<BR>The original problem:<BR>1.Function
al_dev_cancel_ioctl() called from the 2 places:<BR>directly from IBAL and by OS
(as a callback)<BR>2. This is because I/O request can be cancelled both by
user-level mechanism and <BR>by OS.<BR>3. Function al_dev_cancel_ioctl() calls
to IoGetCurrentIrpStackLocation() in order <BR>to receive context and take a
spinlock (p_context->cb_lock)<BR>4. But IoGetCurrentIrpStackLocation() as
well further access to stack pointer<BR>should be protected !!! <BR>5. BSOD
happened when context or IRP itself were already cancelled by concurrent
<BR>calls to al_dev_cancel_ioctl()</SPAN><o:p></o:p></P></DIV>
<DIV>
<P class=MsoNormal> <o:p></o:p></P></DIV>
<DIV>
<P class=MsoNormal><SPAN style="FONT-FAMILY: ; FONT-SIZE: 10pt"
Arial?,?sans-serif??>Signed-off by: Alexander Naslednikov (xalex at
mellanox.co.il)<BR>Index:
B:/users/xalex/2_1_0/core/al/kernel/al_dev.c<BR>===================================================================<BR>---
B:/users/xalex/2_1_0/core/al/kernel/al_dev.c (revision 5929)<BR>+++
B:/users/xalex/2_1_0/core/al/kernel/al_dev.c (revision 5930)<BR>@@ -65,9
+65,16
@@<BR> __proxy_cancel_cblists(<BR> IN al_dev_open_context_t *p_context
);<BR> <BR>+CL_INLINE
void<BR>+al_dev_cancel_ioctl_unlocked(<BR>+ IN al_dev_open_context_t *p_context,<BR>+ IN
OUT cl_ioctl_handle_t *p_ioctl
);<BR> static
void<BR> __construct_open_context(<BR> IN al_dev_open_context_t *p_context
)<BR>@@ -343,12 +350,19 @@<BR> p_context->closing =
TRUE;<BR> <BR> /* Flush any pending IOCTLs in case user-mode
threads died on us. */<BR>+ /* Protect IOCTL cancellation by spinlock to
avoid race <BR>+ */<BR>+ <BR>+ cl_spinlock_acquire(
&p_context->cb_lock );<BR>+ <BR> if(
p_context->h_cm_ioctl )<BR>- al_dev_cancel_ioctl(
p_context->h_cm_ioctl );<BR>- if( p_context->h_comp_ioctl
)<BR>- al_dev_cancel_ioctl( p_context->h_comp_ioctl
);<BR>+ al_dev_cancel_ioctl_unlocked( p_context,
&p_context->h_cm_ioctl );<BR>+ if( p_context->h_comp_ioctl )
<BR>+ al_dev_cancel_ioctl_unlocked( p_context,
&p_context->h_comp_ioctl );<BR> if( p_context->h_misc_ioctl
)<BR>- al_dev_cancel_ioctl( p_context->h_misc_ioctl
);<BR>+ al_dev_cancel_ioctl_unlocked( p_context,
&p_context->h_misc_ioctl );<BR>+ <BR>+ cl_spinlock_release (
&p_context->cb_lock );<BR> <BR> while(
p_context->ref_cnt )<BR> {<BR>@@ -514,20 +528,31
@@<BR> <BR> /* Get the stack location.
*/<BR> p_io_stack = IoGetCurrentIrpStackLocation( h_ioctl
);<BR>+ if (!p_io_stack) {<BR>+ AL_PRINT( TRACE_LEVEL_WARNING,
AL_DBG_DEV, ("This IOCTL was previosly destroyed \n")
);<BR>+ return;<BR>+ }<BR> <BR> p_context =
(al_dev_open_context_t
*)p_io_stack->FileObject->FsContext;<BR>- ASSERT( p_context
);<BR>-<BR>+ if (!p_context) {<BR>+ AL_PRINT( TRACE_LEVEL_ERROR,
AL_DBG_ERROR, ("This IOCTL was previosly destroyed \n")
);<BR>+ return;<BR>+ }<BR> /* Clear the IOCTL.
*/<BR> cl_spinlock_acquire( &p_context->cb_lock
);<BR> switch( cl_ioctl_ctl_code( h_ioctl )
)<BR> {<BR> case
UAL_GET_COMP_CB_INFO:<BR> ph_ioctl =
&p_context->h_comp_ioctl;<BR>+ ASSERT(ph_ioctl);<BR> break;<BR> case
UAL_GET_MISC_CB_INFO:<BR> ph_ioctl =
&p_context->h_misc_ioctl;<BR> break;<BR>+ case
NULL:<BR>+ AL_PRINT( TRACE_LEVEL_WARNING, AL_DBG_DEV, ("This IOCTL
was previosly destroyed \n") );<BR>+ cl_spinlock_release(
&p_context->cb_lock
);<BR>+ return;<BR> default:<BR> AL_PRINT(
TRACE_LEVEL_ERROR, AL_DBG_ERROR, ("Invalid CB type\n")
);<BR> ph_ioctl = NULL;<BR>@@ -550,7 +575,33
@@<BR> AL_EXIT( AL_DBG_DEV );<BR> }<BR> <BR>+/*<BR>+ *
Cancel any pending IOCTL calls for the specified type.<BR>+ * This routine is
also called when closing the device.<BR>+ * The lock should already be taken by
the caller<BR>+ */<BR>+CL_INLINE
void<BR>+al_dev_cancel_ioctl_unlocked(<BR>+ IN al_dev_open_context_t *p_context,<BR>+ IN
OUT cl_ioctl_handle_t *p_ioctl
)<BR>+{<BR>+ <BR>+ AL_ENTER( AL_DBG_DEV );<BR> <BR>+#pragma
warning(push, 3)<BR>+ IoSetCancelRoutine( *p_ioctl, NULL );<BR>+#pragma
warning(pop)<BR>+<BR>+ /* Complete the IOCTL.
*/<BR>+ cl_ioctl_complete( *p_ioctl, CL_CANCELED, 0
);<BR>+ proxy_context_deref( p_context );<BR>+ *p_ioctl =
NULL;<BR>+<BR>+ AL_EXIT( AL_DBG_DEV
);<BR>+}<BR>+<BR>+<BR>+<BR> void<BR> al_dev_cancel_io(<BR> IN DEVICE_OBJECT *p_dev_obj,<BR>Index:
B:/users/xalex/2_1_0/inc/kernel/complib/cl_ioctl_osd.h<BR>===================================================================<BR>---
B:/users/xalex/2_1_0/inc/kernel/complib/cl_ioctl_osd.h (revision
5929)<BR>+++
B:/users/xalex/2_1_0/inc/kernel/complib/cl_ioctl_osd.h (revision
5930)<BR>@@ -49,7 +49,9 @@<BR> #define IOCTL_CODE( type, cmd
) \<BR> CTL_CODE( type, (cmd & 0x0FFF), METHOD_BUFFERED,
FILE_ANY_ACCESS)<BR> <BR>+#define BAD_IRP_STACK
0<BR> <BR>+<BR> typedef
PIRP cl_ioctl_handle_t;<BR> <BR> <BR>@@ -101,6 +103,10
@@<BR> IO_STACK_LOCATION *p_io_stack;<BR> <BR> p_io_stack
= IoGetCurrentIrpStackLocation( h_ioctl );<BR>+ if (!p_io_stack)
{<BR>+ /* "This IOCTL was previosly destroyed !
*/<BR>+ return BAD_IRP_STACK ;<BR>+ }<BR> return
p_io_stack->Parameters.DeviceIoControl.IoControlCode;<BR> }<BR> </SPAN><o:p></o:p></P></DIV></DIV></DIV></BODY></HTML>