Always use safe call to read for regular files and block devices on unix if the RTS is multi-threaded, regardless of O_NONBLOCK #166
Comments
|
The actual merge request is located here. It's a one-line change that adds a check to see if the file is a |
|
I am a little concerned about the interplay with the rewrite of the I/O manager. @bgamari could you explain what the intent and effect that will have on the |
|
@mixphix, I suspect that you mean the |
|
Yes, thanks. That's reass"uring". :) |
|
As someone who uses Haskell to access slow network drives, I'm utterly in favor. |
|
I'd like to get input from @Rufflewind and @Mistuke Note that other packages such as file-io/unix etc may need patching. Otherwise we create subtle differences across the ecosystem. E.g.: https://github.com/hasufell/file-io/blob/master/System/File/Posix.hs |
|
Could someone explain the problem with |
|
@treeowl it is extensively explained in the ticket's thread on Gitlab. |
|
I don't think turning off O_NONBLOCK is the proper solution. I think the real bug is on this line: | isNonBlocking fd = unsafe_readIt's probably a bad idea to assume that I think the proper fix should be to remove this hack from On a separate matter, it might be worth also diving into the |
Yeah architecturally, I few this as an implementation detail of the I/O manager, which may be in This makes it a funny thing to consider here, procedurally. It is possible there could be some "pre IO manager" IO primitives that are more directly exposing what the OS provides, unvarnished. These would be very dangerous to use in most code, but also great building blocks for writing the IO manager in the first place. That strikes me as a good architecture, and then those "pre IO manager" IO primitives would be squarely in CLC remit since they are regular public library functions, with little implementation wiggle room left by their "public" behavior. |
@Kleidukos @arybczak @nh2 could you please provide a self-contained summary, which can serve as a proposal? Precisely because GitLab tickets are extremely extensive. It's not like an average Haskell developer has any idea about |
Bodigrim
added
the
awaits-proposal
|
Since no one got time to prepare a summary of the issue, here is mine. Haskell FFI allows to specify the safety level. According to the Haskell Report 2010, in a single-threaded environment:
This is however quite benign: in the most common case, when FFI connects Haskell to an independent low-level C library, there are no callbacks. The situation gets worse in a multi-threaded environment: if one thread executes an
A common pattern is to define both isValidUtf8 :: ShortByteString -> Bool
isValidUtf8 sbs@(unSBS -> ba#) = accursedUnutterablePerformIO $ do
let n = length sbs
-- Use a safe FFI call for large inputs to avoid GC synchronization pauses
-- in multithreaded contexts.
-- This specific limit was chosen based on results of a simple benchmark, see:
-- https://github.com/haskell/bytestring/issues/451#issuecomment-991879338
-- When changing this function, also consider changing the related function:
-- Data.ByteString.isValidUtf8
i <- if n < 1000000 || not (isPinned ba#)
then cIsValidUtf8 ba# (fromIntegral n)
else cIsValidUtf8Safe ba# (fromIntegral n)
IO (\s -> (# touch# ba# s, () #))
return $ i /= 0
-- We import bytestring_is_valid_utf8 both unsafe and safe. For small inputs
-- we can use the unsafe version to get a bit more performance, but for large
-- inputs the safe version should be used to avoid GC synchronization pauses
-- in multithreaded contexts.
foreign import ccall unsafe "bytestring_is_valid_utf8" cIsValidUtf8
:: ByteArray# -> CSize -> IO CInt
foreign import ccall safe "bytestring_is_valid_utf8" cIsValidUtf8Safe
:: ByteArray# -> CSize -> IO CIntGetting closer to the topic, foreign import capi unsafe "HsBase.h read"
c_read :: CInt -> Ptr Word8 -> CSize -> IO CSsize
foreign import capi safe "HsBase.h read"
c_safe_read :: CInt -> Ptr Word8 -> CSize -> IO CSsizeThey are used further in readRawBufferPtr :: String -> FD -> Ptr Word8 -> Int -> CSize -> IO Int
readRawBufferPtr loc !fd !buf !off !len
#if defined(javascript_HOST_ARCH)
= fmap fromIntegral . uninterruptibleMask_ $
throwErrnoIfMinus1 loc (c_read (fdFD fd) (buf `plusPtr` off) len)
#else
| isNonBlocking fd = unsafe_read -- unsafe is ok, it can't block
| otherwise = do r <- throwErrnoIfMinus1 loc
(unsafe_fdReady (fdFD fd) 0 0 0)
if r /= 0
then read
else do threadWaitRead (fromIntegral (fdFD fd)); read
where
do_read call = fromIntegral `fmap`
throwErrnoIfMinus1RetryMayBlock loc call
(threadWaitRead (fromIntegral (fdFD fd)))
read = if threaded then safe_read else unsafe_read
unsafe_read = do_read (c_read (fdFD fd) (buf `plusPtr` off) len)
safe_read = do_read (c_safe_read (fdFD fd) (buf `plusPtr` off) len)
#endifThe idea is that we use But if On the lowest level non-blocking mode is implemented by passing While regular files cannot block indefinitely as other types of file descriptors such as sockets and pipes can, they always block the caller for a brief amount of time: you cannot read a file from a floppy drive or network share without delay, and no amount of The proposed patch attempts to fix the issue at the level of - fdIsNonBlocking = fromEnum is_nonblock
+ fdIsNonBlocking = fromEnum (is_nonblock && fd_type /= RegularFile)That's not totally bulletproof: after all users can create An alternative approach would be to perform the check in Thus I think that the proposers made a reasonable choice, best possible without major refactoring. One nitpick is that block devices should also be excluded (see - fdIsNonBlocking = fromEnum is_nonblock
+ fdIsNonBlocking = fromEnum (is_nonblock && fd_type /= RegularFile && fd_type /= RawDevice)I would also appreciate improved documentation for @Kleidukos @arybczak @nh2 could you please indicate that you are still interested in the proposal? I'm somewhat worried that I had to prepare the long text above myself, I'm not a proposer after all. If there is no active champion to respond to the feedback within two weeks, we'll have little choice other than close as abandoned. |
|
@Bodigrim Yes, excellent summary! Some detail remarks:
Where "brief" can in practice, and today's real-world industry settings be
Hit any of those for multiple files concurrently, and the Haskell program freezes for a while. Thus motivating concretely why this proposal exists. |
|
@nh2 if you update the MR adding |
|
I put up a new, slightly extended MR https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11338. Besides some pieces of documentation, the only material change is one line: - fdIsNonBlocking = fromEnum is_nonblock
+ fdIsNonBlocking = fromEnum (is_nonblock && fd_type /= RegularFile && fd_type /= RawDevice)For anyone new to this discussion, the summary of the issue is available above, at #166 (comment). I'll change hats and trigger a vote shortly. |
|
Actually, given that this is a non-trivial piece of code and it would be unfortunate to spoil multiyear efforts by calling a vote too early, let me ask for non-binding opinions first. Dear CLC members, could you please read #166 (comment) and opine on https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11338? |
|
@Bodigrim thank you for the excellent summary! I currently have trouble finding a common case where this would break existing code and behaviour, and other than potentially reducing stalls, I can't find any. As such I'm in favour of this change. |
|
The performance regression is somewhat concerning, no? Is Marlow's comment (https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7986#note_502536) being considered in this proposal as well? Otherwise, I am in favor of this. |
We are essentially fixing a DDoS vulnerability (= a single slow file I/O can render a multithreaded program unresponsible), so a modest performance regression is acceptable. We'd better get it correct first, optimize constant factor later.
My view is that such optimization could be a subject of a future proposal. It's not straightforward to implement: at least one has to extend |
If we ever go with that approach, I would recommend adding a |
|
Any more non-binding opinions? @tomjaguarpaw @mixphix @parsonsmatt @hasufell |
|
I'm +1 The performance regression can be fixed later. I'm kind of baffled by the current behavior and wasn't aware of it. As a note: the title is confusing. This has technically nothing to do with passing O_NONBLOCK to Edit: as indicated in my MR comment I'd change the title to: Always use |
RTS configuration for this sounds like a good way to allow people to opt out of the changed behavior or tweak it? |
@hasufell You are right. That's my fault: I orignally wrote the commit message |
Kleidukos
changed the title
|
Dear CLC members, let's vote on https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11338/diffs to fix a DDoS vulnerability (= a single slow file I/O, e. g., network shared drive, can render an entire multithreaded program unresponsive). This patch is backwards-compatible and does not introduce new API. @hasufell @tomjaguarpaw @parsonsmatt @mixphix @velveteer @angerman +1 from me. |
|
+1, as it doesn't break existing code. I see the concern for potential change in runtime behavior. That does however rather lead me to believe we should have an additional exploit api to let the end user chose rather than rely on a heuristic. |
|
+1 |
1 similar comment
|
+1 |
|
+1 |
|
Thanks all, that’s enough votes to approve unconditionally. Indeed this area may benefit from a more invasive contribution, refactoring |
Bodigrim
added
approved
Hello CLC :)
From the original bug GHC uses O_NONBLOCK on regular files, which has no effect, and blocks the runtime
The thread is fairly detailed, and quite interesting to read entirely
Current patch lives at : https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7986
@arybczak has provided benchmarks:
with the following comment:
From a personal standpoint, I can attest that in industrial environments that make use of network-based storage like AWS, this is a very important fix (disclaimer: All the companies I've worked at in the recent years use such a type of storage for applicative servers).
It must be noted that this is happening in parallel of a rewrite of the I/O Manager with io_uring, so while this is a fix for a bug, it is not bound to be a permanent solution.
The text was updated successfully, but these errors were encountered: