Change Details

INVARIANTS kernels may trigger a KASSERT panic from sndbuf_acquire(), when fuzzing write(2) using stress2, because of a race in chn_write(). In the case of chn_write(), what sndbuf_acquire() does is extend the ready-to-read area of the buffer by a specified amount of bytes. The KASSERT in question makes sure the number of bytes we want to extend the ready area by, is less than or equal to the number of free bytes in the buffer. This makes sense, because we cannot extend the ready area to something larger than what is available (i.e., free) in the first place. What chn_write() currently does for every write is; calculate the appropriate write size, let's say X, unlock the channel, uiomove() X bytes to the channel's buffer, lock the channel, and call sndbuf_acquire() to extend the ready area by X bytes. The problem with this approach, however, is the following. Suppose an empty channel buffer with a length of 1024 bytes, and 2 threads, (A) and (B), where (B) is a higher-priority one. Suppose thread (A) wants to write 1024 bytes. It unlocks the channel and uiomove()s 1024 bytes to the channel buffer. At the same time, thread (B) picks up the lock, and because it is higher priority, it keeps dominating the lock for a few iterations. By the time thread (A) picks up the lock again, it tries to call sndbuf_acquire() with an size of 1024 bytes, which was calculated before it performed the uiomove(). In this case, there is a very high chance that the buffer will not be empty, that is, have a free area of 1024 bytes, as was the case when thread (A) started executing, and so the KASSERT will trigger a panic because the condition (bytes <= free) is not met. To fix this, call sndbuf_acquire() and update the remaining total write size before unlocking the channel. Do the same for chn_read() to avoid similar panics from sndbuf_dispose(). Reported by: pho Tested by: christos, pho Sponsored by: The FreeBSD Foundation MFC after: 1 week

INVARIANTS kernels may trigger a KASSERT panic from sndbuf_acquire(), when fuzzing write(2) using stress2, because of a race in chn_write(). In the case of chn_write(), what sndbuf_acquire() does is extend the ready-to-read area of the buffer by a specified amount of bytes. The KASSERT in question makes sure the number of bytes we want to extend the ready area by, is less than or equal to the number of free bytes in the buffer. This makes sense, because we cannot extend the ready area to something larger than what is available (i.e., free) in the first place. What chn_write() currently does for every write is; calculate the appropriate write size, let's say X, unlock the channel, uiomove() X bytes to the channel's buffer, lock the channel, and call sndbuf_acquire() to extend the ready area by X bytes. The problem with this approach, however, is the following. Suppose an empty channel buffer with a length of 1024 bytes, and 2 threads, (A) and (B), where (B) is a higher-priority one. Suppose thread (A) wants to write 1024 bytes. It unlocks the channel and uiomove()s 1024 bytes to the channel buffer. At the same time, thread (B) picks up the lock, and because it is higher priority, it keeps dominating the lock for a few iterations. By the time thread (A) picks up the lock again, it tries to call sndbuf_acquire() with an size of 1024 bytes, which was calculated before it performed the uiomove(). In this case, there is a very high chance that the buffer will not be empty, that is, have a free area of 1024 bytes, as was the case when thread (A) started executing, and so the KASSERT will trigger a panic because the condition (bytes <= free) is not met. Another scenario that can trigger a panic is the following: suppose a buffer with a size of 4 bytes, and two threads: (A) and (B). In the first iteration, thread (A) wants to write 2 bytes, while the buffer is empty, BUT the pointer (sndbuf_getfreeptr()) is at the end (i.e., buf[3]). In the first iteration of the loop, because of the way we calculate t, we'll end up writing only 1 byte, so after sz -= t, sz will be 1, and so we'll need one more iteration in the inner loop, to write the remaining 1 byte. Now we're at the end of the first loop, thread (A) unlocks the channel, it has written 1 byte, it needs to write 1 more, and the buffer is left with 3 empty slots. Now thread (B) picks up the lock, and it wants to write 3 (or more) bytes. Eventually it writes the 3 bytes, and it leaves the buffer with 0 free slots. By the time thread (A) picks up the lock again, and continues with the second iteration of the inner loop, it will try to write the last byte, but sndbuf_acquire() will panic because there is no free space anymore. To fix this, get rid of the inner loop and calculate the write size on each iteration. That means that in the scenarios explained above, we'll end up entering the chn_sleep() case. Modify it as well, so that we do not kill the channel if we need to sleep more. Do the same for chn_read() to avoid possible similar panics from sndbuf_dispose(). Reported by: pho Tested by: christos, pho Sponsored by: The FreeBSD Foundation MFC after: 1 week

INVARIANTS kernels may trigger a KASSERT panic from sndbuf_acquire(), when when fuzzing write(2) using stress2, because of a race in chn_write(). In the case of chn_write(), what sndbuf_acquire() does is extend the ready-to-read area of the buffer by a specified amount of bytes. The KASSERT in KASSERT in question makes sure the number of bytes we want to extend the ready area by, is less than or equal to the number of free bytes in the buffer.ready area by, This makes sense,is less than or equal to the number of free bytes in the buffer. This makes sense, because we cannot extend the ready area to something larger than what is something larger than what is available (i.e., free) in the first place. What chn_write() currently does for every write is; calculate the appropriate appropriate write size, let's say X, unlock the channel, uiomove() X bytes to the channel's bytes to the channel's buffer, lock the channel, and call sndbuf_acquire() to extend the ready area by X bytes. The problem with this approach, however, is the followingsndbuf_acquire() to extend the ready area by X bytes. Suppose anThe problem with empty channel buffer with a length of 1024 bytesthis approach, however, and 2 threadsis the following. Suppose an empty channel buffer with a length of 1024 bytes, (A) and (B),2 threads, (A) and (B), where (B) is a higher-priority one. Suppose thread (A) wants to write 1024 (A) wants to write 1024 bytes. It unlocks the channel and uiomove()s 1024 bytes to the channel buffer. At1024 bytes to the same time,channel buffer. thread (B) picks up the lockAt the same time, and because it is higherthread (B) picks up prioritythe lock, it keeps dominating the lock for a few iterations.and because it is higher priority, By the time threadit keeps dominating the (A) picks up the lock again,lock for a few iterations. it tries to call sndbuf_acquire() with anBy the time thread (A) picks up the lock size of 1024 bytes, which was calculated before it performed the uiomove().again, Init tries to call sndbuf_acquire() with an size of 1024 bytes, this case, there is a very high chance thatwhich was calculated before it performed the buffer will not be empty,uiomove(). thatIn this case, is, have a free area of 1024 bytesthere is a very high chance that the buffer will not be empty, as was the case when thread (A) startedthat is, executinghave a free area of 1024 bytes, and soas was the KASSERT will trigger a panic because the condition (bytescase when thread (A) started executing, and so the KASSERT will trigger a panic because the condition (bytes <= free) is not met. To fix this, call sndbuf_acquire() and updateAnother scenario that can trigger a panic is the remaining total write sizefollowing: suppose a before unlocking the channelbuffer with a size of 4 bytes, and two threads: (A) and (B). Do the same for chn_read() to avoid similarIn the panics from first iteration, thread (A) wants to write 2 bytes, while the buffer is empty, BUT the pointer (sndbuf_getfreeptr()) is at the end (i.e., buf[3]). In the first iteration of the loop, because of the way we calculate t, we'll end up writing only 1 byte, so after sz -= t, sz will be 1, and so we'll need one more iteration in the inner loop, to write the remaining 1 byte. Now we're at the end of the first loop, thread (A) unlocks the channel, it has written 1 byte, it needs to write 1 more, and the buffer is left with 3 empty slots. Now thread (B) picks up the lock, and it wants to write 3 (or more) bytes. Eventually it writes the 3 bytes, and it leaves the buffer with 0 free slots. By the time thread (A) picks up the lock again, and continues with the second iteration of the inner loop, it will try to write the last byte, but sndbuf_acquire() will panic because there is no free space anymore. To fix this, get rid of the inner loop and calculate the write size on each iteration. That means that in the scenarios explained above, we'll end up entering the chn_sleep() case. Modify it as well, so that we do not kill the channel if we need to sleep more. Do the same for chn_read() to avoid possible similar panics from sndbuf_dispose(). Reported by: pho Tested by: christos, pho Sponsored by: The FreeBSD Foundation MFC after: 1 week