[Openib-windows] RE: [PATCH] MTHCA: Secure FW update
Leonid Keller
leonid at mellanox.co.il
Sun Apr 2 01:42:15 PST 2006
Thank you, commited to rev 270
> -----Original Message-----
> From: Fab Tillier [mailto:ftillier at silverstorm.com]
> Sent: Friday, March 31, 2006 11:26 PM
> To: Leonid Keller
> Cc: openib-windows at openib.org
> Subject: [PATCH] MTHCA: Secure FW update
>
> Hi Leo,
>
> This patch performs some cleanup of the FW update paths. The
> changes are:
>
> - The fw_access_ctrl function needs to return
> ib_api_status_t, not NTSTATUS.
> This is a bug that was carried over from the MT23108 driver
> (I'll send a patch for that soon).
>
> - There is no need to track the process ID in the CR space
> mappings - there is no way for the process ID to change for a
> particular h_um_ca handle, as it is tied to an IBAL instance
> which is tied to a single process.
>
> - MmMapLockedPagesSpecifyCache for UserMode throws and
> exception if it fails, rather than returning NULL like for
> kernel calls. This needs to be trapped with __try/__except.
>
> - Added some checks on buffers - it is possible for buf_len
> to be non-zero while p_buf is NULL.
>
> - Fab
>
> Index: hw/mthca/kernel/hca_driver.c
> ===================================================================
> --- hw/mthca/kernel/hca_driver.c (revision 266)
> +++ hw/mthca/kernel/hca_driver.c (working copy)
> @@ -345,6 +345,9 @@
>
> PAGED_CODE();
>
> + if( !p_buffer )
> + return STATUS_INVALID_PARAMETER;
> +
> if (p_BusInterface)
> {
>
> @@ -439,7 +442,6 @@
> struct list_head list;
> PMDL p_mdl;
> PVOID va;
> - PEPROCESS p_pcs;
> } mthca_map_space;
>
> static NTSTATUS
> @@ -461,7 +463,7 @@
> HCA_ENTER( HCA_DBG_PNP );
>
> // sanity checks
> - if ( buf_size < sizeof *p_res ) {
> + if ( buf_size < sizeof *p_res || !p_buf ) {
> status = STATUS_INVALID_PARAMETER;
> goto err_invalid_params;
> }
> @@ -485,7 +487,7 @@
> if ( ka == NULL) {
> HCA_PRINT(TRACE_LEVEL_ERROR , HCA_DBG_SHIM,
> ("No kernel mapping of CR space.\n") );
> - status = STATUS_UNSUCCESSFUL;
> + status = STATUS_INSUFFICIENT_RESOURCES;
> goto err_map_to_kernel;
> }
> p_ext->bar[HCA_BAR_TYPE_HCR].virt = ka; @@
> -505,12 +507,16 @@
> MmBuildMdlForNonPagedPool(p_mdl);
>
> // map the buffer into user space
> - ua = MmMapLockedPagesSpecifyCache( p_mdl, UserMode,
> MmNonCached,
> - NULL, FALSE, NormalPagePriority );
> - if (ua == NULL) {
> - HCA_PRINT(TRACE_LEVEL_ERROR , HCA_DBG_SHIM,
> + __try
> + {
> + ua = MmMapLockedPagesSpecifyCache( p_mdl,
> UserMode, MmNonCached,
> + NULL, FALSE, NormalPagePriority );
> + }
> + __except(EXCEPTION_EXECUTE_HANDLER)
> + {
> + HCA_PRINT(TRACE_LEVEL_ERROR , HCA_DBG_SHIM,
> ("MmMapLockedPagesSpecifyCache failed.\n") );
> - status = STATUS_UNSUCCESSFUL;
> + status = STATUS_INSUFFICIENT_RESOURCES;
> goto err_map_to_user;
> }
>
> @@ -521,7 +527,6 @@
> // resource tracking
> p_map->p_mdl = p_mdl;
> p_map->va = ua;
> - p_map->p_pcs = PsGetCurrentProcess();
> list_add_tail(&p_map->list, &p_context->map_list);
>
> up( &p_context->mutex );
> @@ -553,12 +558,11 @@
> unmap_crspace *parm = (unmap_crspace *)p_buf;
> mthca_map_space *p_map, *p_tmp;
> int found = FALSE;
> - PEPROCESS p_pcs = PsGetCurrentProcess();
>
> HCA_ENTER( HCA_DBG_PNP );
>
> // sanity checks
> - if ( buf_size < sizeof *parm ) {
> + if ( buf_size < sizeof *parm || !p_buf ) {
> status = STATUS_INVALID_PARAMETER;
> goto err_invalid_params;
> }
> @@ -569,7 +573,7 @@
> // look for the mapping info
> list_for_each_entry_safe(p_map, p_tmp,
> &p_context->map_list, list,
> mthca_map_space, mthca_map_space) {
> - if (p_map->va == parm->va && p_map->p_pcs == p_pcs) {
> + if (p_map->va == parm->va) {
> found = TRUE;
> break;
> }
> @@ -579,7 +583,7 @@
> HCA_PRINT(TRACE_LEVEL_ERROR , HCA_DBG_SHIM,
> ("Not found internal info for
> unmappipng.%p for PID %d.\n" ,
> parm->va,
> (int)(INT_PTR)PsGetCurrentProcessId()));
> - status = STATUS_UNSUCCESSFUL;
> + status = STATUS_INVALID_PARAMETER;
> goto err_not_found;
> }
>
> @@ -638,7 +642,6 @@
> PVOID p_data;
> ULONG offset;
> ULONG POINTER_ALIGNMENT length;
> - ib_ci_op_t *p_ci;
> mlnx_hob_t *p_hob;
> struct ib_ucontext * p_context;
>
> @@ -646,28 +649,23 @@
> UNREFERENCED_PARAMETER(num_handles);
> UNREFERENCED_PARAMETER(p_umv_buf);
>
> - status = STATUS_SUCCESS;
> + status = STATUS_INVALID_DEVICE_REQUEST;
>
> - if( p_umv_buf ) {
> - p_context = (struct ib_ucontext *)h_ca;
> - p_hob = HOB_FROM_IBDEV( ((struct
> ib_ucontext*)h_ca)->device );
> - }
> - else {
> - p_context = NULL;
> - p_hob = (mlnx_hob_t *)(const void *)h_ca;
> - }
> + if( !p_umv_buf )
> + return IB_UNSUPPORTED;
>
> + p_context = (struct ib_ucontext *)h_ca;
> + p_hob = HOB_FROM_IBDEV( ((struct ib_ucontext*)h_ca)->device );
> p_dev_obj = (DEVICE_OBJECT
> *)EXT_FROM_HOB(p_hob)->cl_ext.p_self_do;
> - p_ci = p_ci_op;
>
> - if ( !p_ci || !p_ci->buf_size )
> - return STATUS_INVALID_DEVICE_REQUEST;
> + if ( !p_ci_op || !p_ci_op->buf_size )
> + return IB_INVALID_PARAMETER;
>
> - length = p_ci->buf_size;
> - offset = p_ci->buf_info;
> - p_data = p_ci->p_buf;
> + length = p_ci_op->buf_size;
> + offset = p_ci_op->buf_info;
> + p_data = p_ci_op->p_buf;
>
> - switch ( p_ci->command )
> + switch ( p_ci_op->command )
> {
> case FW_MAP_CRSPACE:
> status = __map_crspace(p_context, p_hob,
> p_data, length); @@ -703,7 +701,7 @@
> if_ready = 0;
>
> BusInterface.InterfaceDereference((PVOID)BusInterface.Context);
> }
> - return status;
> + return IB_SUCCESS;
>
> case FW_OPEN_IF: // open BusInterface
> if ( !if_ready )
> @@ -713,13 +711,16 @@
> if ( NT_SUCCESS( status ) )
> {
> if_ready = 1;
> - status = STATUS_SUCCESS;
> }
> }
> - return status;
> + else
> + {
> + status = STATUS_SUCCESS;
> + }
> + break;
>
> default:
> - status = STATUS_NOT_SUPPORTED;
> + status = STATUS_INVALID_DEVICE_REQUEST;
> }
>
> if ( status != STATUS_SUCCESS )
> @@ -730,9 +731,22 @@
>
> BusInterface.InterfaceDereference((PVOID)BusInterface.Context);
> }
> HCA_PRINT( TRACE_LEVEL_ERROR, HCA_DBG_INIT,
> - ("fw_access_ctrl failed returns
> %08x.\n", status));
> + ("fw_access_ctrl failed, ntstatus:
> %08x.\n", status));
> }
> - return status;
> + switch( status )
> + {
> + case STATUS_SUCCESS:
> + return IB_SUCCESS;
> +
> + case STATUS_INVALID_DEVICE_REQUEST:
> + return IB_UNSUPPORTED;
> +
> + case STATUS_INSUFFICIENT_RESOURCES:
> + return IB_INSUFFICIENT_RESOURCES;
> +
> + default:
> + return IB_ERROR;
> + }
> }
>
> static NTSTATUS
>
More information about the ofw
mailing list