core: keep the ring waist in bounds when a ring grows - #136
Open
munzzyy wants to merge 1 commit into
Open
Conversation
evpl_iovec_ring_resize() and evpl_dgram_ring_resize() move head and tail
into the new array. The waist they recompute as
((waist + size) - tail) - size
The added and the subtracted size cancel. That leaves waist - tail with
no reduction at all, so once the ring has wrapped far enough that waist
sits below tail it goes negative. evpl_iovec_ring_waist() then indexes
the array with it. The read lands in front of the allocation
evpl_valloc() just returned.
Mask it instead, the way evpl_iovec_ring_elements() already does for the
same remap. mask still holds the pre-resize size - 1 at that point. It
is reassigned two lines further down, so the reduction is modulo the old
size. That is the index space the copy loop above remapped out of.
Only the rdmacm flush paths advance a waist, so reaching this through a
protocol takes a device. The test drives the ring directly: fill, drain,
refill so head wraps behind tail, put the waist below tail, then add one
more entry to force the grow.
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.
evpl_iovec_ring_resize()remaps head and tail into the freshly allocated array,then does this to the waist:
The
+ ring->sizeand the- ring->sizecancel, so it is plainwaist - tailwith no reduction. The sibling helper does the same remap correctly:
Resize only runs when the ring is full, so
headistail - 1and the waist sitssomewhere between them. Where it has wrapped past zero and
tailhas not, its rawindex is below
tailand the subtraction goes negative.evpl_iovec_ring_waist()indexes the array with whatever came out:so the caller gets a pointer in front of the block
evpl_valloc()just returned.evpl_rdmacm_flush_rdma_reads()andevpl_rdmacm_flush_datagram()both call itand read through the result.
src/core/dgram_ring.h:74carries the identical line, indexed the same way byevpl_dgram_ring_waist(), and both rdmacm flush paths walk that ring too.The fix is the
& ring->maskthe sibling already uses.maskstill holds thepre-resize
size - 1on that line; it is reassigned two lines further down,after
size. So the reduction is modulo the old size, which is the index spacethe copy loop immediately above just remapped out of.
How wide is the bad range
Every tail/waist pair a full 8-slot ring can present, shipped expression against
the masked one:
28 of 64. Every disagreement is a negative index; there is no case where the
current expression is merely off by a slot.
Reproducer
Only the rdmacm paths advance a waist and I have no RDMA device here, so this
drives
evpl_iovec_ring_add()andevpl_iovec_ring_resize()directly against aDebug (ASan) build: fill the ring, drain most of it, refill so head wraps behind
tail, set the waist below tail, then add one more entry to force the grow.
Line 61 is
waist->length. The waist should have come out at 3. With the patch:Test
src/core/tests/iovec_ring_resize.cdoes the same setup and checks two thingsacross the grow: the waist lands inside the new array, and it still names the
entry it named before. On e27fb01:
It builds against the internal header rather than a protocol because a waist
only moves in
src/core/rdmacm/rdmacm.c, and putting it behind a protocol meanshardware in the loop.
Testing
Arch Linux, gcc 16.1.1. Both builds run under
unshare -r -nbecause this boxhas no CAP_NET_ADMIN for the netns wrapper, and
http/basicandhttp/chunkedwant port 80.
Both build clean under
-Werror, zero warnings.uncrustify -c etc/uncrustify.cfg --checkpasses on the two headers and the new test, andreuse lintis stillcompliant at 288/288.
Not tested on real RDMA hardware. I have none, so the read through the negative
index is shown above by driving the ring directly rather than by taking an RDMA
completion.
One thing I am unsure about: the test includes
core/iovec_ring.hdirectly, whichnothing else under
src/core/testsdoes. If you would rather this went in as aconformance case or not at all, say so and I will drop it and leave the two-line
fix.