[ofw][PATCH] [IBBUS][WinVerbs][SDP] Validation of non-zerosizewhen calling to ExAllocatePoolWithTag
Fab Tillier
ftillier at microsoft.com
Mon Oct 26 12:03:33 PDT 2009
Hi Alex,
I don't understand how the size could become zero just because verifier is running. If it does, then the code is buggy, and the stack trace from the BSOD should shed light on the offending code.
Just applying a blanket fix that hides the error isn't a good solution, IMO.
-Fab
Alex Naslednikov wrote on Mon, 26 Oct 2009 at 11:53:19:
> 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