NDIS - cyclical call UnbindAdapterHandler

NTDEV
1

I write the protocol driver, NDIS 5.0. I test it on Win 7 x86. At unloding of the driver after call NdisDeregisterProtocol there is cyclical call UnbindAdapterHandler.

The source code of handlers:

VOID NdisBindAdapter(
PNDIS_STATUS Status,
NDIS_HANDLE BindContext,
PNDIS_STRING DeviceName,
PVOID SystemSpecific1,
PVOID SystemSpecific2
)
{
NDIS_STATUS nsts = STATUS_SUCCESS, OpenErrorStatus = NDIS_STATUS_SUCCESS;
NDIS_MEDIUM Medium = {NdisMedium802_3};
UINT MediumIdx = 0;
PADAPTERDATA adpdata;

DbgPrint(“NdisBindAdapter –>\n”);

if (devext.AdapterCnt>=1)
{
*Status = NDIS_STATUS_OPEN_FAILED;
return;
}

adpdata = ExAllocatePoolWithTag(NonPagedPool, sizeof(ADAPTERDATA), ‘ADRA’);
if (adpdata == NULL)
{
*Status = NDIS_STATUS_OPEN_FAILED;
return;
}
RtlZeroMemory(adpdata, sizeof(ADAPTERDATA));

adpdata->AdapterName.Length = 0;
adpdata->AdapterName.MaximumLength = (USHORT)(DeviceName->Length + sizeof(UNICODE_NULL));
adpdata->AdapterName.Buffer = ExAllocatePoolWithTag(PagedPool, adpdata->AdapterName.MaximumLength, ‘ENRA’);
if (adpdata->AdapterName.Buffer == NULL)
{
*Status = NDIS_STATUS_OPEN_FAILED;
return;
}
RtlZeroMemory(adpdata->AdapterName.Buffer, adpdata->AdapterName.MaximumLength);
RtlCopyUnicodeString(&adpdata->AdapterName, DeviceName);

NdisInitializeEvent(&adpdata->EvtOpenWait);
NdisInitializeEvent(&adpdata->EvtReqWait);
NdisInitializeEvent(&adpdata->EvtCloseWait);

NdisResetEvent(&adpdata->EvtOpenWait);
NdisOpenAdapter(Status, &OpenErrorStatus, &(adpdata->hAdapter), &MediumIdx, &Medium[0], 1, devext.hProto,
(NDIS_HANDLE)adpdata, &(adpdata->AdapterName), 0, NULL);
DbgPrint(“Status = 0x%08X, OpenErrorStatus = 0x%08X\n”, *Status, OpenErrorStatus);
if (NT_SUCCESS(*Status))
{
if (*Status == NDIS_STATUS_SUCCESS)
{
DbgPrint(“MediumIdx = %u\n”, MediumIdx);
NdisOpenAdapterComplete(adpdata, NDIS_STATUS_SUCCESS, NDIS_STATUS_SUCCESS);
}
else
if (*Status == NDIS_STATUS_PENDING)
{
*Status = NDIS_STATUS_SUCCESS;
DbgPrint(“NdisOpenAdapter pending…\n”);
NdisWaitEvent(&adpdata->EvtOpenWait, 0);
}
}
else
{
DbgPrint(“NdisOpenAdapter FAILED! Status = 0x%08X\n”, *Status);
*Status = STATUS_UNSUCCESSFUL;
return;
}

devext.AdapterCnt++;

DbgPrint(“NdisBindAdapter <–\n”);
}

VOID NdisUnbindAdapter(PNDIS_STATUS Status, NDIS_HANDLE ProtocolBindingContext, NDIS_HANDLE UnbindContext)
{
PADAPTERDATA adpdata;

DbgPrint(“–> NdisUnbindAdapter\n”);

adpdata = (PADAPTERDATA)ProtocolBindingContext;

if (adpdata->hAdapter != INVALID_HANDLE_VALUE)
{
NdisResetEvent(&adpdata->EvtCloseWait);
NdisCloseAdapter(Status, &adpdata->hAdapter);
if (NT_SUCCESS(*Status))
{
if (*Status == NDIS_STATUS_PENDING)
{
DbgPrint(“NdisCloseAdapter pending…\n”);
NdisWaitEvent(&adpdata->EvtCloseWait, 0);
*Status = NDIS_STATUS_SUCCESS;
}
adpdata->hAdapter = INVALID_HANDLE_VALUE;
}
else
{
*Status = STATUS_UNSUCCESSFUL;
DbgPrint(“NdisCloseAdapter FAILED!\n”);
}
}

DbgPrint(“<– NdisUnbindAdapter\n”);
}

2

> I write the protocol driver, NDIS 5.0. I test it on Win 7 x86. At unloding

of the driver after call NdisDeregisterProtocol there is cyclical call
UnbindAdapterHandler.

I presume you mean there are multiple calls when you expect only one. You
need to log the addresses of adpdata (use %p) when it is created, and the
address that you are passed in.

The source code of handlers:

VOID NdisBindAdapter(
PNDIS_STATUS Status,
NDIS_HANDLE BindContext,
PNDIS_STRING DeviceName,
PVOID SystemSpecific1,
PVOID SystemSpecific2
)
{
NDIS_STATUS nsts = STATUS_SUCCESS, OpenErrorStatus =
NDIS_STATUS_SUCCESS;

One variable, one line, never, ever use comma in declaration lists.

NDIS_MEDIUM Medium = {NdisMedium802_3};
UINT MediumIdx = 0;
PADAPTERDATA adpdata;

DbgPrint(“NdisBindAdapter –>\n”);

if (devext.AdapterCnt>=1)

Put spaces around operators. The compiler may not care, but human readers do

{
*Status = NDIS_STATUS_OPEN_FAILED;
return;
}

adpdata = ExAllocatePoolWithTag(NonPagedPool, sizeof(ADAPTERDATA),
‘ADRA’);
if (adpdata == NULL)
{
*Status = NDIS_STATUS_OPEN_FAILED;
return;
}
RtlZeroMemory(adpdata, sizeof(ADAPTERDATA));

adpdata->AdapterName.Length = 0;

So why do you need to redundantly set this to zero? Do you think that
setting it to zero more than once makes it a better zero?

adpdata->AdapterName.MaximumLength = (USHORT)(DeviceName->Length +
sizeof(UNICODE_NULL));
adpdata->AdapterName.Buffer = ExAllocatePoolWithTag(PagedPool,
adpdata->AdapterName.MaximumLength, ‘ENRA’);
if (adpdata->AdapterName.Buffer == NULL)
{
*Status = NDIS_STATUS_OPEN_FAILED;

Classic memory-leak error here: who deallocates ‘adpdata’? Note that
Driver Verifier will smack you down, hard, for leaving memory allocated.

return;
}
RtlZeroMemory(adpdata->AdapterName.Buffer,
adpdata->AdapterName.MaximumLength);
RtlCopyUnicodeString(&adpdata->AdapterName, DeviceName);

NdisInitializeEvent(&adpdata->EvtOpenWait);
NdisInitializeEvent(&adpdata->EvtReqWait);
NdisInitializeEvent(&adpdata->EvtCloseWait);

NdisResetEvent(&adpdata->EvtOpenWait);
NdisOpenAdapter(Status, &OpenErrorStatus, &(adpdata->hAdapter),
&MediumIdx, &Medium[0], 1, devext.hProto,
(NDIS_HANDLE)adpdata, &(adpdata->AdapterName), 0,
NULL);

I hope this call makes a copy of the adpdata, otherwise, we lose it when
this function exits

DbgPrint(“Status = 0x%08X, OpenErrorStatus = 0x%08X\n”, *Status,
OpenErrorStatus);
if (NT_SUCCESS(*Status))
{
if (*Status == NDIS_STATUS_SUCCESS)
{
DbgPrint(“MediumIdx = %u\n”, MediumIdx);
NdisOpenAdapterComplete(adpdata, NDIS_STATUS_SUCCESS,
NDIS_STATUS_SUCCESS);
}
else
if (*Status == NDIS_STATUS_PENDING)
{
*Status = NDIS_STATUS_SUCCESS;
DbgPrint(“NdisOpenAdapter pending…\n”);
NdisWaitEvent(&adpdata->EvtOpenWait, 0);
}
}
else
{
DbgPrint(“NdisOpenAdapter FAILED! Status = 0x%08X\n”,
*Status);
*Status = STATUS_UNSUCCESSFUL;

Ditto memory leak but now you leak two blocks of data.

Driver Verifier does not have a notion of “forgiveness”.

return;
}

devext.AdapterCnt++;

DbgPrint(“NdisBindAdapter <–\n”);

Use something like

DbgPrint(“NdisBidAdapter <- %p\n”, adpdata):

and for all other return statements do
DbgPrint(“NdisBindAdaper <- (failed)\n”);

I prefer to mark entry and exit by using “->” and “<-” at the front of the
line, except that EVERY debug output is done like this:
DbgPrint(“%s -> NdisBindAdapter…”, MYDRIVERNAME, …);
where “…” means “Whatever else you want”. Note also that there are now
preprocessor symbols that give the current function name; look for
FUNCTION. What I do for analysis is write some little scripts in my
favorite language. The I take the debug output spew and save it as a
file. I run one script that extracts only the lines with the MYDRIVERNAME
string (#define MYDRIVERNAME “JOESNDIS”) and writes out only those lines;
I use ordinary stdout piping to put this in a file. Then I use another
script to interpret the -> and <- symbols to “prettyprint” it with
indentations. Combine this with printouts of selected parameters and
return values, and many bugs just jump off the page at you. After a brief
delay to kick myself over making such an obvious error, I go in and fix
it. These tools are easy to write, and if you make sure all your debug
printout has a consistent pattern, you can get a lot of leverage.

But watch out for those memory leaks!
joe

}

VOID NdisUnbindAdapter(PNDIS_STATUS Status, NDIS_HANDLE
ProtocolBindingContext, NDIS_HANDLE UnbindContext)
{
PADAPTERDATA adpdata;

DbgPrint(“–> NdisUnbindAdapter\n”);

adpdata = (PADAPTERDATA)ProtocolBindingContext;

if (adpdata->hAdapter != INVALID_HANDLE_VALUE)
{
NdisResetEvent(&adpdata->EvtCloseWait);
NdisCloseAdapter(Status, &adpdata->hAdapter);
if (NT_SUCCESS(*Status))
{
if (*Status == NDIS_STATUS_PENDING)
{
DbgPrint(“NdisCloseAdapter pending…\n”);
NdisWaitEvent(&adpdata->EvtCloseWait, 0);
*Status = NDIS_STATUS_SUCCESS;
}
adpdata->hAdapter = INVALID_HANDLE_VALUE;
}
else
{
*Status = STATUS_UNSUCCESSFUL;
DbgPrint(“NdisCloseAdapter FAILED!\n”);
}
}

DbgPrint(“<– NdisUnbindAdapter\n”);

When is adpdata and its contents freed?

}

NTDEV is sponsored by OSR

Visit the list at: http://www.osronline.com/showlists.cfm?list=ntdev

OSR is HIRING!! See http://www.osr.com/careers

For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars

To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer

3

?ddress of adpdata in NdisBindAdapter and address of adpdata in NdisUnbindAdapter is equal.

4

> ?ddress of adpdata in NdisBindAdapter and address of adpdata in

NdisUnbindAdapter is equal.

In other words, you are saying that the unbind operation is called more
than once for a given adapter?

The word “cyclical” has many different interpretations, including
recursive calls. And it was not clear that subsequent calls had specified
the same adapter. Now you have clarified it, and it’s time for the
network gurus to jump in.
joe

NTDEV is sponsored by OSR

Visit the list at: http://www.osronline.com/showlists.cfm?list=ntdev

OSR is HIRING!! See http://www.osr.com/careers

For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars

To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer

5

Windows 7 will call ProtocolUnbindAdapter in a loop until the protocol calls NdisCloseAdapter on the binding handle. Although it appears you’re calling NdisCloseAdapter, you’re not calling it *on the binding handle*. You’re passing the *address of the handle* instead:

NdisCloseAdapter(Status, &adpdata->hAdapter);

Remove the extra ampersand. Make sure that NdisCloseAdapter actually writes NDIS_STATUS_SUCCESS or NDIS_STATUS_PENDING into the Status field.

In addition to Joe’s code review (do not ignore his remarks about the memory leaks; there are many), I’ll add another couple networking-specific remarks:

else if (*Status == NDIS_STATUS_PENDING)
{
*Status = NDIS_STATUS_SUCCESS;
NdisWaitEvent(&adpdata->EvtOpenWait, 0);

You don’t know that the operation completed successfully. The bind operation may have completed asynchronously with a failure status. You need to retrieve the 2nd parameter from your ProtocolBindAdapterComplete handler to know whether the bind operation succeeded. Failure to ignore this important fact will result in bugchecks on accessing a bad handle when binding fails.

It’s interesting to me that the failure branch for NdisOpenAdapter doesn’t call your NdisOpenAdapterComplete routine.

INVALID_HANDLE_VALUE is a usermode concept. It is never used in NDIS or WDM. Best to use NULL or 0 to represent invalid handles instead, because you don’t know if INVALID_HANDLE_VALUE will actually be a valid handle value.

Your driver needs some work before it’s ready for production use. Take a look at this sample driver to get an idea of how an NDIS protocol can work: http://code.msdn.microsoft.com/NDISProt-9a3cab24