fix(flow-entry): Fix entries overwriting for flow table#1290
Open
fix(flow-entry): Fix entries overwriting for flow table#1290
Conversation
We've had a long-standing issue where push()-ing a new entry into the
flow table's priority queue does not actually insert the new value when
an entry with a similar key already exists in the queue, resulting in an
invalid weak reference in the flow table.
Expected behaviour
------------------
Looking at our flow table implementation, we've got an insert() method,
defined in pkt-meta/src/flow_table/table.rs, that simply says: "Add a
flow to the table." Looking at this comment, the expectation is that,
when trying to insert an entry with a key that is already present in the
table, the new value will replace the existing one, and the old value
will be returned by the method.
Actual behaviour
----------------
- The old value is returned indeed
- The new value is not inserted correctly. Instead, we only keep a
"weak" reference in the table.
Explanations
------------
Here's what happens for the first insertion:
1. We want to create a new entry, we call self.insert(flow_key,
flow_info)
2. insert() takes an Arc pointer - a strong reference - to flow_info,
then calls self.insert_common(flow_key, &val) defined in
pkt-meta/src/flow_table/table.rs
3. insert_common() downgrades the pointer and stores a **weak pointer**
in the self.table (the "table" itself)
4. insert_common() also pushes a clone of the Arc **into the priority
queue**, to be able to reap expired entries later
5. insert_common() exits, returning None (no prior value)
6. insert() exits, the Arc gets out of scope and is dropped, but it's OK
because we still hold a clone in the priority queue so the weak
reference in self.table remains valid
7. ... Later, at lookup time, we find the weak reference in self.table
and successfully upgrade it to obtain an Arc to our initial flow_info
object
Now, here's what happens when trying to insert another value (same or
different), with an identical key:
1. We call self.insert(flow_key, flow_info)
2. insert() takes an Arc pointer to flow_info, then calls
self.insert_common(flow_key, &val)
3. insert_common() downgrades the pointer and stores a **weak pointer**
in the self.table (the "table" itself), **overwriting the existing
reference**
4. insert_common() tries also pushes a clone of the Arc into the
priority queue, _but wait!_
- It does so by calling the push() method, defined in
pkt-meta/src/flow_table/thread_local_pq.rs
- ... which calls in turn the priority queue's push(), for which the
docs say: "If an element equal to `item` is already in the queue,
its priority is updated and the old priority is returned in `Some`"
- Do we have the same item in the queue? Items are defined as a
couple "Entry { key, value }", and we may have a different value...
- ... but it doesn't matter, because this is determined using the
hash() method for Entry, defined in
pkt-meta/src/flow_table/thread_local_pq.rs, and which only hashes
the key
- So our items are identical indeed: we update its priority **and do
not replace the value in the priority queue**
5. insert_common() exits, correctly returning the old value
6. insert() exits, the Arc gets out of scope and is dropped: given that
we haven't stored a strong reference for the current value in the
priority queue, **the weak value in the table becomes invalid**
7. ... Later, at lookup time, we find the invalid weak reference, reap
it from the table, and return no value
Fixing the issue
----------------
Fix the issue by always removing any existing entry with an identical
key hash before trying to insert a new entry into the priority queue.
This way, we're sure we do not keep an older entry with a lingering weak
reference to a deleted value.
Fixes: 0bade5a ("feat(pkt-meta): Add initial flow table implementation")
Signed-off-by: Quentin Monnet <qmo@qmon.net>
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
1 participant
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.
We've had a long-standing issue where push()-ing an entry into the flow table's priority queue does not actually insert the new value when an entry with an identical key already exists in the queue, resulting in an invalid weak reference in the flow table; here's a fix.
See commit description or linked issue for details.
Fixes: #1085