In geeral, there is so little advantage to putting in a pointer in this
fashion that it is hardly worth the effort to do so.
The first and most fundaental error here is te idea that you can cast a
pointer to a 32-bit integer. This code will not port to Win64, and if you
do a 64-bit driver, you have to worry about whether it was called from a
32-bit or 64-bit app. So you should forget any mechanism that believes
anything about pointer sizes. You will save yourself infinite grief.
I have a Device_Ioctl call I use to write data to my hardware. The input
buffer is an array of various values such as an address, width, count, and
finally a pointer to the data buffer containing the values I want to write
to my hardware.
driverParam[0] = offset;
driverParam[1] = count;
driverParam[2] = width;
driverParam[3] = (UINT32) hostAddr; //pass pointer to buffer as a UINT32
***
This represents such a fundamental failure of design in 2013 that I cannot
fathom any excuse that could allow it to make sense. Lose the entire idea
that a pointer fits into 32 bits. That way madness lies.
****
status = DeviceIoControl
(
hDevice,
IOCTL_DEVICE_WRITE,
driverParam,
4*sizeof(UINT32),
***
Of course, the only sensible thing would be to write 3*sizeof(UINT32) +
sizeof(PVOID).
You are literally living in another century.
****
NULL,
0,
***
This is why it makes no sense. There are two perfectly fine parameters
that you should be using to point to the data, and you shoud use
OUT_DIRECT. In a giant act of stupidity, the output was called input, and
the input was called output, which is true unless it is input. The only
intelligent way to think of DeviceIoControl is that it has two buffers:
The “parameters” parameter is always called in buffered mode, and the
“data” parameter is in one of buffered mode (in which case, it can only be
written by the driver), or is direct-mode souce of data (OUT_DIRECT) or
direct-mode sink of data (IN_DIRECT). So you are inventing a
mind-boggling complex solution to a truly trivial problem.
Lose the entire concept of passing a user-level address. You simply don’t
want to go there.
***
&bytesReturned,
&ol
);
The driver will retrieve the input buffer via
WdfRequestRetrieveInputBuffer and
write to my hardware with:
WRITE_REGISTER_BUFFER_ULONG((PULONG) bytePointer, (PULONG)inputBuffer[4],
count);
//bytePointer is an assembled address from my offset value
***
This line is so completely nonsense that it is mind-blowing. You can’t
use a user-level address like this; the fact that it EVER worked is what
is surprising. Do NOT use Driver Verifier on this driver; it will reach
out of your screen and strangle you. Rewrite it to be a proper driver
before using Driver Verifier.
In addition, you have shown this line with absolutely NO context! You
have not
You do not have a device driver. You have an artifact of code that
guarantees that it will crash your system. As I said, what is amazing is
tat it ever worked!
There is actually no need to pass an offset and count in. Since you know
the offset, there is no reason you can’t compute this address in your
program; the count can be passed as te buffer size, so instead of NULL,0
you will pass te address you want, and the count, instead, and your
problems will go away. So your parameters should be
DeviceIoControl(handle, NULL, 0, hostaddr+offset, count,
&bytesTransferred, NULL)
Don’t worry about how to fix your driver. It is unfixable, and the “fix”
involves learning how to create a MDL, lock down the pages, etc., and even
then it will be wrong because it will lock down all the pages, so you will
have to build a partial MDL, and the actual fix is so trivial that it is
not worth the effort.
****
This has worked for years but recently I noticed on some systems I get a
crash and the kernel dump points to this instruction suggesting the
(PULONG)inputBuffer[4] value is invalid now:
DRIVER_IRQL_NOT_LESS_OR_EQUAL (d1)
****
NEWS FLASH! You ASKED for this crash. The code you showed can guarantee
this! If you had used the Driver Verifier when you wrote this, it would
have caught it. You obviously believe in miracles, because that is the
only mechanism that could prevent this.
****
An attempt was made to access a pageable (or completely invalid) address
at an
interrupt request level (IRQL) that is too high. This is usually
caused by drivers using improper addresses.
***
When we ask for the !analyze -v output, we don’t mean that you should omit
all the useful information and include the lines that most of us can
recite from memory. Like, what routine were you in, what IRQL were you
running at, what are hte registers, what is the stack dump, and all that
REALLY IMPORTANT stuff.
*****
I tried switching to METHOD_IN_DIRECT and pass this buffer in using the
outputBuffer and the crash disappears. I’m guessing that the buffered
method loses track of the buffer I point to and it may become pageable
memory causing this crash. I also tried forcing the buffer to be a local
value in my IOCTL driver call, ignoring the pointer I passed in, and the
crash vanishes too. I don’t know how else to prove the pointer is invalid
after the WdfRequestRetrieveInputBuffer call.
****
There is no concept of “losing track”; there is absolutely nothing you
have done that could cause te buffer to be paged in and locked down. It
didn’t “become” pageable; you did nothing whatsoever to make it non-paged!
I have no idea what you mean by “forcing the buffer to be a local” because
I know of no way to accomplish this.
You can safely assume that if you have not explicitly done something to
lock the pages, they are not going to be locked. That’s why IN/OUT_DIRECT
is used; it tells the I/O Manager that the pages must be paged in an
locked down. WdfRequestRetrieveInputBuffer obtains a pointer to those 4
UINT32. Period. Since no place along the way have you have done anything
to lock the pages of your buffer down, the pointer you get from WdfRRIB is
perfectly valid. Your choice to interpret some of those bits as a
pointer, beside being a deep and fundamental design error which makes
unsupportable assumptions like pointers being 32 bits, or that a user
address is valid in arbitrary kernel contexts, there is NO WAY the I/O
Manager is going to know that some of those bits are a pointer. That’s
now your responsibility. But the interface you have designed is
remarkably clumsy, and should be scrapped, rather than trying any other
“fix”.
****
The MSDN site says that small transfers should use the BUFFERED method
because DIRECT I/O will be slower so I’d like to stick with buffered.
****
The reason direct mode is slow is that it has to do all those things like
bring all the pages and make sure they’re locked down! If you really want
to use buffered mode, pass in a pointer as the “parameters” which has
everything you need. For example, the count is redundant because you can
pass that in to the DeviceIoControl. The pointer to that buffer can be
computed based on the offset. You didn’t show the “width” parameter being
used at all. Bottom line, you have to think VERY carefully about ever
passing a user address into the kernel as part of the bits you send. The
simplest predicate to apply is “If I’m putting a user address in the data
packet I’m sending, I’ve made a fundamental design error”
(Note: before I generate a ton of responses from the advanced driver
writers saying “That’s completely wrong, and bad advice” let me assure
you: newbies need simple design rules to build successful drivers.
Putting up a sign that says “land mines” in front of a field of land mines
saves the experience described here, which is stepping on a land mine.
Those of us with serious driver experience either know where the mines are
buried, or we know how to use mine detectors. Newbies say “mines?”)
The fact that you didn’t understand what went wrong means you have walked
into a mine field and are now looking at your missing leg and saying “What
happened? My GPS said this was the shortest route!”
The MSDN was correct, but you completely misunderstood what it was saying.
*****
I’m
thinking I could pass in the entire buffer rather than a pointer but then
I’d have to copy the entire buffer in my application which would cause
performance problems too.
****
Several problems here:
(a) why do you think you need to copy the entire buffer?
(b) why do you think a copy on a modern machine is slow?
(c) how big is your buffer? On the one hand, you say you want to used
buffered mode because you have small amounts of data. Then you say that
copying those few bytes will be a performance problem? [hint: what do you
think the kernel does when you use buffered I/O? Oh, right, it COPIES
THE DATA]
(d) if you pass in the width (whatever that is) and a direct reference to
the data, where is the copy being done? I see no copy here.
***
The parameters have been set for years so I
can’t simply build the buffer with data and parameters without a copy.
***
And you think this is a problem?
Seriously, this is not a device driver you have described; it is a land
mine. Nothing short of a total rewrite is going to save it. Or, abandon
the concept of pointer, and offset, and count, and write a simple driver
that is actually correct.
You either have to change the interface or figure out all the things you
HAVE to fix to make it work. Changing the interface and the calls is
ultimately simpler. However, do NOT reuse the IOCTL code if you change
the interface,
***
What is the proper way to pass in a data buffer for the driver? Does it
appear that I’m making valid assumptions?
***
(a) pointers are 32 bits
(b) it is valid to touched unlocked pages in a driver
(c) there is a psychic component in the I/O Manager that knows that some
of the bits are a pointer and will lock that data down.
Any one of these invalid assumptions dooms your driver.
****
Is there some way to pass in a
pointer and not lose the data it points to?
****
Absolutely! Use the “data” parameter and direct mode. Or, if you like
walking around in minefields while poking with a stick (so you don’t
actually step on a land mine), then you can simulate everything direct
mode does, getting the same performance cost direct mode has, but with the
added advantage of costing weeks of effort.
****
I can’t seem to use the
output buffer for the input data wen using METHOD_BUFFERED. At least I
tried as I did with METHOD_IN_DIRECT and it didn’t work for BUFFERED
transfers.
****
No surprise here; if you use the “data” parameter, you can only write to
it. That’s how the call is defined to work in buffered mode.
I suspect the root cause of this horrid code is a completely failed
understanding of “performance”. You have pre-optimized a solution based
on rumor, rather than factual data. It is hard to offer specific advice
because
(a) you have given no numbers on the quantity of bytes you send down
(b) you have given no numbers on how many times per second you need to
make this call
(c) you have no measurements to tell you anything about actual performance
(d) the code fragment you show has no context
(e) you have not indicated if this device needs interrupts and hence also
needs a DPC
(f) you omitted any meaningful data from !analyze -v
Provide relevant information and the readers here might be able to suggest
a simpler and correct approach.
I say, scrap it and start over. Others may not be so negative.
Someday, I should write an essay called “how to define a driver
interface”, which used to be a two- hour lecture when I taught courses on
this.
joe
*****
NTDEV is sponsored by OSR
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