fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36278?usp=email ) Change subject: Fix critical bug in default TCP keepalive user timeout ...................................................................... Fix critical bug in default TCP keepalive user timeout It turns out that our calculation of the TCP_USER_TIMEOUT value was flawed in several ways: * there should have been parenthesis around the + operator (line->keepalive_probe_interval + line->keepalive_idle_timeout) as the keepalive_idle_timeout is in seconds, not milli-seconds. * in the default case, all those values are configured to -1 (E1INP_USE_DEFAULT). This means we're using 1000 * -1 * -1 + -1 = 999 i.e. just below a second which clearly is not enough for a lossy satellite or wifi back-haul. This fixes a regression introduced in Ia7659c209aea0d26eb37d31e771adc91b17ae668 (libosmo-abis >= 1.4.0) when TCP keepalive user timeouts became enabled by default. The initial support for TCP_USER_TIMEOUT was merged in I5e7425958472aa5d758e09bfbefc7d7d37bf6f5f (libosmo-abis >= 0.7.0) but since TCP keepalives were not yet enabled by default, only users with explicit TCP keepalive configuration in their config files would be affected - and then only of the second part of the bug (operator precedence). In addition, let's print the actually-used values to the log, helping to spot unintended values. Change-Id: Idca24d3e676a45d860d9eec60dc2097d8d87f3bf Related: OS#5785, OS#6375, SYS#6801 Fixes: Ia7659c209aea0d26eb37d31e771adc91b17ae668 (cherry picked from commit 12fae9aeebd86e242abd5c682a12fd1c9da68497) --- M src/input/ipaccess.c 1 file changed, 60 insertions(+), 24 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-abis refs/changes/78/36278/1 diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 2383265..88b5ac0 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -599,7 +599,7 @@ static void update_fd_settings(struct e1inp_line *line, int fd) { int ret; - int val; + int val, idle_val, interval_val, retry_count_val, user_timeout_val; if (line->keepalive_num_probes) { /* Enable TCP keepalive to find out if the connection is gone */ @@ -611,46 +611,43 @@ else LOGP(DLINP, LOGL_NOTICE, "TCP Keepalive is enabled\n"); + idle_val = line->keepalive_idle_timeout > 0 ? + line->keepalive_idle_timeout : + DEFAULT_TCP_KEEPALIVE_IDLE_TIMEOUT; + interval_val = line->keepalive_probe_interval > -1 ? + line->keepalive_probe_interval : + DEFAULT_TCP_KEEPALIVE_INTERVAL; + retry_count_val = line->keepalive_num_probes > 0 ? + line->keepalive_num_probes : + DEFAULT_TCP_KEEPALIVE_RETRY_COUNT; + user_timeout_val = 1000 * retry_count_val * (interval_val + idle_val); + LOGP(DLINP, LOGL_NOTICE, + "TCP keepalive idle_timeout=%us, interval=%us, retry_count=%u user_timeout=%ums\n", + idle_val, interval_val, retry_count_val, user_timeout_val); /* The following options are not portable! */ - val = line->keepalive_idle_timeout > 0 ? - line->keepalive_idle_timeout : - DEFAULT_TCP_KEEPALIVE_IDLE_TIMEOUT; - ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, - &val, sizeof(val)); + ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &idle_val, sizeof(idle_val)); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "Failed to set TCP keepalive idle time: %s\n", strerror(errno)); } - val = line->keepalive_probe_interval > -1 ? - line->keepalive_probe_interval : - DEFAULT_TCP_KEEPALIVE_INTERVAL; - ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, - &val, sizeof(val)); + ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &interval_val, sizeof(interval_val)); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "Failed to set TCP keepalive interval: %s\n", strerror(errno)); } - val = line->keepalive_num_probes > 0 ? - line->keepalive_num_probes : - DEFAULT_TCP_KEEPALIVE_RETRY_COUNT; - ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, - &val, sizeof(val)); + ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &retry_count_val, sizeof(retry_count_val)); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "Failed to set TCP keepalive count: %s\n", strerror(errno)); } - val = 1000 * line->keepalive_num_probes * - line->keepalive_probe_interval + - line->keepalive_idle_timeout; - ret = setsockopt(fd, IPPROTO_TCP, TCP_USER_TIMEOUT, - &val, sizeof(val)); - if (ret < 0) { + ret = setsockopt(fd, IPPROTO_TCP, TCP_USER_TIMEOUT, &user_timeout_val, sizeof(user_timeout_val)); + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set TCP user timoeut: %s\n", - strerror(errno)); + "Failed to set TCP user timeout: %s\n", + strerror(errno)); } } -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36278?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: rel-1.4.2 Gerrit-Change-Id: Idca24d3e676a45d860d9eec60dc2097d8d87f3bf Gerrit-Change-Number: 36278 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-MessageType: newchange