[ofw] [IPoIB] Fix for improper handling of IPoIB params
Alex Naslednikov
alexn at mellanox.co.il
Thu Jun 26 06:10:33 PDT 2008
Of course,
We found (using WHQL test) that IPoIB driver got stuck when one set
invalid registers parameters.
See
HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Class\{4D36E972-E325
-11CE-BFC1-08002bE10318}\<last_interface_number>...for the list of
parameters.
The default and min/max values of these parameters defined in
netipoib.def
There was (almost) no check on validity of these parameters, and this
patch solves this and doesn't allow driver to be initialized with wrong
parameters.
XaleX
________________________________
From: ofw-bounces at lists.openfabrics.org
[mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Estrin
Sent: Thursday, June 26, 2008 2:46 PM
To: Alex Naslednikov; ofw at lists.openfabrics.org; Tzachi Dar
Subject: RE: [ofw] [IPoIB] Fix for improper handling of IPoIB params
Would you please explain what improper handling this patch is intended
to fix?
Thanks,
Alex
________________________________
From: ofw-bounces at lists.openfabrics.org
[mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Thursday, June 26, 2008 8:00 AM
To: ofw at lists.openfabrics.org; Tzachi Dar
Subject: [ofw] [IPoIB] Fix for improper handling of IPoIB params
Reposting this message again (there was a porblem with my first
post)
________________________________
This patch fixes improper handling of IPoIB parameters received
from registers
Signed-off by: xalex (xalex at mellanox.co.il
<mailto:xalex at mellanox.co.il> )
Index: ulp/ipoib/kernel/SOURCES
===================================================================
--- ulp/ipoib/kernel/SOURCES (revision 2629)
+++ ulp/ipoib/kernel/SOURCES (revision 2630)
@@ -25,14 +25,16 @@
TARGETLIBS= \
$(TARGETPATH)\*\complib.lib \
- $(DDK_LIB_PATH)\ndis.lib
+ $(DDK_LIB_PATH)\ndis.lib \
+ $(DDK_LIB_PATH)\ntstrsafe.lib \
+ $(DDK_LIB_PATH)\strsafe.lib
!if !defined(DDK_TARGET_OS) || "$(DDK_TARGET_OS)"=="Win2K"
#
# The driver is built in the Win2K build environment
# - use the library version of safe strings
#
-TARGETLIBS= $(TARGETLIBS) $(DDK_LIB_PATH)\ntstrsafe.lib
+#TARGETLIBS= $(TARGETLIBS) $(DDK_LIB_PATH)\ntstrsafe.lib
!endif
!IFDEF ENABLE_EVENT_TRACING
Index: ulp/ipoib/kernel/ipoib_driver.c
===================================================================
--- ulp/ipoib/kernel/ipoib_driver.c (revision 2629)
+++ ulp/ipoib/kernel/ipoib_driver.c (revision 2630)
@@ -30,7 +30,7 @@
* $Id$
*/
-
+#include "limits.h"
#include "ipoib_driver.h"
#include "ipoib_debug.h"
@@ -47,8 +47,12 @@
#include <complib/cl_init.h>
#include <initguid.h>
#include <iba/ipoib_ifc.h>
+#include "ntstrsafe.h"
+#include "strsafe.h"
+
+
#if defined(NDIS50_MINIPORT)
#define MAJOR_NDIS_VERSION 5
#define MINOR_NDIS_VERSION 0
@@ -59,6 +63,8 @@
#error NDIS Version not defined, try defining NDIS50_MINIPORT
or NDIS51_MINIPORT
#endif
+PDRIVER_OBJECT g_p_drv_obj;
+
static const NDIS_OID SUPPORTED_OIDS[] =
{
OID_GEN_SUPPORTED_LIST,
@@ -125,7 +131,72 @@
uint32_t g_ipoib_dbg_flags = 0x00000fff;
ipoib_globals_t g_ipoib = {0};
+typedef struct _IPOIB_REG_ENTRY
+{
+ NDIS_STRING RegName; // variable name text
+ BOOLEAN bRequired; // 1 -> required, 0 ->
optional
+ UINT FieldOffset; // offset in parent struct
+ UINT FieldSize; // size (in bytes) of the
field
+ UINT Default; // default value to use
+ UINT Min; // minimum value allowed
+ UINT Max; // maximum value allowed
+} IPOIB_REG_ENTRY, *PIPOIB_REG_ENTRY;
+IPOIB_REG_ENTRY HCARegTable[] = {
+ // reg value name If Required Offset in parentr
struct Field size Default Min
Max
+ {NDIS_STRING_CONST("RqDepth"), 1,
IPOIB_OFFSET(rq_depth), IPOIB_SIZE(rq_depth),
512, 128, 1024},
+ {NDIS_STRING_CONST("RqLowWatermark"), 0,
IPOIB_OFFSET(rq_low_watermark), IPOIB_SIZE(rq_low_watermark), 4,
2, 8},
+ {NDIS_STRING_CONST("SqDepth"), 1,
IPOIB_OFFSET(sq_depth), IPOIB_SIZE(sq_depth),
512, 128, 1024},
+ {NDIS_STRING_CONST("SendChksum"), 1,
IPOIB_OFFSET(send_chksum_offload), IPOIB_SIZE(send_chksum_offload),0,
0, 1},
+ {NDIS_STRING_CONST("RecvChksum"), 1,
IPOIB_OFFSET(recv_chksum_offload), IPOIB_SIZE(recv_chksum_offload),0,
0, 1},
+ {NDIS_STRING_CONST("SaTimeout"), 1,
IPOIB_OFFSET(sa_timeout), IPOIB_SIZE(sa_timeout),
1000, 250, UINT_MAX},
+ {NDIS_STRING_CONST("SaRetries"), 1,
IPOIB_OFFSET(sa_retry_cnt), IPOIB_SIZE(sa_retry_cnt), 10,
1, UINT_MAX},
+ {NDIS_STRING_CONST("RecvRatio"), 1,
IPOIB_OFFSET(recv_pool_ratio), IPOIB_SIZE(recv_pool_ratio), 1,
1, 10},
+ {NDIS_STRING_CONST("PayloadMtu"), 1,
IPOIB_OFFSET(payload_mtu), IPOIB_SIZE(payload_mtu),
2044 , 60 , 2044}
+};
+
+#define IPOIB_NUM_REG_PARAMS (sizeof (HCARegTable) /
sizeof(IPOIB_REG_ENTRY))
+
+
+static void
+ipoib_create_log(
+ NDIS_HANDLE h_adapter,
+ UINT ind,
+ ULONG eventLogMsgId)
+
+{
+#define cMaxStrLen 40
+#define cArrLen 3
+
+ PWCHAR logMsgArray[cArrLen];
+ WCHAR strVal[cMaxStrLen];
+ NDIS_STRING AdapterInstanceName;
+
+ IPOIB_INIT_NDIS_STRING(&AdapterInstanceName);
+ if (NdisMQueryAdapterInstanceName(&AdapterInstanceName,
h_adapter)!= NDIS_STATUS_SUCCESS ){
+ ASSERT(FALSE);
+ IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR,IPOIB_DBG_ERROR,
("[IPoIB] Init:Failed to retreive adapter name.\n"));
+ return;
+ }
+ logMsgArray[0] = AdapterInstanceName.Buffer;
+
+ if (RtlStringCbPrintfW(strVal, sizeof(strVal), L"0x%x",
HCARegTable[ind].Default) != STATUS_SUCCESS) {
+ ASSERT(FALSE);
+ IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR,IPOIB_DBG_ERROR,
+ ("[IPoIB] Init: Problem copying string value: exiting\n"));
+ return;
+ }
+
+ logMsgArray[0] = AdapterInstanceName.Buffer;
+ logMsgArray[1] = HCARegTable[ind].RegName.Buffer;
+ logMsgArray[2] = strVal;
+
+ NdisWriteEventLogEntry(g_p_drv_obj, eventLogMsgId, 0, cArrLen,
&logMsgArray, 0, NULL);
+
+}
+
+
+
NTSTATUS
DriverEntry(
IN PDRIVER_OBJECT p_drv_obj,
@@ -248,6 +319,7 @@
NDIS_MINIPORT_CHARACTERISTICS characteristics;
IPOIB_ENTER( IPOIB_DBG_INIT );
+ g_p_drv_obj = p_drv_obj;
#ifdef _DEBUG_
PAGED_CODE();
@@ -397,6 +469,7 @@
}
+
NDIS_STATUS
ipoib_get_adapter_params(
IN NDIS_HANDLE* const wrapper_config_context,
@@ -405,9 +478,13 @@
NDIS_STATUS status;
NDIS_HANDLE h_config;
NDIS_CONFIGURATION_PARAMETER *p_param;
- NDIS_STRING keyword;
PUCHAR mac;
UINT len;
+ UINT value;
+ PIPOIB_REG_ENTRY pRegEntry;
+ UINT i;
+ PUCHAR structPointer;
+ int sq_depth_step = 128;
IPOIB_ENTER( IPOIB_DBG_INIT );
@@ -419,124 +496,96 @@
return status;
}
- /* Required: Receive queue depth. */
- RtlInitUnicodeString( &keyword, L"RqDepth" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
+ // read all the registry values
+ for (i = 0, pRegEntry = HCARegTable; i < IPOIB_NUM_REG_PARAMS;
++i)
{
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("Receive Queue Depth parameter missing.\n") );
- return status;
- }
- p_adapter->params.rq_depth =
p_param->ParameterData.IntegerData;
+ // initialize pointer to appropriate place inside 'params'
+ structPointer = (PUCHAR) &p_adapter->params +
pRegEntry[i].FieldOffset;
- /* Optional: Receive queue low watermark. */
- RtlInitUnicodeString( &keyword, L"RqLowWatermark" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS ||
!p_param->ParameterData.IntegerData )
- {
- p_adapter->params.rq_low_watermark =
p_adapter->params.rq_depth >> 2;
- }
- else
- {
- p_adapter->params.rq_low_watermark =
- p_adapter->params.rq_depth /
p_param->ParameterData.IntegerData;
- }
+ // Get the configuration value for a specific parameter.
Under NT the
+ // parameters are all read in as DWORDs.
+ NdisReadConfiguration(
+ &status,
+ &p_param,
+ h_config,
+ &pRegEntry[i].RegName,
+ NdisParameterInteger);
- /* Required: Send queue depth. */
- RtlInitUnicodeString( &keyword, L"SqDepth" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
- {
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("Send Queue Depth parameter missing.\n") );
- return status;
- }
- p_adapter->params.sq_depth =
p_param->ParameterData.IntegerData;
- /* Send queue depth needs to be a power of two. */
- if( p_adapter->params.sq_depth <= 128 )
- p_adapter->params.sq_depth = 128;
- else if( p_adapter->params.sq_depth <= 256 )
- p_adapter->params.sq_depth = 256;
- else if( p_adapter->params.sq_depth <= 512 )
- p_adapter->params.sq_depth = 512;
- else
- p_adapter->params.sq_depth = 1024;
+ // If the parameter was present, then check its value for
validity.
+ if (status == NDIS_STATUS_SUCCESS)
+ {
+ // Check that param value is not too small or too large
+ if (p_param->ParameterData.IntegerData < pRegEntry[i].Min ||
+ p_param->ParameterData.IntegerData > pRegEntry[i].Max)
+ {
+ value = pRegEntry[i].Default;
+ ipoib_create_log(p_adapter->h_adapter, i,
EVENT_IPOIB_WRONG_PARAMETER_WRN);
+ IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("Read
configuration.Registry %S value is out of range, setting default value=
0x%x\n", pRegEntry[i].RegName.Buffer, value));
- /* Required: Send Checksum Offload. */
- RtlInitUnicodeString( &keyword, L"SendChksum" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
- {
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("Send Checksum Offload parameter missing.\n") );
- return status;
- }
- p_adapter->params.send_chksum_offload =
(p_param->ParameterData.IntegerData != 0);
+ }
+ else
+ {
+ value = p_param->ParameterData.IntegerData;
+ IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("Read
configuration. Registry %S, Value= 0x%x\n", pRegEntry[i].RegName.Buffer,
value));
+ }
+ }
- /* Required: Send Checksum Offload. */
- RtlInitUnicodeString( &keyword, L"RecvChksum" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
- {
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("Recv Checksum Offload parameter missing.\n") );
- return status;
- }
- p_adapter->params.recv_chksum_offload =
(p_param->ParameterData.IntegerData != 0);
+ else
+ {
+ value = pRegEntry[i].Default;
+ status = NDIS_STATUS_SUCCESS;
+ if (pRegEntry[i].bRequired)
+ {
+ ipoib_create_log(p_adapter->h_adapter, i,
EVENT_IPOIB_WRONG_PARAMETER_ERR);
+ IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_INIT, ("Read
configuration.Registry %S value not found, setting default value=
0x%x\n", pRegEntry[i].RegName.Buffer, value));
+ }
+ else
+ {
+ ipoib_create_log(p_adapter->h_adapter, i,
EVENT_IPOIB_WRONG_PARAMETER_INFO);
+ IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("Read
configuration. Registry %S value not found, Value= 0x%x\n",
pRegEntry[i].RegName.Buffer, value));
+ }
- /* Required: SA query timeout, in milliseconds. */
- RtlInitUnicodeString( &keyword, L"SaTimeout" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
- {
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("SA query timeout parameter missing.\n") );
- return status;
- }
- p_adapter->params.sa_timeout =
p_param->ParameterData.IntegerData;
+ }
+ //
+ // Store the value in the adapter structure.
+ //
+ switch(pRegEntry[i].FieldSize)
+ {
+ case 1:
+ *((PUCHAR) structPointer) = (UCHAR) value;
+ break;
- /* Required: SA query retry count. */
- RtlInitUnicodeString( &keyword, L"SaRetries" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
- {
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("SA query retry count parameter missing.\n") );
- return status;
+ case 2:
+ *((PUSHORT) structPointer) = (USHORT) value;
+ break;
+
+ case 4:
+ *((PULONG) structPointer) = (ULONG) value;
+ break;
+
+ default:
+ IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR, ("Bogus
field size %d\n", pRegEntry[i].FieldSize));
+ break;
+ }
}
- p_adapter->params.sa_retry_cnt =
p_param->ParameterData.IntegerData;
- /* Required: Receive pool to queue depth ratio. */
- RtlInitUnicodeString( &keyword, L"RecvRatio" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
- {
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("Receive pool to queue depth ratio parameter missing.\n")
);
- return status;
+ // Send queue depth needs to be a power of two
+ //static const INT sq_depth_step = 128;
+
+ if (p_adapter->params.sq_depth % sq_depth_step) {
+ static const c_sq_ind = 2;
+ p_adapter->params.sq_depth = sq_depth_step *(
+ p_adapter->params.sq_depth / sq_depth_step + !!(
(p_adapter->params.sq_depth % sq_depth_step) > (sq_depth_step/2) ));
+ ipoib_create_log(p_adapter->h_adapter, c_sq_ind,
EVENT_IPOIB_WRONG_PARAMETER_WRN);
+ IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("SQ DEPTH
value was rounded to the closest acceptable value of 0x%x\n",
p_adapter->params.sq_depth ));
+
}
- p_adapter->params.recv_pool_ratio =
p_param->ParameterData.IntegerData;
- /* required: MTU size. */
- RtlInitUnicodeString( &keyword, L"PayloadMtu" );
- NdisReadConfiguration(
- &status, &p_param, h_config, &keyword, NdisParameterInteger
);
- if( status != NDIS_STATUS_SUCCESS )
- {
- IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
- ("PayloadMtu parameter missing. Use the default.\n") );
- return status;
- }
- p_adapter->params.payload_mtu =
p_param->ParameterData.IntegerData;
+
+ // Adjusting the low watermark parameter
+ p_adapter->params.rq_low_watermark =
+ p_adapter->params.rq_depth /
p_adapter->params.rq_low_watermark;
+
p_adapter->params.xfer_block_size = (sizeof(eth_hdr_t) +
p_adapter->params.payload_mtu);
NdisReadNetworkAddress( &status, &mac, &len, h_config );
@@ -2344,8 +2393,14 @@
/* Must cast here because the service name is an array of
unsigned chars but
* strcpy want a pointer to a signed char */
- strcpy( (char *)ib_service.svc_rec.service_name, ATS_NAME );
-
+ if ( StringCchCopy( (char *)ib_service.svc_rec.service_name,
+ sizeof(ib_service.svc_rec.service_name) / sizeof(char),
ATS_NAME ) != S_OK) {
+ ASSERT(FALSE);
+ IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR,IPOIB_DBG_ERROR,
+ ("Problem copying ATS name: exiting\n"));
+ return;
+ }
+
/* IP Address in question will be put in below */
ib_service.port_guid = p_adapter->guids.port_guid;
ib_service.timeout_ms = p_adapter->params.sa_timeout;
Index: ulp/ipoib/kernel/ipoib_log.mc
===================================================================
--- ulp/ipoib/kernel/ipoib_log.mc (revision 2629)
+++ ulp/ipoib/kernel/ipoib_log.mc (revision 2630)
@@ -283,3 +283,27 @@
Language=English
%2: The local port rate is too slow for the existing broadcast
MC group.
.
+
+MessageId=0x0058
+Facility=IPoIB
+Severity=Error
+SymbolicName=EVENT_IPOIB_WRONG_PARAMETER_ERR
+Language=English
+%2: Incorrect value or non-existing registry for the required
IPoIB parameter %3, overriding it by default value: %4
+.
+
+MessageId=0x0059
+Facility=IPoIB
+Severity=Warning
+SymbolicName=EVENT_IPOIB_WRONG_PARAMETER_WRN
+Language=English
+%2: Incorrect value or non-existing registry entry for the
required IPoIB parameter %3, overriding it by default value: %4
+.
+
+MessageId=0x005A
+Facility=IPoIB
+Severity=Informational
+SymbolicName=EVENT_IPOIB_WRONG_PARAMETER_INFO
+Language=English
+%2: Incorrect value or non-existing registry for the optional
IPoIB parameter %3, overriding it by default value: %4
+.
Index: ulp/ipoib/kernel/ipoib_driver.h
===================================================================
--- ulp/ipoib/kernel/ipoib_driver.h (revision 2629)
+++ ulp/ipoib/kernel/ipoib_driver.h (revision 2630)
@@ -126,4 +126,13 @@
ipoib_resume_oids(
IN ipoib_adapter_t* const p_adapter );
+#define IPOIB_OFFSET(field)
((UINT)FIELD_OFFSET(ipoib_params_t,field))
+#define IPOIB_SIZE(field)
sizeof(((ipoib_params_t*)0)->field)
+#define IPOIB_INIT_NDIS_STRING(str) \
+ (str)->Length = 0; \
+ (str)->MaximumLength = 0; \
+ (str)->Buffer = NULL;
+
+
+
#endif /* _IPOIB_DRIVER_H_ */
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20080626/bb6dbcf2/attachment.html>
More information about the ofw
mailing list