<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.MsoAcetate, li.MsoAcetate, div.MsoAcetate
{mso-style-priority:99;
mso-style-link:"Balloon Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:8.0pt;
font-family:"Tahoma","sans-serif";}
span.EmailStyle17
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:windowtext;}
span.EmailStyle18
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.EmailStyle19
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.BalloonTextChar
{mso-style-name:"Balloon Text Char";
mso-style-priority:99;
mso-style-link:"Balloon Text";
font-family:"Tahoma","sans-serif";}
span.EmailStyle22
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></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]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="color:#1F497D">OK, thanks. My eyes missed the &</span> p_thread->osd.p_thread.<o:p></o:p></p>
<p class="MsoNormal">Looks fine to commit sans Fab’s overall comment.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Stan.<span style="color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Leonid Keller [mailto:leonid@mellanox.co.il]
<br>
<b>Sent:</b> Tuesday, October 18, 2011 12:54 AM<br>
<b>To:</b> Smith, Stan; ofw_list<br>
<b>Subject:</b> RE: [ofw] [bug] [COMPLIB] A bug in COMPLIB thread handling mechanism.<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">PSB</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Smith, Stan [mailto:stan.smith@intel.com]
<br>
<b>Sent:</b> Monday, October 17, 2011 8:15 PM<br>
<b>To:</b> Leonid Keller; ofw_list<br>
<b>Subject:</b> RE: [ofw] [bug] [COMPLIB] A bug in COMPLIB thread handling mechanism.</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Hi Leo,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> I tested your patch with no problems. Although I did have one question; please see below.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Stan.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> ofw-bounces@lists.openfabrics.org [mailto:ofw-bounces@lists.openfabrics.org]
<b>On Behalf Of </b>Leonid Keller<br>
<b>Sent:</b> Sunday, October 09, 2011 4:26 AM<br>
<b>To:</b> ofw_list<br>
<b>Subject:</b> [ofw] [bug] [COMPLIB] A bug in COMPLIB thread handling mechanism.</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:8.5pt;font-family:"Courier New"">We got a BSOD in cl_thread_destroy() function on ZwClose().</span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:8.5pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:8.5pt;font-family:"Courier New"">The code review showed that the old mechanism doesn’t take a reference on the created thread and waits for the thread exit while it may already be a non-existed
object.</span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:8.5pt;font-family:"Courier New"">It also closes the thread handle too late.</span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:8.5pt;font-family:"Courier New"">Please, review the patch.</span><o:p></o:p></p>
<p class="MsoNormal" style="text-autospace:none"><span style="font-size:8.5pt;font-family:"Courier New""> </span><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Index: B:/users/leonid/svn/winib/trunk/core/complib/kernel/cl_thread.c<o:p></o:p></p>
<p class="MsoNormal">===================================================================<o:p></o:p></p>
<p class="MsoNormal">--- B:/users/leonid/svn/winib/trunk/core/complib/kernel/cl_thread.c (revision 8921)<o:p></o:p></p>
<p class="MsoNormal">+++ B:/users/leonid/svn/winib/trunk/core/complib/kernel/cl_thread.c (revision 8922)<o:p></o:p></p>
<p class="MsoNormal">@@ -38,11 +38,8 @@<o:p></o:p></p>
<p class="MsoNormal"> __thread_callback( <o:p></o:p></p>
<p class="MsoNormal"> IN cl_thread_t* p_thread )<o:p></o:p></p>
<p class="MsoNormal"> {<o:p></o:p></p>
<p class="MsoNormal">- /* Store the thread pointer so that destroy and is_current_thread work. */<o:p></o:p></p>
<p class="MsoNormal">- p_thread->osd.p_thread = KeGetCurrentThread();<o:p></o:p></p>
<p class="MsoNormal">-<o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">If </span>p_thread->osd.p_thread initialization is removed, where is it set as it is dereferenced later on?
<span style="font-family:Wingdings">ç</span>== <span style="color:red">QUESTION?</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">[LK] In </span>ObReferenceObjectByHandle() below. It takes a reference.<o:p></o:p></p>
<p class="MsoNormal"><span style="color:red"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"> /* Bump the thread's priority. */<o:p></o:p></p>
<p class="MsoNormal">- KeSetPriorityThread( p_thread->osd.p_thread, LOW_REALTIME_PRIORITY );<o:p></o:p></p>
<p class="MsoNormal">+ KeSetPriorityThread( KeGetCurrentThread(), LOW_REALTIME_PRIORITY );<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"> /* Call the user's thread function. */<o:p></o:p></p>
<p class="MsoNormal"> (*p_thread->pfn_callback)( (void*)p_thread->context );<o:p></o:p></p>
<p class="MsoNormal">@@ -91,6 +88,15 @@<o:p></o:p></p>
<p class="MsoNormal"> if( !NT_SUCCESS( status ) )<o:p></o:p></p>
<p class="MsoNormal"> return( CL_ERROR );<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">+ /* get pointer to thread object to wait on it's exit */<o:p></o:p></p>
<p class="MsoNormal">+ status = ObReferenceObjectByHandle( p_thread->osd.h_thread, THREAD_ALL_ACCESS,<o:p></o:p></p>
<p class="MsoNormal">+ NULL, KernelMode, (PVOID*)&p_thread->osd.p_thread, NULL );<o:p></o:p></p>
<p class="MsoNormal">+ CL_ASSERT(status == STATUS_SUCCESS); // According to MSDN, must succeed if I set the params<o:p></o:p></p>
<p class="MsoNormal">+<o:p></o:p></p>
<p class="MsoNormal">+ /* Close the handle to the thread. */<o:p></o:p></p>
<p class="MsoNormal">+ status = ZwClose( p_thread->osd.h_thread );<o:p></o:p></p>
<p class="MsoNormal">+ CL_ASSERT(NT_SUCCESS(status)); // Should always succeed<o:p></o:p></p>
<p class="MsoNormal">+<o:p></o:p></p>
<p class="MsoNormal"> return( CL_SUCCESS );<o:p></o:p></p>
<p class="MsoNormal"> }<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">@@ -112,8 +118,8 @@<o:p></o:p></p>
<p class="MsoNormal"> KeWaitForSingleObject( p_thread->osd.p_thread, Executive, KernelMode,<o:p></o:p></p>
<p class="MsoNormal"> FALSE, NULL );<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">- /* Close the handle to the thread. */<o:p></o:p></p>
<p class="MsoNormal">- ZwClose( p_thread->osd.h_thread );<o:p></o:p></p>
<p class="MsoNormal">+ /* Release the reference to thread object */<o:p></o:p></p>
<p class="MsoNormal">+ ObDereferenceObject( p_thread->osd.p_thread );<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"> /*<o:p></o:p></p>
<p class="MsoNormal"> * Reset the handle in case the user calls destroy and the thread is<o:p></o:p></p>
</div>
</div>
</div>
</body>
</html>