issue: 4043231 Fix keepalive abort to honor TCP_USER_TIMEOUT - #590
issue: 4043231 Fix keepalive abort to honor TCP_USER_TIMEOUT#590tomerdbz wants to merge 1 commit into
Conversation
|
VMA analyzer: [1/2] lwip/opt.h... ✅ relevant ──────────────────────────────────────── ✅ 2 change(s) MAY BE RELEVANT to libvma: TODO - open a task for libvma |
Greptile SummaryThis PR fixes keepalive behavior in XLIO's lwIP TCP stack so that
Confidence Score: 4/5The core keepalive timer logic is sound and last_progress_tmr anchor points are correctly placed, but the setsockopt handlers for the newly enabled TCP_KEEPINTVL and TCP_KEEPCNT options do not enforce the upper-bound limits that the newly added negative test cases exercise, so those tests will fail as written. The abort-path algorithm is correctly implemented: last_progress_tmr is seeded at connection establishment and updated only on genuine forward progress, the user_timeout branch measures elapsed time from the right reference point, and the keep_cnt-based fallback matches previous behavior. However, the setsockopt handlers for TCP_KEEPINTVL and TCP_KEEPCNT reject only non-positive values and do not reject values above INT16_MAX seconds and INT8_MAX respectively, meaning the negative test cases added in this PR will fail. src/core/sock/sockinfo_tcp.cpp (TCP_KEEPINTVL and TCP_KEEPCNT setsockopt handlers) and tests/gtest/tcp/tcp_sockopt.cc (negative test cases for the upper bounds of those two options). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[tcp_slowtmr tick] --> B{SOF_KEEPALIVE set and ESTABLISHED?}
B -- No --> Z[Done]
B -- Yes --> C[ka_elapsed = tcp_ticks - pcb->tmr
ut_elapsed = tcp_ticks - last_progress_tmr]
C --> D{user_timeout_ms != 0?}
D -- Yes --> E{ut_elapsed >= user_timeout_ms AND >= keep_idle?}
D -- No --> F{ka_elapsed > keep_idle + keep_cnt x keep_intvl?}
E -- Yes --> G[ka_abort = true]
E -- No --> H[ka_abort = false]
F -- Yes --> G
F -- No --> H
G --> I[Abort: pcb_remove++ pcb_reset++]
H --> J{ka_elapsed > keep_idle + keep_cnt_sent x keep_intvl?}
J -- Yes --> K[Send probe, keep_cnt_sent++ up to UINT8_MAX]
J -- No --> Z
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[tcp_slowtmr tick] --> B{SOF_KEEPALIVE set and ESTABLISHED?}
B -- No --> Z[Done]
B -- Yes --> C[ka_elapsed = tcp_ticks - pcb->tmr
ut_elapsed = tcp_ticks - last_progress_tmr]
C --> D{user_timeout_ms != 0?}
D -- Yes --> E{ut_elapsed >= user_timeout_ms AND >= keep_idle?}
D -- No --> F{ka_elapsed > keep_idle + keep_cnt x keep_intvl?}
E -- Yes --> G[ka_abort = true]
E -- No --> H[ka_abort = false]
F -- Yes --> G
F -- No --> H
G --> I[Abort: pcb_remove++ pcb_reset++]
H --> J{ka_elapsed > keep_idle + keep_cnt_sent x keep_intvl?}
J -- Yes --> K[Send probe, keep_cnt_sent++ up to UINT8_MAX]
J -- No --> Z
Reviews (10): Last reviewed commit: "issue: 4043231 Fix keepalive abort to ho..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| */ | ||
| #ifndef LWIP_TCP_KEEPALIVE | ||
| #define LWIP_TCP_KEEPALIVE 0 | ||
| #define LWIP_TCP_KEEPALIVE 1 |
There was a problem hiding this comment.
you keep ability to toggle LWIP_TCP_KEEPALIVE , however, you remove build-time branching in 2 places below. So when you disable keepalive here the compilation will fail.
Either make proper compilation in all cases or remove the LWIP_TCP_KEEPALIVE entirely from the code.
There was a problem hiding this comment.
removed LWIP_TCP_KEEPALIVE
|
libvma Relevance Analyzer [1/3] lwip/tcp.c (2 hunks)... ✅ relevant ──────────────────────────────────────── ✅ 3 change(s) MAY BE RELEVANT to libvma: Consider applying these changes to libvma as well. |
676c497 to
04af23a
Compare
| si_tcp_logwarn( | ||
| "TCP_USER_TIMEOUT value %u exceeds RFC 5482 maximum, clamping to %u", | ||
| user_timeout_ms, TCP_USER_TIMEOUT_MAX_MS); |
There was a problem hiding this comment.
WARN level of logs will likely be printed out. This can lead to a flood in CPS scenario.
Linux kernel returns EINVAL when the value is <0 when casted to signed int. Probably we can return EINVAL instead of reducing the value?
| } else if (ka_elapsed > (pcb->keep_idle + pcb->keep_cnt_sent * pcb->keep_intvl) / | ||
| slow_tmr_interval) { | ||
| tcp_keepalive(pcb); | ||
| pcb->keep_cnt_sent++; | ||
| if (pcb->keep_cnt_sent < UINT8_MAX) { | ||
| pcb->keep_cnt_sent++; | ||
| } | ||
| } |
There was a problem hiding this comment.
does it mean that when we send 255 keepalives we start sending them every timer tick regardless of the keepalive parameters? What if we configure TCP_USER_TIMEOUT to that high allowed value, can this counter reach the 8-bit limit?
There was a problem hiding this comment.
as discussed privately see this:
04af23a to
1742209
Compare
1742209 to
a589d47
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
bot:retest |
35a58e6 to
d739cad
Compare
Lab RED-GREEN evidence (issue 4043231)
|
| @@ -4697,7 +4706,6 @@ int sockinfo_tcp::tcp_setsockopt(int __level, int __optname, __const void *__opt | |||
| m_pcb.keep_cnt = keep_cnt; | |||
| } | |||
There was a problem hiding this comment.
The handlers for TCP_KEEPINTVL and TCP_KEEPCNT now always execute (guards removed), but neither enforces the upper-bound limits that Linux imposes and that the newly-enabled negative tests document: TCP_KEEPINTVL must be ≤ INT16_MAX (32 767) seconds, and TCP_KEEPCNT must be ≤ INT8_MAX (127). Both setsockopt handlers only reject non-positive values; they accept 32 768 s or 128 probes respectively, causing the tcp_setsockopt_negative tests added by this PR to fail. For TCP_KEEPINTVL, a value of 32 768 s × 1000 also silently overflows u32_t in pcb->keep_intvl.
When TCP_USER_TIMEOUT is set on a keepalive connection, the abort must be
measured from the last real data progress - matching Linux
tcp_keepalive_timer(), which uses rcv_tstamp/lrcvtime - not from a
reference that a bare keepalive-probe reply advances. Two independent
paths otherwise anchored the timeout on the first keepalive probe, so an
idle connection aborted at keep_idle + user_timeout (~20s) instead of
user_timeout (~12s):
Path A - tcp_user_timeout_occured() reads pcb->ticks_since_data_sent,
which tcp_keepalive() set to 0 on the first probe, so an otherwise-idle
connection started that counter at keep_idle.
Path B - the SOF_KEEPALIVE block measured elapsed from pcb->tmr, which
tcp_in.c resets on every received segment, including the reply to the
first keepalive probe.
Fix:
- Add pcb->last_progress_tmr, set to tcp_ticks only on real forward
progress (connection establishment, in-sequence data received, new
data acknowledged); never advanced by keepalive-probe replies.
- Path B measures user_timeout from last_progress_tmr, with a keep_idle
guard so a probe is attempted before aborting an idle connection when
user_timeout < keep_idle.
- tcp_keepalive() no longer anchors ticks_since_data_sent (Path A); the
zero-window/persist caller in tcp_slowtmr() anchors it explicitly (it
has real pending data), preserving persist + user_timeout behavior.
The keep_cnt-based path (user_timeout == 0) is unchanged. Enable
LWIP_TCP_KEEPALIVE so per-socket TCP_KEEPINTVL/TCP_KEEPCNT are honored.
Signed-off-by: Tomer Cabouly <tcabouly@nvidia.com>
2 participants
Description
When TCP_USER_TIMEOUT is set on a keepalive connection, the abort decision should be based on elapsed time since last received data, not on the keepalive probe count (TCP_KEEPCNT). This matches the Linux tcp_keepalive_timer() behavior where TCP_USER_TIMEOUT overrides TCP_KEEPCNT.
Previously, the keepalive abort always used keep_cnt * keep_intvl, ignoring TCP_USER_TIMEOUT entirely. The separate tcp_user_timeout_ occured() check could eventually fire but measured from the wrong reference point (first probe sent, not last data received), making the effective timeout keep_idle + user_timeout instead of just user_timeout.
Enable LWIP_TCP_KEEPALIVE so that per-socket TCP_KEEPINTVL and TCP_KEEPCNT values are honored instead of using fixed defaults.
What
Fix keepalive abort to honor TCP_USER_TIMEOUT and enable per-socket TCP_KEEPINTVL/TCP_KEEPCNT.
Why ?
issue: 4043231 - When TCP_USER_TIMEOUT is set on a keepalive connection, XLIO aborts based on
probe count (TCP_KEEPCNT) instead of elapsed time.
This differs from Linux, where
TCP_USER_TIMEOUT overrides TCP_KEEPCNT and the connection drops when elapsed time since last
received data exceeds the user timeout.
Additionally, TCP_KEEPINTVL and TCP_KEEPCNT socket
options were silently ignored (LWIP_TCP_KEEPALIVE was compiled out).
How ?
Modify the keepalive abort block in tcp_slowtmr() to branch on user_timeout_ms:
(matches Linux tcp_keepalive_timer() logic).
Enable LWIP_TCP_KEEPALIVE=1 so per-socket keep_intvl and keep_cnt fields (already initialized
from /proc/sys/net/ipv4/tcp_keepalive_* in the constructor) are used by the timer and exposed
via setsockopt/getsockopt.
Change type
What kind of change does this PR introduce?
Check list