[ofw][PATCH] [IBBUS][WinVerbs][SDP] Validation of non-zerosizewhen calling to ExAllocatePoolWithTag
Alex Naslednikov
xalex at mellanox.co.il
Mon Oct 26 11:53:19 PDT 2009
I got your point.
The problem is that we received zero-size only when running verifier
with a low-mem simulation.
It's not a normal situation, but our code isn't ready for this case.
Take into account that only "problemmatic" calls to
ExAllocatePoolWithTag were replaced; i.e all call were chances to get
zero-size > 0.
ASSERT may be good for a check version, but I don't believe we can find
and eliminate all the problemmatic cases by doing so.
-----Original Message-----
From: Fab Tillier [mailto:ftillier at microsoft.com]
Sent: Monday, October 26, 2009 8:40 PM
To: Alex Naslednikov; ofw at lists.openfabrics.org; Leonid Keller
Subject: RE: [ofw][PATCH] [IBBUS][WinVerbs][SDP] Validation of
non-zerosizewhen calling to ExAllocatePoolWithTag
Hi Alex,
Alex Naslednikov wrote on Mon, 26 Oct 2009 at 11:35:57:
> Because you will get BSOD when running verifier with low-resources.
> And why to call this function with zero when it's not advisable ?
Right, that's my point - you should assert instead of trapping it, so
that instances of this bad call are found and the code fixed. Your
changes are just suppressing the problem, rather than solving it.
The assert should happen before you get the BSOD with verifier, and you
will end up fixing all code paths that might call the function with a
size of zero, thus removing the need for your ExSafe wrapper functions.
-Fab
>
> -----Original Message-----
> From: Fab Tillier [mailto:ftillier at microsoft.com]
> Sent: Monday, October 26, 2009 8:31 PM
> To: Alex Naslednikov; ofw at lists.openfabrics.org; Leonid Keller
> Subject: RE: [ofw][PATCH] [IBBUS][WinVerbs][SDP] Validation of
> non-zero sizewhen calling to ExAllocatePoolWithTag
>
> Why not just assert that size isn't zero, rather than suppressing the
> error and returning?
>
> -Fab
>
>
>> -----Original Message-----
>> From: ofw-bounces at lists.openfabrics.org [mailto:ofw-
>> bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
>> Sent: Monday, October 26, 2009 11:03 AM
>> To: ofw at lists.openfabrics.org; Leonid Keller
>> Subject: [ofw][PATCH] [IBBUS][WinVerbs][SDP] Validation of non-zero
>> size when calling to ExAllocatePoolWithTag
>>
>> According to WinDDK, "calling ExAllocatePoolWithTag with memory size
>> == 0 will result in pool header wastage"
>> In addition, verifier with low mem simulation will crash when calling
>> the mentioned function with memory size == 0 This patch fixes this
>> problem by replacing unsafe call with appropriate macro signed-off
> by:
>
>> Alexander Naslednikov (xalex at mellanox.co.il)
>>
>> Index: D:/windows/MLNX_WinOF_trunk/ulp/sdp/kernel/SdpGenUtils.cpp
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/ulp/sdp/kernel/SdpGenUtils.cpp
>> (revision 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/ulp/sdp/kernel/SdpGenUtils.cpp
>> (revision 4992)
>> @@ -372,14 +372,22 @@
>> return WSAEINVAL;
>> }
>> }
>> +class ZeroSizePool {
>> +} szPool;
>>
>> void* __cdecl operator new(size_t n ) throw() {
>> +
>> + //From WinDDK: "Avoid calling with memory size == 0. Doing so will
>> result in pool header wastage"
>> + // Verifier with low mem simulation will crash with memory size ==
>> + 0 if (n ==0) return &szPool;
>> ASSERT(n != 0x30);
>> return ExAllocatePoolWithTag(NonPagedPool , n,
>> GLOBAL_ALLOCATION_TAG); }
>>
>> void __cdecl operator delete(void* p) {
>> - ExFreePoolWithTag(p, GLOBAL_ALLOCATION_TAG);
>> + if (p != &szPool)
>> + ExFreePoolWithTag(p, GLOBAL_ALLOCATION_TAG);
>> }
>>
>> void* __cdecl operator new(size_t n, void *addr ) throw() {
>> Index: D:/windows/MLNX_WinOF_trunk/core/winmad/kernel/wm_driver.c
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/core/winmad/kernel/wm_driver.c
>> (revision 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/core/winmad/kernel/wm_driver.c
>> (revision 4992)
>> @@ -238,8 +238,8 @@
>> attr = NULL;
>> goto out;
>> }
>> -
>> - attr = ExAllocatePoolWithTag(PagedPool, size, 'acmw');
>> +
>> + attr = ExAllocatePoolWithTagSafeEx(PagedPool, size, 'acmw');
>> if (attr == NULL) {
>> goto out;
>> } @@ -269,7 +269,8 @@ }
>>
>> size = sizeof(WM_IB_PORT) * attr->num_ports;
>> - pDevice->pPortArray = ExAllocatePoolWithTag(PagedPool, size,
>> 'pimw');
>> +
>> + pDevice->pPortArray = ExAllocatePoolWithTagSafeEx(PagedPool, size,
>> 'pimw') ;
>> if (pDevice->pPortArray == NULL) {
>> status = STATUS_NO_MEMORY;
>> goto out;
>> Index: D:/windows/MLNX_WinOF_trunk/core/winverbs/kernel/wv_device.c
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/core/winverbs/kernel/wv_device.c
>> (revision 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/core/winverbs/kernel/wv_device.c
>> (revision 4992)
>> @@ -178,8 +178,8 @@
>> attr = NULL;
>> goto out;
>> }
>> -
>> - attr = ExAllocatePoolWithTag(PagedPool, size, 'acvw');
>> +
>> + attr = ExAllocatePoolWithTagSafeEx(PagedPool, size, 'acvw');
>> if (attr == NULL) {
>> goto out;
>> } @@ -210,7 +210,7 @@ pDevice->PortCount = attr->num_ports;
>> ExFreePoolWithTag(attr, 'acvw');
>> - pDevice->pPorts = ExAllocatePoolWithTag(NonPagedPool,
>> sizeof(WV_PORT)
>> *
>> + pDevice->pPorts = ExAllocatePoolWithTagSafeEx(NonPagedPool,
>> sizeof(WV_PORT) *
>> pDevice->PortCount, 'cpvw');
>> if (pDevice->pPorts == NULL) {
>> return STATUS_NO_MEMORY;
>> Index:
>> D:/windows/MLNX_WinOF_trunk/core/complib/kernel/cl_memory_osd.c
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/core/complib/kernel/cl_memory_osd.c
>> (revision 4987) +++
>> D:/windows/MLNX_WinOF_trunk/core/complib/kernel/cl_memory_osd.c
>> (revision 4992) @@ -38,6 +38,7 @@
>> IN const size_t size,
>> IN const boolean_t pageable )
>> {
>> +
>> if( pageable )
>> {
>> CL_ASSERT( KeGetCurrentIrql() < DISPATCH_LEVEL ); @@ -46,7 +47,7
> @@
>> else
>> {
>> CL_ASSERT( KeGetCurrentIrql() <= DISPATCH_LEVEL );
>> - return( ExAllocatePoolWithTag( NonPagedPool, size, 'virp' ) );
>> + return( ExAllocatePoolWithTagSafeEx( NonPagedPool, size, 'virp' )
>> + );
>> }
>> }
>> Index: D:/windows/MLNX_WinOF_trunk/core/bus/kernel/bus_port_mgr.c
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/core/bus/kernel/bus_port_mgr.c
>> (revision 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/core/bus/kernel/bus_port_mgr.c
>> (revision 4992)
>> @@ -1599,7 +1599,7 @@
>> dev_id_size = p_ext->pdo.p_pdo_device_info->device_id_size;
>>
>> /* Device ID is "IBA\SID_<sid> where <sid> is the IO device Service
>> ID. */
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, dev_id_size, 'vedq'
>> );
>> + p_string = ExAllocatePoolWithTagSafeEx( NonPagedPool, dev_id_size,
>> 'vedq' );
>> if( !p_string )
>> {
>> BUS_TRACE_EXIT( BUS_DBG_ERROR,
>> @@ -1635,7 +1635,7 @@
>>
>> dev_id_size = p_ext->pdo.p_pdo_device_info->hardware_id_size;
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, dev_id_size, 'ihqp'
>> );
>> + p_string = ExAllocatePoolWithTagSafeEx( NonPagedPool, dev_id_size,
>> 'ihqp' );
>> if( !p_string )
>> {
>> BUS_TRACE_EXIT( BUS_DBG_ERROR,
>> @@ -1669,8 +1669,8 @@
>> p_ext = (bus_port_ext_t*)p_dev_obj->DeviceExtension;
>>
>> dev_id_size = p_ext->pdo.p_pdo_device_info->compatible_id_size;
>> -
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, dev_id_size, 'ihqp'
>> );
>> +
>> + p_string = ExAllocatePoolWithTagSafeEx( NonPagedPool, dev_id_size,
>> 'ihqp' );
>> if( !p_string )
>> {
>> BUS_TRACE_EXIT( BUS_DBG_ERROR, @@ -1753,9 +1753,8 @@ return
>> STATUS_NO_SUCH_DEVICE;
>> }
>> + p_string = ExAllocatePoolWithTagSafeEx( NonPagedPool, p_ext-
>>> pdo.p_pdo_device_info->description_size, 'edqp' );
>>
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, p_ext-
>>> pdo.p_pdo_device_info->description_size, 'edqp' );
>> -
>> if( !p_string )
>> {
>> BUS_TRACE_EXIT( BUS_DBG_ERROR,
>> Index: D:/windows/MLNX_WinOF_trunk/core/iou/kernel/iou_ioc_mgr.c
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/core/iou/kernel/iou_ioc_mgr.c
>> (revision
>> 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/core/iou/kernel/iou_ioc_mgr.c
>> +++ (revision
>> 4992)
>> @@ -952,8 +952,9 @@
>> {
>> dev_id_size = (p_ext->pdo.p_pdo_device_info)->device_id_size;
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, dev_id_size,
'didq'
>> );
>>
>> + p_string = ExAllocatePoolWithTagSafeEx( NonPagedPool, dev_id_size,
>> 'didq' );
>> +
>> if( !p_string )
>> {
>> IOU_PRINT_EXIT( TRACE_LEVEL_ERROR, IOU_DBG_ERROR, @@ -1027,7
>> +1028,7 @@
>> {
>> hw_id_size = p_ext->pdo.p_pdo_device_info->hardware_id_size;
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, hw_id_size, 'ihqi'
>> );
>> + p_string = ExAllocatePoolWithTagSafeEx( NonPagedPool, hw_id_size,
>> 'ihqi' );
>> if( !p_string )
>> {
>> IOU_PRINT_EXIT( TRACE_LEVEL_ERROR, IOU_DBG_ERROR, @@ -1142,9
>> +1143,9 @@
>> {
>> compat_id_size = p_ext->pdo.p_pdo_device_info->compatible_id_size;
>> +
>> + p_string = ExAllocatePoolWithTagSafeEx( NonPagedPool,
>> compat_id_size, 'icqi' );
>>
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, compat_id_size,
>> 'icqi' );
>> -
>> if( !p_string )
>> {
>> IOU_PRINT_EXIT( TRACE_LEVEL_ERROR, IOU_DBG_ERROR, @@ -1302,7
>> +1303,7 @@
>>
>> if ( p_ext->pdo.p_pdo_device_info )
>> {
>> - p_string = ExAllocatePoolWithTag( NonPagedPool, p_ext-
>>> pdo.p_pdo_device_info->description_size, + p_string =
>>> ExAllocatePoolWithTagSafeEx( NonPagedPool, p_ext-
>>> pdo.p_pdo_device_info->description_size,
>> 'edqi' );
>> if( !p_string )
>> {
>> Index: D:/windows/MLNX_WinOF_trunk/inc/complib/cl_memory.h
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/inc/complib/cl_memory.h (revision
>> 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/inc/complib/cl_memory.h (revision
>> +++ 4992)
>> @@ -919,6 +919,21 @@
>> /*
>> * Define allocation macro.
>> */
>> + +/* From WinDDK: "Avoid calling ExAllocatePoolWithTag with memory
>> size == 0. + Doing so will result in pool header wastage" + Verifier
>> with low mem simulation will crash with memory size == 0 +*/ #define
>> ExAllocatePoolWithTagSafeEx( pageable, size, tag ) \ + (size == 0 ?
>> NULL : ExAllocatePoolWithTag(pageable, size, tag)) + +#define
>> ExAllocatePoolWithTagSafeExNonPaged(size, tag ) \ + (size == 0 ?
NULL
>> : ExAllocatePoolWithTag(NonPagedPool, size, tag )) + +#define
>> ExAllocatePoolWithTagSafeExPaged(size, tag ) \ + (size == 0 ? NULL
:
>> ExAllocatePoolWithTag(PagedPool, size, tag )) + + #if defined(
>> CL_TRACK_MEM )
>>
>> #define cl_malloc( a ) \
>> Index: D:/windows/MLNX_WinOF_trunk/etc/kernel/index_list.c
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/etc/kernel/index_list.c (revision
>> 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/etc/kernel/index_list.c (revision
>> +++ 4992)
>> @@ -28,7 +28,9 @@
>> */
>> #include "index_list.h"
>> +#include <complib/cl_memory.h>
>>
>> +
>> INDEX_ENTRY EmptyList;
>>
>> static BOOLEAN IndexListGrow(INDEX_LIST *pIndexList) @@ -37,7 +39,8
>> @@
>> SIZE_T size, i;
>>
>> size = pIndexList->Size + (PAGE_SIZE / sizeof(INDEX_ENTRY));
>> - array = ExAllocatePoolWithTag(NonPagedPool, size *
>> sizeof(INDEX_ENTRY), 'xdni');
>> +
>> + array = ExAllocatePoolWithTagSafeEx(NonPagedPool, size *
>> sizeof(INDEX_ENTRY), 'xdni');
>> if (array == NULL) {
>> return FALSE;
>> }
>> Index: D:/windows/MLNX_WinOF_trunk/hw/mlx4/kernel/inc/l2w_memory.h
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/hw/mlx4/kernel/inc/l2w_memory.h
>> (revision 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/hw/mlx4/kernel/inc/l2w_memory.h
>> (revision 4992)
>> @@ -86,13 +86,13 @@
>> ASSERT(bsize);
>> switch (gfp_mask) {
>> case GFP_ATOMIC:
>> - ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_ATOMIC
);
>> + ptr = ExAllocatePoolWithTagSafeEx( NonPagedPool, bsize,
>> MT_TAG_ATOMIC );
>> break;
>> case GFP_KERNEL:
>> - ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_KERNEL
);
>> + ptr = ExAllocatePoolWithTagSafeEx( NonPagedPool, bsize,
>> MT_TAG_KERNEL );
>> break;
>> case GFP_HIGHUSER:
>> - ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_HIGH );
>> + ptr = ExAllocatePoolWithTagSafeEx( NonPagedPool, bsize,
>> + MT_TAG_HIGH
>> );
>> break; default: cl_dbg_out("kmalloc: unsupported flag %d\n",
>> gfp_mask);
>> Index: D:/windows/MLNX_WinOF_trunk/hw/mthca/kernel/mt_memory.h
>> ===================================================================
>> --- D:/windows/MLNX_WinOF_trunk/hw/mthca/kernel/mt_memory.h (revision
>> 4987)
>> +++ D:/windows/MLNX_WinOF_trunk/hw/mthca/kernel/mt_memory.h (revision
>> 4992)
>> @@ -52,13 +52,13 @@
>> MT_ASSERT( KeGetCurrentIrql() <= DISPATCH_LEVEL );
>> switch (gfp_mask) {
>> case GFP_ATOMIC:
>> - ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_ATOMIC
);
>> + ptr = ExAllocatePoolWithTagSafeEx( NonPagedPool, bsize,
>> MT_TAG_ATOMIC );
>> break;
>> case GFP_KERNEL:
>> - ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_KERNEL
);
>> + ptr = ExAllocatePoolWithTagSafeEx( NonPagedPool, bsize,
>> MT_TAG_KERNEL );
>> break;
>> case GFP_HIGHUSER:
>> - ptr = ExAllocatePoolWithTag( NonPagedPool, bsize, MT_TAG_HIGH );
>> + ptr = ExAllocatePoolWithTagSafeEx( NonPagedPool, bsize,
>> + MT_TAG_HIGH
>> );
>> break; default: cl_dbg_out("kmalloc: unsupported flag %d\n",
>> gfp_mask);
>
More information about the ofw
mailing list