Tunnel/v8 #10597
Tunnel/v8 #10597
Conversation
v4 was doing redundant recursion level setup. v6 was missing PKT_REBUILT_FRAGMENT flag.
In preparation of cleaning up thread safety, move "verdicted" logic out of Packet::flags. Unsafe writes to "flags" can potentially have side effects.
Give each packet explicit tunnel type `ttype`: none, root, child. Assigning happens when a (tunnel) packet is set up and is thread safe.
Allows caller to take their own lock.
No longer update `Packet::flags` for tracking packet modifications, as thread safety was not guaranteed. Clearly separate between various kinds of `Packet::nfq_v` accesses for: - mark - mark_modified - verdicted These are either done under lock (Packet::persistent.tunnel_lock) or, if the Packet is not part of a tunnel, not under lock. This is safe as in all the related logic the Packet's tunnel state is fixed and can no longer change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10597 +/- ##
===========================================
+ Coverage 64.07% 82.73% +18.65%
===========================================
Files 851 924 +73
Lines 135327 247448 +112121
===========================================
+ Hits 86716 204722 +118006
+ Misses 48611 42726 -5885
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 19132 |
| @@ -1015,7 +1018,7 @@ void DecodeUnregisterCounters(void); | |||
| #define PKT_STREAM_NOPCAPLOG BIT_U32(12) | |||
|
|
|||
| #define PKT_TUNNEL BIT_U32(13) | |||
| #define PKT_TUNNEL_VERDICTED BIT_U32(14) | |||
| // vacancy | |||
|
|
|||
| /** Packet checksum is not computed (TX packet for example) */ | |||
| #define PKT_IGNORE_CHECKSUM BIT_U32(15) | |||
There was a problem hiding this comment.
Can't all the defines be shifted by two?
There was a problem hiding this comment.
Not shifting to keep flags stable. This mostly helps in debugging reported core dumps.
| } else { | ||
| verdict = false; | ||
| } | ||
| verdict = VerdictTunnelPacketInternal(p); |
There was a problem hiding this comment.
I guess I don't like to declare a var inside a lock, then use it outside. It's a matter of style I think, I don't think it has practical effect
There was a problem hiding this comment.
ok, the comment was based on the experience from the previous PRs.
| /** Packet mark is modified */ | ||
| #define PKT_MARK_MODIFIED BIT_U32(11) | ||
|
|
||
| // vacancy |
|
Merged in #10629, thanks! |
Tunnel handling updates to improve thread safety.
Misc related cleanups.
Replaces #9985: