Fix order and locking to prevent use-after-free in submit_send - #322
Open
aversecat wants to merge 1 commit into
Open
Fix order and locking to prevent use-after-free in submit_send#322aversecat wants to merge 1 commit into
aversecat wants to merge 1 commit into
Conversation
A panic was recorded in testing during lock-recover-invalidate:
kernel BUG at lib/list_debug.c:26:
The stack:
#8 __list_add_valid.cold
#9 submit_send+? at ffffffffc11c18d0 [scoutfs] (+0x320)
#10 scoutfs_net_submit_request_node at ffffffffc11c4b60 [scoutfs]
#11 scoutfs_server_lock_request at ffffffffc11f5ba9 [scoutfs]
#12 process_waiting_requests at ffffffffc11bfbb6 [scoutfs]
#13 scoutfs_net_proc_worker at ffffffffc11c1fac [scoutfs]
#14 process_one_work
The crash is because __list_add_valid does BUG() on the prev->next != next
check, the disassembly at frame #9 shows:
submit_send+772: 48 8b ... mov 0x108(%rbx),%r14 ; r14 = resend_queue.prev (prev arg)
submit_send+779: 4c 8d ... lea 0x100(%rbx),%r15 ; r15 = &conn->resend_queue (next/head arg)
submit_send+786: 4c 89 ef mov %r13,%rdi ; rdi = new = &msend->head
submit_send+789: 4c 89 fa mov %r15,%rdx ; rdx = next = &resend_queue
submit_send+792: 4c 89 f6 mov %r14,%rsi ; rsi = prev = resend_queue.prev
submit_send+795: e8 ... call __list_add_valid ; __list_add_valid(new, prev, next)
Recovering r13-15 we get:
new = 0xff43eefc49f779f0
prev = 0xff43eefc49f773f0
next = 0xff43eefc4a1f1100
but prev here points to a kmalloc-192 object that is marked freed
on the slab of that CPU, and the __rb_parent_color and next/prev
members of prev point to itselfs, so this msend had already been
freed by free_msend(). Separately, the conn itself (kmalloc-1k)
was freed and its slab object reused.
All other worker threads were in teardown: scoutfs_net_shutdown_worker()
or similar. So it appears that submit_send added to conn->resend_queue
of a connection that's being shut down.
Fix by unlinking the accepted list *before* queue splice/free, so
that submit_send can't find it by rid anymore. Second, lock the conn
before doing the splice/free, so a racing submit_send must stop. This
matches how submit_send() does inserts holding conn->lock.
Signed-off-by: Auke Kok <auke.kok@versity.com>
Contributor
Author
|
retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A panic was recorded in testing during lock-recover-invalidate:
The stack:
#8 __list_add_valid.cold
#9 submit_send+? at ffffffffc11c18d0 [scoutfs] (+0x320)
#10 scoutfs_net_submit_request_node at ffffffffc11c4b60 [scoutfs]
#11 scoutfs_server_lock_request at ffffffffc11f5ba9 [scoutfs]
#12 process_waiting_requests at ffffffffc11bfbb6 [scoutfs]
#13 scoutfs_net_proc_worker at ffffffffc11c1fac [scoutfs]
#14 process_one_work
The crash is because __list_add_valid does BUG() on the prev->next != next check, the disassembly at frame #9 shows:
submit_send+772: 48 8b ... mov 0x108(%rbx),%r14 ; r14 = resend_queue.prev (prev arg)
submit_send+779: 4c 8d ... lea 0x100(%rbx),%r15 ; r15 = &conn->resend_queue (next/head arg)
submit_send+786: 4c 89 ef mov %r13,%rdi ; rdi = new = &msend->head
submit_send+789: 4c 89 fa mov %r15,%rdx ; rdx = next = &resend_queue
submit_send+792: 4c 89 f6 mov %r14,%rsi ; rsi = prev = resend_queue.prev
submit_send+795: e8 ... call __list_add_valid ; __list_add_valid(new, prev, next)
Recovering r13-15 we get:
new = 0xff43eefc49f779f0
prev = 0xff43eefc49f773f0
next = 0xff43eefc4a1f1100
but prev here points to a kmalloc-192 object that is marked freed on the slab of that CPU, and the __rb_parent_color and next/prev members of prev point to itselfs, so this msend had already been freed by free_msend(). Separately, the conn itself (kmalloc-1k) was freed and its slab object reused.
All other worker threads were in teardown: scoutfs_net_shutdown_worker() or similar. So it appears that submit_send added to conn->resend_queue of a connection that's being shut down.
Fix by unlinking the accepted list before queue splice/free, so that submit_send can't find it by rid anymore. Second, lock the conn before doing the splice/free, so a racing submit_send must stop. This matches how submit_send() does inserts holding conn->lock.