- gerrit-log - lists.osmocom.org

[S] Change in libosmo-netif[master]: Makefile.am: Fix dependency: rebuild doxygen on src/include changes
by laforge 15 Mar '24

15 Mar '24

Attention is currently required from: laforge. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-netif/+/36289?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: Makefile.am: Fix dependency: rebuild doxygen on src/include changes ...................................................................... Makefile.am: Fix dependency: rebuild doxygen on src/include changes Change-Id: Id28f0927d01f82ceb969fb7c758cb391b86631c1 --- M Makefile.am 1 file changed, 10 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/89/36289/2 -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/36289?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Id28f0927d01f82ceb969fb7c758cb391b86631c1 Gerrit-Change-Number: 36289 Gerrit-PatchSet: 2 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in pysim[master]: osmo-smdpp: fix generation of transactionId
by dexter 15 Mar '24

15 Mar '24

Attention is currently required from: laforge. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/36295?usp=email ) Change subject: osmo-smdpp: fix generation of transactionId ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I noticed that when I compared the output from osmo-smdpp to the output from productive SMDP+ servers. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/36295?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I8439aa9d70f6fe798fa88b623bac13debdc19ca1 Gerrit-Change-Number: 36295 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Fri, 15 Mar 2024 08:34:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in pysim[master]: osmo-smdpp: fix generation of transactionId
by dexter 15 Mar '24

15 Mar '24

dexter has uploaded this change for review. ( https://gerrit.osmocom.org/c/pysim/+/36295?usp=email ) Change subject: osmo-smdpp: fix generation of transactionId ...................................................................... osmo-smdpp: fix generation of transactionId The hex string of the generated transactionId contains lowercase hex digits. However SGP.22 explicitly spcifies to use uppercase hex digits when using JSON fromatted messages. See section 6.5.2.6 for example. Related: SYS#6720 Change-Id: I8439aa9d70f6fe798fa88b623bac13debdc19ca1 --- M osmo-smdpp.py 1 file changed, 15 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/pysim refs/changes/95/36295/1 diff --git a/osmo-smdpp.py b/osmo-smdpp.py index 582b222..1f4311d 100755 --- a/osmo-smdpp.py +++ b/osmo-smdpp.py @@ -239,7 +239,7 @@ # Generate a TransactionID which is used to identify the ongoing RSP session. The TransactionID # SHALL be unique within the scope and lifetime of each SM-DP+. - transactionId = uuid.uuid4().hex + transactionId = uuid.uuid4().hex.upper() assert not transactionId in self.rss # Generate a serverChallenge for eUICC authentication attached to the ongoing RSP session. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/36295?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I8439aa9d70f6fe798fa88b623bac13debdc19ca1 Gerrit-Change-Number: 36295 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: newchange

1 0

[S] Change in libosmo-abis[rel-1.5.2]: Bump version: 1.5.1.3-0eed → 1.5.2
by fixeria 15 Mar '24

15 Mar '24

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36283?usp=email ) Change subject: Bump version: 1.5.1.3-0eed → 1.5.2 ...................................................................... Bump version: 1.5.1.3-0eed → 1.5.2 Change-Id: I307eac0e6d5f68e356250b1bea289645693073e4 --- M debian/changelog M src/Makefile.am 2 files changed, 19 insertions(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/debian/changelog b/debian/changelog index 37f214f..1a3ec80 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,12 @@ +libosmo-abis (1.5.2) unstable; urgency=medium + + [ Harald Welte ] + * [cosmetic] input/ipaccess.c: We use tab indent, not spaces [wtf] + * input/ipaccess: Make sure to say "TCP keepalive" + * Fix critical bug in default TCP keepalive user timeout + + -- Vadim Yanitskiy <vyanitskiy(a)sysmocom.de> Fri, 15 Mar 2024 00:32:06 +0700 + libosmo-abis (1.5.1) unstable; urgency=medium [ Harald Welte ] diff --git a/src/Makefile.am b/src/Makefile.am index 927d63f..0ef418c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,7 +1,7 @@ # This is _NOT_ the library release version, it's an API version. # Please read chapter "Library interface versions" of the libtool documentation # before making any modifications: https://www.gnu.org/software/libtool/manual/html_node/Versioning.html -ABIS_LIBVERSION=13:1:0 +ABIS_LIBVERSION=13:2:0 TRAU_LIBVERSION=9:1:7 AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include -I$(top_builddir) -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36283?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: rel-1.5.2 Gerrit-Change-Id: I307eac0e6d5f68e356250b1bea289645693073e4 Gerrit-Change-Number: 36283 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in libosmo-abis[rel-1.5.2]: [cosmetic] input/ipaccess.c: We use tab indent, not spaces [wtf]
by fixeria 15 Mar '24

15 Mar '24

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36280?usp=email ) Change subject: [cosmetic] input/ipaccess.c: We use tab indent, not spaces [wtf] ...................................................................... [cosmetic] input/ipaccess.c: We use tab indent, not spaces [wtf] Change-Id: I6ba728ccf7f6d81dc42d1ff82a29b557752e80ba (cherry picked from commit 361488f83f2cd721bc735ad65936320b2571590b) --- M src/input/ipaccess.c 1 file changed, 20 insertions(+), 10 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 48a1d40..d63c0d2 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -639,15 +639,15 @@ LOGP(DLINP, LOGL_ERROR, "Failed to set 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) + 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) LOGP(DLINP, LOGL_ERROR, - "Failed to set user timoeut: %s\n", - strerror(errno)); + "Failed to set user timoeut: %s\n", + strerror(errno)); } val = 1; @@ -718,8 +718,8 @@ struct osmo_fd *bfd; int i, ret; - /* We don't know yet which OML link to associate it with. Thus, we - * allocate a temporary E1 line until we have received ID. */ + /* We don't know yet which OML link to associate it with. Thus, we + * allocate a temporary E1 line until we have received ID. */ line = e1inp_line_clone(tall_ipa_ctx, link->line, "ipa_bfd"); if (line == NULL) { LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n"); -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36280?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: rel-1.5.2 Gerrit-Change-Id: I6ba728ccf7f6d81dc42d1ff82a29b557752e80ba Gerrit-Change-Number: 36280 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in libosmo-abis[rel-1.5.2]: input/ipaccess: Make sure to say "TCP keepalive"
by fixeria 15 Mar '24

15 Mar '24

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36281?usp=email ) Change subject: input/ipaccess: Make sure to say "TCP keepalive" ...................................................................... input/ipaccess: Make sure to say "TCP keepalive" We have TCP and IPA keepalive. Reading a message like "input/ipaccess.c:612 Keepalive is set: 0" is misleading, as one might assume it relates to IPA. Be explicit. Related: SYS#6801, OS#6375 Change-Id: I01cbda27eb7826eb11f44e034d746b7c39b399a4 (cherry picked from commit 715653778c33e31980452de86029720735cdcf4a) --- M src/input/ipaccess.c 1 file changed, 32 insertions(+), 10 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index d63c0d2..acbeda1 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -606,10 +606,10 @@ val = 1; ret = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &val, sizeof(val)); if (ret < 0) - LOGP(DLINP, LOGL_ERROR, "Failed to set keepalive: %s\n", + LOGP(DLINP, LOGL_ERROR, "Failed to enable TCP keepalive: %s\n", strerror(errno)); else - LOGP(DLINP, LOGL_NOTICE, "Keepalive is set: %i\n", ret); + LOGP(DLINP, LOGL_NOTICE, "TCP Keepalive is enabled\n"); /* The following options are not portable! */ val = line->keepalive_idle_timeout > 0 ? @@ -617,37 +617,41 @@ DEFAULT_TCP_KEEPALIVE_IDLE_TIMEOUT; ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &val, sizeof(val)); - if (ret < 0) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set keepalive idle time: %s\n", + "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)); - if (ret < 0) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set keepalive interval: %s\n", + "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)); - if (ret < 0) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set keepalive count: %s\n", + "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) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set user timoeut: %s\n", + "Failed to set TCP user timeout: %s\n", strerror(errno)); + } } val = 1; -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36281?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: rel-1.5.2 Gerrit-Change-Id: I01cbda27eb7826eb11f44e034d746b7c39b399a4 Gerrit-Change-Number: 36281 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[M] Change in libosmo-abis[rel-1.5.2]: Fix critical bug in default TCP keepalive user timeout
by fixeria 15 Mar '24

15 Mar '24

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36282?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, 57 insertions(+), 21 deletions(-) Approvals: laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index acbeda1..015407d 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,42 +611,39 @@ 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)); + 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 timeout: %s\n", -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36282?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: rel-1.5.2 Gerrit-Change-Id: Idca24d3e676a45d860d9eec60dc2097d8d87f3bf Gerrit-Change-Number: 36282 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in libosmo-abis[rel-1.4.2]: [cosmetic] input/ipaccess.c: We use tab indent, not spaces [wtf]
by fixeria 15 Mar '24

15 Mar '24

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36286?usp=email ) Change subject: [cosmetic] input/ipaccess.c: We use tab indent, not spaces [wtf] ...................................................................... [cosmetic] input/ipaccess.c: We use tab indent, not spaces [wtf] Change-Id: I6ba728ccf7f6d81dc42d1ff82a29b557752e80ba (cherry picked from commit 361488f83f2cd721bc735ad65936320b2571590b) --- M src/input/ipaccess.c 1 file changed, 20 insertions(+), 10 deletions(-) Approvals: laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 418606e..4f6353b 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -639,15 +639,15 @@ LOGP(DLINP, LOGL_ERROR, "Failed to set 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) + 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) LOGP(DLINP, LOGL_ERROR, - "Failed to set user timoeut: %s\n", - strerror(errno)); + "Failed to set user timoeut: %s\n", + strerror(errno)); } val = 1; @@ -718,8 +718,8 @@ struct osmo_fd *bfd; int i, ret; - /* We don't know yet which OML link to associate it with. Thus, we - * allocate a temporary E1 line until we have received ID. */ + /* We don't know yet which OML link to associate it with. Thus, we + * allocate a temporary E1 line until we have received ID. */ line = e1inp_line_clone(tall_ipa_ctx, link->line, "ipa_bfd"); if (line == NULL) { LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n"); -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36286?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: I6ba728ccf7f6d81dc42d1ff82a29b557752e80ba Gerrit-Change-Number: 36286 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[M] Change in libosmo-abis[rel-1.4.2]: Fix critical bug in default TCP keepalive user timeout
by fixeria 15 Mar '24

15 Mar '24

fixeria has submitted this change. ( 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, 57 insertions(+), 21 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index aac79f2..f038622 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,42 +611,39 @@ 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)); + 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 timeout: %s\n", -- 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: 2 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in libosmo-abis[rel-1.4.2]: input/ipaccess: Make sure to say "TCP keepalive"
by fixeria 15 Mar '24

15 Mar '24

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36277?usp=email ) Change subject: input/ipaccess: Make sure to say "TCP keepalive" ...................................................................... input/ipaccess: Make sure to say "TCP keepalive" We have TCP and IPA keepalive. Reading a message like "input/ipaccess.c:612 Keepalive is set: 0" is misleading, as one might assume it relates to IPA. Be explicit. Related: SYS#6801, OS#6375 Change-Id: I01cbda27eb7826eb11f44e034d746b7c39b399a4 (cherry picked from commit 715653778c33e31980452de86029720735cdcf4a) --- M src/input/ipaccess.c 1 file changed, 32 insertions(+), 10 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 4f6353b..aac79f2 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -606,10 +606,10 @@ val = 1; ret = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &val, sizeof(val)); if (ret < 0) - LOGP(DLINP, LOGL_ERROR, "Failed to set keepalive: %s\n", + LOGP(DLINP, LOGL_ERROR, "Failed to enable TCP keepalive: %s\n", strerror(errno)); else - LOGP(DLINP, LOGL_NOTICE, "Keepalive is set: %i\n", ret); + LOGP(DLINP, LOGL_NOTICE, "TCP Keepalive is enabled\n"); /* The following options are not portable! */ val = line->keepalive_idle_timeout > 0 ? @@ -617,37 +617,41 @@ DEFAULT_TCP_KEEPALIVE_IDLE_TIMEOUT; ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &val, sizeof(val)); - if (ret < 0) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set keepalive idle time: %s\n", + "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)); - if (ret < 0) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set keepalive interval: %s\n", + "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)); - if (ret < 0) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set keepalive count: %s\n", + "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) + if (ret < 0) { LOGP(DLINP, LOGL_ERROR, - "Failed to set user timoeut: %s\n", + "Failed to set TCP user timeout: %s\n", strerror(errno)); + } } val = 1; -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36277?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: I01cbda27eb7826eb11f44e034d746b7c39b399a4 Gerrit-Change-Number: 36277 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

2026

2025

2024

2023

2022

gerrit-log