<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=iso-8859-1">
<META NAME="Generator" CONTENT="MS Exchange Server version 5.5.2654.45">
<TITLE>RE: [Openib-windows] [PATCH] 2 new vendor calls</TITLE>
</HEAD>
<BODY>
<P><FONT SIZE=2>Hi Fab,</FONT>
<BR><FONT SIZE=2>Thank you for your comments.</FONT>
</P>
<P><FONT SIZE=2>First of all, the problem with unmapping is in need in releasing of the MDL, not in unmapping itself.</FONT>
</P>
<P><FONT SIZE=2>You are right about security and resource tracking: i saw these problems, but didn't find a simple solution. </FONT>
<BR><FONT SIZE=2>The right solution IMO would have been to add the context of FW_MAP_CRSPACE operation to the process resource tracking information and to perform FW_UNMAP_CRSPACE automatically upon process termination.</FONT></P>
<P><FONT SIZE=2>Then we could remove the context from FW_MAP_CRSPACE and not provide FW_UNMAP_CRSPACE at all.</FONT>
</P>
<P><FONT SIZE=2>Your idea of putting the context into CA object seams to me troublesome:</FONT>
<BR> <FONT SIZE=2>- it prevents several applications to use simultaneously this mechanism (on the same HCA);</FONT>
<BR> <FONT SIZE=2>- if an application was killed and restarted, there will be a memory leak.</FONT>
</P>
<P><FONT SIZE=2>I don't know enough how the resource tracking mechaism works today and whether there is a simple way to implement the above solution. </FONT></P>
<P><FONT SIZE=2>Could you enlight me on this ?</FONT>
<BR><FONT SIZE=2>TIA</FONT>
</P>
<BR>
<P><FONT SIZE=2>> -----Original Message-----</FONT>
<BR><FONT SIZE=2>> From: Fabian Tillier [<A HREF="mailto:ftillier@silverstorm.com">mailto:ftillier@silverstorm.com</A>]</FONT>
<BR><FONT SIZE=2>> Sent: Wednesday, September 21, 2005 10:00 PM</FONT>
<BR><FONT SIZE=2>> To: Leonid Keller</FONT>
<BR><FONT SIZE=2>> Cc: openib-windows@openib.org</FONT>
<BR><FONT SIZE=2>> Subject: Re: [Openib-windows] [PATCH] 2 new vendor calls</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> On 9/21/05, Leonid Keller <leonid@mellanox.co.il> wrote:</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > i've added 2 new vendor calls for "tools" (like FW burning </FONT>
<BR><FONT SIZE=2>> tool flint.exe):</FONT>
<BR><FONT SIZE=2>> > FW_MAP_CRSPACE - maps HCA's CR SPACE into user's </FONT>
<BR><FONT SIZE=2>> space and returns</FONT>
<BR><FONT SIZE=2>> > VA and an opaque object for unmap.</FONT>
<BR><FONT SIZE=2>> > FW_UNMAP_CRSPACE - unmaps CR SPACE and </FONT>
<BR><FONT SIZE=2>> releases resources.</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > Any comments are appreciated.</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > Signed-off-by: Leonid Keller(leonid@mellanox.co.il)</FONT>
<BR><FONT SIZE=2>> > Index:</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> S:/svn.wininf/trunk/hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/MdPnp.c</FONT>
<BR><FONT SIZE=2>> > ===================================================================</FONT>
<BR><FONT SIZE=2>> > ---</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> S:/svn.wininf/trunk/hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/MdPnp.c</FONT>
<BR><FONT SIZE=2>> > (revision 438)</FONT>
<BR><FONT SIZE=2>> > +++</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> S:/svn.wininf/trunk/hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/MdPnp.c</FONT>
<BR><FONT SIZE=2>> > (revision 439)</FONT>
<BR><FONT SIZE=2>> > @@ -490,11 +490,18 @@</FONT>
<BR><FONT SIZE=2>> > {</FONT>
<BR><FONT SIZE=2>> > if(</FONT>
<BR><FONT SIZE=2>> > l_pIrpStack->Parameters.QueryInterface.InterfaceSpecificData</FONT>
<BR><FONT SIZE=2>> > )</FONT>
<BR><FONT SIZE=2>> > {</FONT>
<BR><FONT SIZE=2>> > - // Our interface. Return </FONT>
<BR><FONT SIZE=2>> the HH HCA handle</FONT>
<BR><FONT SIZE=2>> > in</FONT>
<BR><FONT SIZE=2>> > - // the </FONT>
<BR><FONT SIZE=2>> "InterfaceSpecificData" member.</FONT>
<BR><FONT SIZE=2>> > -</FONT>
<BR><FONT SIZE=2>> > *(HH_hca_hndl_t*)l_pIrpStack->Parameters.QueryInterface.</FONT>
<BR><FONT SIZE=2>> > - InterfaceSpecificData =</FONT>
<BR><FONT SIZE=2>> > l_pMdDevContext->m_hHhHca;</FONT>
<BR><FONT SIZE=2>> > - pi_pIrp->IoStatus.Status = STATUS_SUCCESS;</FONT>
<BR><FONT SIZE=2>> > + struct _hca_if {</FONT>
<BR><FONT SIZE=2>> > + HH_hca_hndl_t hh_hndl;</FONT>
<BR><FONT SIZE=2>> > + void * </FONT>
<BR><FONT SIZE=2>> kernel_crspace_addr;</FONT>
<BR><FONT SIZE=2>> > + ULONG kernel_crspace_size;</FONT>
<BR><FONT SIZE=2>> > + } *if_p =</FONT>
<BR><FONT SIZE=2>> > l_pIrpStack->Parameters.QueryInterface.InterfaceSpecificData;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // Our interface. Return </FONT>
<BR><FONT SIZE=2>> the HH HCA handle</FONT>
<BR><FONT SIZE=2>> > and other data</FONT>
<BR><FONT SIZE=2>> > + if_p->hh_hndl = </FONT>
<BR><FONT SIZE=2>> l_pMdDevContext->m_hHhHca;</FONT>
<BR><FONT SIZE=2>> > + if_p->kernel_crspace_addr =</FONT>
<BR><FONT SIZE=2>> > l_pMdDevContext->m_Cr.m_pKernelAddr;</FONT>
<BR><FONT SIZE=2>> > + if_p->kernel_crspace_size =</FONT>
<BR><FONT SIZE=2>> > l_pMdDevContext->m_Cr.m_ulKernelSize;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + pi_pIrp->IoStatus.Status = STATUS_SUCCESS;</FONT>
<BR><FONT SIZE=2>> > l_Status = STATUS_SUCCESS;</FONT>
<BR><FONT SIZE=2>> > break;</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.c</FONT>
<BR><FONT SIZE=2>> > ===================================================================</FONT>
<BR><FONT SIZE=2>> > --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.c</FONT>
<BR><FONT SIZE=2>> > (revision 438)</FONT>
<BR><FONT SIZE=2>> > +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.c</FONT>
<BR><FONT SIZE=2>> > (revision 439)</FONT>
<BR><FONT SIZE=2>> > @@ -142,6 +142,28 @@</FONT>
<BR><FONT SIZE=2>> > return p_hca;</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > +mlnx_hca_t*</FONT>
<BR><FONT SIZE=2>> > +mlnx_hca_from_hh_hndl(</FONT>
<BR><FONT SIZE=2>> > + IN HH_hca_hndl_t</FONT>
<BR><FONT SIZE=2>> > hh_hndl )</FONT>
<BR><FONT SIZE=2>> > +{</FONT>
<BR><FONT SIZE=2>> > + cl_list_item_t *p_item;</FONT>
<BR><FONT SIZE=2>> > + mlnx_hca_t *p_hca = NULL;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + cl_spinlock_acquire( &hob_lock );</FONT>
<BR><FONT SIZE=2>> > + p_item = cl_qlist_head( &mlnx_hca_list );</FONT>
<BR><FONT SIZE=2>> > + while( p_item != cl_qlist_end( &mlnx_hca_list ) )</FONT>
<BR><FONT SIZE=2>> > + {</FONT>
<BR><FONT SIZE=2>> > + p_hca = PARENT_STRUCT( p_item, mlnx_hca_t, </FONT>
<BR><FONT SIZE=2>> list_item );</FONT>
<BR><FONT SIZE=2>> > + if( p_hca->hh_hndl == hh_hndl )</FONT>
<BR><FONT SIZE=2>> > + break;</FONT>
<BR><FONT SIZE=2>> > + p_item = cl_qlist_next( p_item );</FONT>
<BR><FONT SIZE=2>> > + p_hca = NULL;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > + cl_spinlock_release( &hob_lock );</FONT>
<BR><FONT SIZE=2>> > + return p_hca;</FONT>
<BR><FONT SIZE=2>> > +}</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > /*</FONT>
<BR><FONT SIZE=2>> > void</FONT>
<BR><FONT SIZE=2>> > mlnx_names_from_guid(</FONT>
<BR><FONT SIZE=2>> > Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.c</FONT>
<BR><FONT SIZE=2>> > ===================================================================</FONT>
<BR><FONT SIZE=2>> > --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.c</FONT>
<BR><FONT SIZE=2>> > (revision 438)</FONT>
<BR><FONT SIZE=2>> > +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.c</FONT>
<BR><FONT SIZE=2>> > (revision 439)</FONT>
<BR><FONT SIZE=2>> > @@ -540,8 +540,11 @@</FONT>
<BR><FONT SIZE=2>> > p_io_stack->Parameters.QueryInterface.Version = 1;</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> You must increment the version number if you change the </FONT>
<BR><FONT SIZE=2>> interface definition.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > p_io_stack->Parameters.QueryInterface.Size = 0;</FONT>
<BR><FONT SIZE=2>> > p_io_stack->Parameters.QueryInterface.Interface =</FONT>
<BR><FONT SIZE=2>> > NULL;</FONT>
<BR><FONT SIZE=2>> > -</FONT>
<BR><FONT SIZE=2>> > p_io_stack->Parameters.QueryInterface.InterfaceSpecificData</FONT>
<BR><FONT SIZE=2>> > =</FONT>
<BR><FONT SIZE=2>> > - &p_ext->hca.hh_hndl;</FONT>
<BR><FONT SIZE=2>> > + {</FONT>
<BR><FONT SIZE=2>> > + void *p = &p_ext->hca.s;</FONT>
<BR><FONT SIZE=2>> > + memset( p, 0, sizeof(p_ext->hca.s) );</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Why not just do memset( &p_ext->hca.s, 0, sizeof(p_ext->hca.s) )?</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > p_io_stack->Parameters.QueryInterface.InterfaceSpecificData</FONT>
<BR><FONT SIZE=2>> > = p;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Why not just use &p_ext->hca.s rather than the local variable p? This</FONT>
<BR><FONT SIZE=2>> would make the code more readable.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > p_io_stack->Parameters.QueryInterface.InterfaceType</FONT>
<BR><FONT SIZE=2>> > =</FONT>
<BR><FONT SIZE=2>> > &GUID_MD_INTERFACE;</FONT>
<BR><FONT SIZE=2>> > p_irp->IoStatus.Status = STATUS_NOT_SUPPORTED;</FONT>
<BR><FONT SIZE=2>> > @@ -562,7 +565,8 @@</FONT>
<BR><FONT SIZE=2>> > ("Query interface for HCA handle </FONT>
<BR><FONT SIZE=2>> returned %08x.\n",</FONT>
<BR><FONT SIZE=2>> > status) );</FONT>
<BR><FONT SIZE=2>> > return status;</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > -</FONT>
<BR><FONT SIZE=2>> > + p_ext->hca.hh_hndl = p_ext->hca.s.hh_hndl;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Why duplicate the handle. Just change use of p_ext->hca.hh_hndl to</FONT>
<BR><FONT SIZE=2>> p_ext->hca.s.hh_hndl and save yourself the trouble.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > HCA_EXIT( HCA_DBG_PNP );</FONT>
<BR><FONT SIZE=2>> > return status;</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > @@ -1296,6 +1300,107 @@</FONT>
<BR><FONT SIZE=2>> > return status;</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > +static NTSTATUS</FONT>
<BR><FONT SIZE=2>> > +__map_crspace(</FONT>
<BR><FONT SIZE=2>> > + IN mlnx_hob_t </FONT>
<BR><FONT SIZE=2>> *</FONT>
<BR><FONT SIZE=2>> > p_hob,</FONT>
<BR><FONT SIZE=2>> > + IN PVOID</FONT>
<BR><FONT SIZE=2>> > p_buf,</FONT>
<BR><FONT SIZE=2>> > + IN ULONG</FONT>
<BR><FONT SIZE=2>> > buf_size</FONT>
<BR><FONT SIZE=2>> > + )</FONT>
<BR><FONT SIZE=2>> > +{</FONT>
<BR><FONT SIZE=2>> > + NTSTATUS status;</FONT>
<BR><FONT SIZE=2>> > + PMDL mdl_p;</FONT>
<BR><FONT SIZE=2>> > + mlnx_hca_t *p_hca =</FONT>
<BR><FONT SIZE=2>> > mlnx_hca_from_hh_hndl(p_hob->hh_hndl);</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Rather than this, please change the definition of mlnx_hob_t to point</FONT>
<BR><FONT SIZE=2>> to the mlnx_hca_t. This should be trivial, and can replace the copy</FONT>
<BR><FONT SIZE=2>> of the device object pointer at hca_verbs.c@114</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> When you change mlnx_hob_t, please remove the copy of the device</FONT>
<BR><FONT SIZE=2>> object pointer as well as the copy of the hh_hndl, since both of these</FONT>
<BR><FONT SIZE=2>> can be accessed from the mlnx_hca_t.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Once you do that, you can eliminate this lookup function all together.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > + PVOID ua, ka;</FONT>
<BR><FONT SIZE=2>> > + ULONG sz;</FONT>
<BR><FONT SIZE=2>> > + struct _map_crspace *res_p = (struct _map_crspace *)p_buf;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + HCA_ENTER( HCA_DBG_PNP );</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // sanity checks</FONT>
<BR><FONT SIZE=2>> > + if ( buf_size < sizeof *res_p ) {</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_INVALID_PARAMETER;</FONT>
<BR><FONT SIZE=2>> > + goto out;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > + if (p_hca == NULL) {</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_UNSUCCESSFUL;</FONT>
<BR><FONT SIZE=2>> > + goto out;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > + ka = p_hca->s.kernel_crspace_addr;</FONT>
<BR><FONT SIZE=2>> > + sz = p_hca->s.kernel_crspace_size;</FONT>
<BR><FONT SIZE=2>> > + if ( sz == 0 || ka == NULL) {</FONT>
<BR><FONT SIZE=2>> > + HCA_TRACE( HCA_DBG_ERROR, ("No kernel mapping of CR</FONT>
<BR><FONT SIZE=2>> > space.\n") );</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_UNSUCCESSFUL;</FONT>
<BR><FONT SIZE=2>> > + goto out;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // prepare for mapping to user space</FONT>
<BR><FONT SIZE=2>> > + mdl_p = IoAllocateMdl( ka, sz, FALSE,FALSE,NULL);</FONT>
<BR><FONT SIZE=2>> > + if (mdl_p == NULL) {</FONT>
<BR><FONT SIZE=2>> > + HCA_TRACE( HCA_DBG_ERROR, ("IoAllocateMdl </FONT>
<BR><FONT SIZE=2>> failed.\n") );</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_INSUFFICIENT_RESOURCES;</FONT>
<BR><FONT SIZE=2>> > + goto out;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // fill MDL</FONT>
<BR><FONT SIZE=2>> > + MmBuildMdlForNonPagedPool(mdl_p);</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // map the buffer into user space</FONT>
<BR><FONT SIZE=2>> > + ua = MmMapLockedPagesSpecifyCache( mdl_p, UserMode, </FONT>
<BR><FONT SIZE=2>> MmNonCached,</FONT>
<BR><FONT SIZE=2>> > + NULL, FALSE, NormalPagePriority );</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> This must be done withing a try/except block. See the docs for</FONT>
<BR><FONT SIZE=2>> MmBuildMdlForNonPagedPool.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > + if (ua == NULL) {</FONT>
<BR><FONT SIZE=2>> > + HCA_TRACE( HCA_DBG_ERROR, </FONT>
<BR><FONT SIZE=2>> ("MmMapLockedPagesSpecifyCache</FONT>
<BR><FONT SIZE=2>> > failed.\n") );</FONT>
<BR><FONT SIZE=2>> > + IoFreeMdl( mdl_p );</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_UNSUCCESSFUL;</FONT>
<BR><FONT SIZE=2>> > + goto out;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // fill the structure</FONT>
<BR><FONT SIZE=2>> > + res_p->va = ua;</FONT>
<BR><FONT SIZE=2>> > + res_p->size = sz;</FONT>
<BR><FONT SIZE=2>> > + res_p->ctx = mdl_p;</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_SUCCESS;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > +out:</FONT>
<BR><FONT SIZE=2>> > + HCA_EXIT( HCA_DBG_PNP );</FONT>
<BR><FONT SIZE=2>> > + return status;</FONT>
<BR><FONT SIZE=2>> > +}</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> There must be some tracking of the mapped CR space to prevent two apps</FONT>
<BR><FONT SIZE=2>> from mapping the space simulatneously, as well as to unmap the space</FONT>
<BR><FONT SIZE=2>> if the app crashes or doesn't issue the unmap call - basically, if the</FONT>
<BR><FONT SIZE=2>> CA instance is closed, any mappings must be undone.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > +static NTSTATUS</FONT>
<BR><FONT SIZE=2>> > +__unmap_crspace(</FONT>
<BR><FONT SIZE=2>> > + IN PVOID</FONT>
<BR><FONT SIZE=2>> > p_buf,</FONT>
<BR><FONT SIZE=2>> > + IN ULONG</FONT>
<BR><FONT SIZE=2>> > buf_size</FONT>
<BR><FONT SIZE=2>> > + )</FONT>
<BR><FONT SIZE=2>> > +{</FONT>
<BR><FONT SIZE=2>> > + NTSTATUS status;</FONT>
<BR><FONT SIZE=2>> > + PMDL mdl_p;</FONT>
<BR><FONT SIZE=2>> > + PVOID ua;</FONT>
<BR><FONT SIZE=2>> > + struct _unmap_crspace *parm = (struct </FONT>
<BR><FONT SIZE=2>> _unmap_crspace *)p_buf;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + HCA_ENTER( HCA_DBG_PNP );</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // sanity checks</FONT>
<BR><FONT SIZE=2>> > + if ( buf_size < sizeof *parm ) {</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_INVALID_PARAMETER;</FONT>
<BR><FONT SIZE=2>> > + goto out;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > + mdl_p = parm->ctx;</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> You can't trust user-mode to hand you the context. You're basically</FONT>
<BR><FONT SIZE=2>> letting a user-mode process give you a kernel pointer that you will</FONT>
<BR><FONT SIZE=2>> dereference. This is a huge security vulnerability. Better to remove</FONT>
<BR><FONT SIZE=2>> the context from the IOCTL interface all together and store it in the</FONT>
<BR><FONT SIZE=2>> CA instance.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > + ua = parm->va;</FONT>
<BR><FONT SIZE=2>> > + if ( mdl_p == NULL || ua == NULL) {</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_INVALID_PARAMETER;</FONT>
<BR><FONT SIZE=2>> > + goto out;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + // do the work</FONT>
<BR><FONT SIZE=2>> > + MmUnmapLockedPages(ua, mdl_p);</FONT>
<BR><FONT SIZE=2>> > + IoFreeMdl( mdl_p );</FONT>
<BR><FONT SIZE=2>> > + status = STATUS_SUCCESS;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > +out:</FONT>
<BR><FONT SIZE=2>> > + HCA_EXIT( HCA_DBG_PNP );</FONT>
<BR><FONT SIZE=2>> > + return status;</FONT>
<BR><FONT SIZE=2>> > +}</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > ib_api_status_t</FONT>
<BR><FONT SIZE=2>> > fw_access_ctrl(</FONT>
<BR><FONT SIZE=2>> > IN const void* __ptr64</FONT>
<BR><FONT SIZE=2>> > p_context,</FONT>
<BR><FONT SIZE=2>> > @@ -1335,6 +1440,14 @@</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > switch ( p_ci->command )</FONT>
<BR><FONT SIZE=2>> > {</FONT>
<BR><FONT SIZE=2>> > + case FW_MAP_CRSPACE:</FONT>
<BR><FONT SIZE=2>> > + status = __map_crspace(p_hob, </FONT>
<BR><FONT SIZE=2>> p_data, length);</FONT>
<BR><FONT SIZE=2>> > + break;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > + case FW_UNMAP_CRSPACE:</FONT>
<BR><FONT SIZE=2>> > + status = __unmap_crspace(p_data, length);</FONT>
<BR><FONT SIZE=2>> > + break;</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > case FW_READ: // read data from flash</FONT>
<BR><FONT SIZE=2>> > if ( if_ready )</FONT>
<BR><FONT SIZE=2>> > {</FONT>
<BR><FONT SIZE=2>> > Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.h</FONT>
<BR><FONT SIZE=2>> > ===================================================================</FONT>
<BR><FONT SIZE=2>> > --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.h</FONT>
<BR><FONT SIZE=2>> > (revision 438)</FONT>
<BR><FONT SIZE=2>> > +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.h</FONT>
<BR><FONT SIZE=2>> > (revision 439)</FONT>
<BR><FONT SIZE=2>> > @@ -225,7 +225,12 @@</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > typedef struct {</FONT>
<BR><FONT SIZE=2>> > cl_list_item_t list_item;</FONT>
<BR><FONT SIZE=2>> > - HH_hca_hndl_t hh_hndl;</FONT>
<BR><FONT SIZE=2>> > + HH_hca_hndl_t hh_hndl;</FONT>
<BR><FONT SIZE=2>> > + struct _hca_if {</FONT>
<BR><FONT SIZE=2>> > + HH_hca_hndl_t hh_hndl;</FONT>
<BR><FONT SIZE=2>> > + void * kernel_crspace_addr;</FONT>
<BR><FONT SIZE=2>> > + ULONG kernel_crspace_size;</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Define this structure in MdCard.h just above the definition of the</FONT>
<BR><FONT SIZE=2>> GUID (but within the conditanal include #ifdef) for the interface so</FONT>
<BR><FONT SIZE=2>> that they're kept together.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > + } s;</FONT>
<BR><FONT SIZE=2>> > // char *hca_name_p;</FONT>
<BR><FONT SIZE=2>> > net64_t guid;</FONT>
<BR><FONT SIZE=2>> > const void* __ptr64 p_dev_obj; // </FONT>
<BR><FONT SIZE=2>> hca device object</FONT>
<BR><FONT SIZE=2>> > @@ -373,6 +378,10 @@</FONT>
<BR><FONT SIZE=2>> > mlnx_hca_from_guid(</FONT>
<BR><FONT SIZE=2>> > IN ib_net64_t</FONT>
<BR><FONT SIZE=2>> > guid );</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > +mlnx_hca_t*</FONT>
<BR><FONT SIZE=2>> > +mlnx_hca_from_hh_hndl(</FONT>
<BR><FONT SIZE=2>> > + IN HH_hca_hndl_t</FONT>
<BR><FONT SIZE=2>> > hh_hndl );</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > /*</FONT>
<BR><FONT SIZE=2>> > void</FONT>
<BR><FONT SIZE=2>> > mlnx_names_from_guid(</FONT>
<BR><FONT SIZE=2>> > Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.h</FONT>
<BR><FONT SIZE=2>> > ===================================================================</FONT>
<BR><FONT SIZE=2>> > --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.h</FONT>
<BR><FONT SIZE=2>> > (revision 438)</FONT>
<BR><FONT SIZE=2>> > +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.h</FONT>
<BR><FONT SIZE=2>> > (revision 439)</FONT>
<BR><FONT SIZE=2>> > @@ -129,11 +129,25 @@</FONT>
<BR><FONT SIZE=2>> > #define CPUMODE_MSK (0xc0000000UL)</FONT>
<BR><FONT SIZE=2>> > #define CPUMODE_SHIFT (30)</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > +/* buffer structure for FW_MAP_CRBASE */</FONT>
<BR><FONT SIZE=2>> > +struct _map_crspace {</FONT>
<BR><FONT SIZE=2>> > + PVOID va; /* address of </FONT>
<BR><FONT SIZE=2>> CRSPACE, mapped to</FONT>
<BR><FONT SIZE=2>> > user space */</FONT>
<BR><FONT SIZE=2>> > + PVOID ctx; /* opaque operation </FONT>
<BR><FONT SIZE=2>> context; to be</FONT>
<BR><FONT SIZE=2>> > used in FW_UNMAP_CRBASE */</FONT>
<BR><FONT SIZE=2>> > + ULONG size; /* size of CRSPACE, mapped </FONT>
<BR><FONT SIZE=2>> to user space */</FONT>
<BR><FONT SIZE=2>> > +};</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > +struct _unmap_crspace {</FONT>
<BR><FONT SIZE=2>> > + PVOID va; /* address of </FONT>
<BR><FONT SIZE=2>> CRSPACE, mapped to</FONT>
<BR><FONT SIZE=2>> > user space */</FONT>
<BR><FONT SIZE=2>> > + PVOID ctx; /* operation </FONT>
<BR><FONT SIZE=2>> context, received in</FONT>
<BR><FONT SIZE=2>> > FW_MAP_CRBASE */</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> You can't trust user-mode to pass you valid kernel mode pointers.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > +};</FONT>
<BR><FONT SIZE=2>> > +</FONT>
<BR><FONT SIZE=2>> > /* Definitions intended to become shared with UM. Later... */</FONT>
<BR><FONT SIZE=2>> > #define FW_READ 0x00</FONT>
<BR><FONT SIZE=2>> > #define FW_WRITE 0x01</FONT>
<BR><FONT SIZE=2>> > #define FW_READ_CMD 0x08</FONT>
<BR><FONT SIZE=2>> > #define FW_WRITE_CMD 0x09</FONT>
<BR><FONT SIZE=2>> > +#define FW_MAP_CRSPACE 0x0A</FONT>
<BR><FONT SIZE=2>> > +#define FW_UNMAP_CRSPACE 0x0B</FONT>
<BR><FONT SIZE=2>> > #define FW_OPEN_IF 0xe7</FONT>
<BR><FONT SIZE=2>> > #define FW_CLOSE_IF 0x7e</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > _______________________________________________</FONT>
<BR><FONT SIZE=2>> > openib-windows mailing list</FONT>
<BR><FONT SIZE=2>> > openib-windows@openib.org</FONT>
<BR><FONT SIZE=2>> > <A HREF="http://openib.org/mailman/listinfo/openib-windows" TARGET="_blank">http://openib.org/mailman/listinfo/openib-windows</A></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
</BODY>
</HTML>