Improve wlEglMemoryIsReadable behavior #38
Conversation
In 2eb4628 ("egl-wayland: check interface name in wlEglCheckInterfaceType", 2021-08-30, 1.1.8), the wlEglPointerIsDereferencable function was replaced with a new wlEglMemoryIsReadable function. The new wlEglMemoryIsReadable function relies on the write function returning EFAULT when passed a buffer that is "outside your accessible address space". However, there's nothing to prevent that write call from being interrupted and returning EINTR before it can complete validation of the input arguments. Add a loop to retry the call if it's interrupted with a signal. Additionally, there's no guarantee that a just-created pipe will always be able to immediately accept at least one byte without blocking in which case an EAGAIN error will be returned. Detect an EAGAIN error and treat that as success since the EAGAIN error indicates that the call can succeed if retried implying the arguments are otherwise valid. Signed-off-by: Kyle J. McKay <[email protected]>
In preparation for reusing the same attempt-to-write-on-a-dummy-pipe code, move the code that checks for EINTR and EAGAIN into its own separate static function. Signed-off-by: Kyle J. McKay <[email protected]>
When using the write function call to check for whether or not the buffer being written is "outside your accessible address space," it's important to consider any shortcuts the write function may be taking when validating the arguments. In particular, if the passed in length to write is zero it may not do any validation at all. To that end, treat a length of 0 as 1. Furthermore, since the destination is a pipe that can only ever accept a small number of bytes without blocking, there's no need for the write function call to validate the entire memory range of the buffer being passed to be written, merely the initial portion. Work around this optimization by attempting to write a byte from the very end of the memory range to the pipe. Be careful to avoid a PIPE_BUF size optimization by the write call by draining the single byte before attempting to write the entire range. (If a memory range of exactly PIPE_BUF bytes is passed, but there's only room in the pipe for PIPE_BUF-1 bytes because the write of a single byte from the end used up one byte, then the write call is expected to return EAGAIN in which case there's really no need for it to validate the buffer's memory address -- another optimization shortcut to avoid.) While this will not cause the entire memory range to be validated in the face of write function optimizations, it will force an extra check at the very end which provides a much more robust test than only checking the initial portion of the memory range. An alternative to provide even more checking would be to attempt to write at least one byte from every PAGE_SIZE block within the memory range being checked. However, that would definitely incur a performance penalty for large memory ranges and even that could still potentially miss unwritable holes in the middle. Adding a check of the very end of the memory range provides a speedy compromise that will provide a more accurate answer in the face of write function optimizations without incurring a performance penalty. Signed-off-by: Kyle J. McKay <[email protected]>
If the write function thinks the pointer and/or length arguments are invalid, it may return EINVAL instead of EFAULT. In the context of wlEglMemoryIsReadable that will do just as well as an indication that the memory is most likely unreadable. Therefore allow an EINVAL error without causing an assertion failure. Signed-off-by: Kyle J. McKay <[email protected]>
|
In general, out of curiosity, have you actually encountered any problems that prompted this MR? Or was it just based on inspection of the code? |
|
Also, I was aware when making this change that trying to check a block of memory larger than the pipe buffer would fail. I didn't anticipate ever needing to do that, though, and so decided to avoid the extra complexity. The assert was meant to inform the programmer that such usage wasn't supported. |
|
I saw this code doing this "creative" thing that normally ought to result in a SIGSEGV but due to a non-POSIX-specified behavior extension of the write call allows memory addressing space to be inspected using only standard POSIX API calls, and, function calls that are guaranteed to be async-signal safe at that. Of course I snarfed that code out into a separate test program and played with it. While I was only ever able to get an EFAULT out of linux I did convince a *BSD to give me an EINVAL with just the right size parameter (ridiculously large). However, without the added check of the final byte, the original code returns a "yes it's readable" result for pretty much any length as long as that first little bit is readable. It does not trigger the assertion in the original code -- I was only able to ever trigger the original assertion when I convinced a *BSD to give me an EINVAL. I still suspect that under very heavy resource contention that an EAGAIN might pop out on the initial write to the non-blocking pipe -- can't prove it's not possible without a kernel analysis on all the systems supported by the driver -- but I wasn't able to cause one. Not checking for EINTR is just bad since the write call is documented as being able to return that. The documentation for the write function very clearly states:
Does Since PIPE_BUF is only the size of one memory page on Linux and the old code handled one page, I was thinking the only reason you got rid of the old code was because you needed to check more than one page for readability (or perhaps try to be more portable). You even say that in the commit comment about the new function: "which can check whether an arbitrarily sized block of memory is readable." As the original code is written, it definitely cannot do that. It can only check whether up to a PIPE_BUF size chunk of memory is readable. Without the patches, it does not fail when trying to check a block of memory larger than PIPE_BUF, it succeeds thereby claiming it's all readable. FreeBSD contains sys/sys/syslimits.h: The NVIDIA driver downloads site offers downloads for "FreeBSD x86" and "FreeBSD x64". Is checking only 512 bytes sufficient? Because that's all the original code can guarantee it's checked, certainly not an "arbitrarily sized block of memory." And that only as long as a signal does not arrive at just the wrong time. |
The implementation of the recently added wlEglMemoryIsReadable function will not always check to see whether or not the entire range is readable thanks to some optimizations by the write function.
These patches make its check much more robust (although still not perfect) and handle edge cases such as EINTR, EAGAIN and EINVAL that the original implementation does not while adding only a very tiny (constant) amount of extra work.
With these changes it should almost always provide the correct answer, even on the *BSDs and without an assertion failure either.