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. Incidentally this scenario is exactly what happens consistently during testing, like below. The printfs were added for testing purposes. "BEFORE" means "before uiomove() and unlocking the channel", and "AFTER" means "after re-locking the channel". "count" is the number of bytes we uiomove() and call sndbuf_acquire() with, and "free" is the number of free bytes in the channel's buffer: chn_write(): [BEFORE] dsp0.virtual_play.1: td=100208, count=1024, free=1024 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=1024 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=1024 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=772 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=772 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=520 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=520 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=268 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=268 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=16, free=16 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=16, free=16 chn_write(): [AFTER] dsp0.virtual_play.1: td=100208, count=1024, free=0 Here we see that thread 100208 writes 1024 bytes, unlocks, and then thread 100210 takes over for 5 loops. By the time thread 100208 locks the channel back, thread 100210 has left the buffer with a free size of 0, and so thread 100208 tries to extend the ready area by 1024, which will cause the KASSERT to trigger the panic, since the condition (bytes <= free -> 1024 <= 0) 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. 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. Incidentally this scenario is exactly what happens consistently during testing, like below. The printfs were added for testing purposes. "BEFORE" means "before uiomove() and unlocking the channel", and "AFTER" means "after re-locking the channel". "count" is the number of bytes we uiomove() and call sndbuf_acquire() with, and "free" is the number of free bytes in the channel's buffer: chn_write(): [BEFORE] dsp0.virtual_play.1: td=100208, count=1024, free=1024 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=1024 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=1024 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=772 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=772 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=520 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=520 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=252, free=268 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=252, free=268 chn_write(): [BEFORE] dsp0.virtual_play.1: td=100210, count=16, free=16 chn_write(): [AFTER] dsp0.virtual_play.1: td=100210, count=16, free=16 chn_write(): [AFTER] dsp0.virtual_play.1: td=100208, count=1024, free=0 Here we see that thread 100208 writes 1024 bytes, unlocks, and then thread 100210 takes over for 5 loops. By the time thread 100208 locks the channel back, thread 100210 has left the buffer with a free size of 0, and so thread 100208 tries to extend the ready area by 1024, which will cause the KASSERT to trigger the panic, since the condition (bytes <= free -> 1024 <= 0) 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