napt: Fixes and improvements (IDFGH-6861) #43

rojer · 2022-02-24T23:47:33Z

Fix enable/disable to properly allocate and deallocate tables.
Current algorithm is just broken.
Introduce eviction policy when table gets full: oldest connection
is evicted, instead of new ones getting silently dropped. this
results in much better behavior with small tables than before.
When TCP connection is dropped, RSTs are sent both ways to inform
parties instead of dropping silently. thiw requires additional 8
bytes per entry but is, again, a big improvement for clients in
terms of usability.
FIxed handling of timestamp wraparound (every ~50 days of uptime).
Added ip_portmap_get() to retrieve current port mapping settings.
Added ip_napt_get_stats() for some insight into the state of NAT.

Alvin1Zhang · 2022-03-01T01:29:22Z

Thanks for your contribution.

david-cermak

Thanks for the fixes! LGTM in general

src/core/ipv4/ip4_napt.c

src/core/timeouts.c

src/core/ipv4/ip4_napt.c

david-cermak · 2022-03-10T12:10:22Z

Current algorithm is just broken.

It would be nice to cover this in the test cases.

Also, the current NAPT test suite fails (on compilation):

../../../../src/core/ipv4/ip4_napt.c:64:16: error: C++ style comments are incompatible with C90 [-Werror]
   u32_t src;   // net
                ^

To run the lwip unit tests on host, please follow these steps:

esp-lwip/.gitlab-ci.yml

Lines 33 to 46 in 76303df

    
           - cd ports/unix/check/ 
        
           # updating environment 
        
           - export LWIPDIR=../../../../src && export CK_DEFAULT_TIMEOUT=${TEST_TIMEOUT} 
        
           - export EXTRA_CFLAGS="" && export CC="cc $EXTRA_CFLAGS" && export CCDEP=cc 
        
           # build and run default lwip tests 
        
           - make -j 4 check 
        
           # retest with IP_FORWARD enabled 
        
           - make clean 
        
           - export EXTRA_CFLAGS="-DIP_FORWARD=1" && export CC="cc $EXTRA_CFLAGS" 
        
           - make -j 4 check 
        
           # retest with IP_FORWARD and IP_NAPT enabled 
        
           - make clean 
        
           - export EXTRA_CFLAGS="-DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h" && export CC="cc $EXTRA_CFLAGS" 
        
           - make -j 4 check

rojer · 2022-03-20T18:07:34Z

thanks for the review, all done.
however, i was not able to run the tests as they don't seem to be present in the public repository - there's no ports/ directory.

david-cermak · 2022-03-21T09:17:19Z

@rojer Thanks for the update! Seems like you forgot to revert the changes in src/core/timeouts.c after using sys_timeout()

there's no ports/ directory.

The port directory is present in the contrib repo. I'm sorry, running the CI locally hasn't been very straight-forward. Currently working on improvements for the upcoming branches using GitHub workflows (so contributors can easily check their work). You can temporarily use these workflows to test your changes, though it's very WIP and present only on my personal fork. Tried to pick your changes here, CI fails with the forgotten timeout changes, mentioned above.

rojer · 2022-03-21T21:41:07Z

Seems like you forgot to revert the changes in src/core/timeouts.c after using sys_timeout()

yes, you're right, forgot to commit that change. done.

The port directory is present in the contrib repo

ok, i cloned it and checked out STABLE-2_1_0_RELEASE but now check.h is missing:

[rojer@nbd ~/cesanta/esp/lwip-contrib 35b011d4cf4c4b480f8859c456587a884ec9d287]$ ./qq.sh 
+ cd ports/unix/check/
+ export LWIPDIR=/home/rojer/cesanta/esp/esp-lwip/src
+ LWIPDIR=/home/rojer/cesanta/esp/esp-lwip/src
+ export CK_DEFAULT_TIMEOUT=1000
+ CK_DEFAULT_TIMEOUT=1000
+ export EXTRA_CFLAGS=
+ EXTRA_CFLAGS=
+ export 'CC=cc '
+ CC='cc '
+ export CCDEP=cc
+ CCDEP=cc
+ make -j 1 check
cc  -DLWIP_NOASSERT_ON_ERROR -I/usr/include/check -I/home/rojer/cesanta/esp/esp-lwip/src/../test/unit -g -DLWIP_DEBUG -Wall -pedantic -Werror -Wparentheses -Wsequence-point -Wswitch-default -Wextra -Wundef -Wshadow -Wpointer-arith -Wcast-qual -Wc++-compat -Wwrite-strings -Wold-style-definition -Wcast-align -Wmissing-prototypes -Wredundant-decls -Wnested-externs -Wunreachable-code -Wuninitialized -Wmissing-prototypes -Wredundant-decls -Waggregate-return -Wlogical-not-parentheses -Wlogical-op -Wc90-c99-compat -Wtrampolines -I. -I../../.. -I/home/rojer/cesanta/esp/esp-lwip/src/include -I../../../ports/unix/port/include -c /home/rojer/cesanta/esp/esp-lwip/src/../test/unit/lwip_unittests.c
In file included from /home/rojer/cesanta/esp/esp-lwip/src/../test/unit/lwip_unittests.c:1:
/home/rojer/cesanta/esp/esp-lwip/src/../test/unit/lwip_check.h:7:10: fatal error: check.h: No such file or directory
    7 | #include <check.h>
      |          ^~~~~~~~~
compilation terminated.
make: *** [/home/rojer/cesanta/esp/lwip-contrib/ports/unix/../Common.allports.mk:91: lwip_unittests.o] Error 1

rojer · 2022-03-21T21:59:45Z

check.h is missing

apt-get install check fixed it.

ok, updated the branch. in addition to issues with my patch i had to fix two other unrelated C90/C99 issues that prevented code from building, i have no idea how it works on the CI but they are legit in C90 mode.

tests are now passing:

+ export 'EXTRA_CFLAGS=-DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ EXTRA_CFLAGS='-DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ export 'CC=cc -DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ CC='cc -DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h'
+ make -j 4 check
...
Running suite(s): IPv4
 IPv6
 UDP
 TCP
 TCP_OOS
 TCP_STATE
 DEF
 MEM
 NETIF
 PBUF
 TIMERS
 ETHARP
 DHCP
 MDNS
 MQTT
 SOCKETS
 IP4_ROUTE
100%: Checks: 126, Failures: 0, Errors: 0

david-cermak

Thanks again for the update! just some minor nitpicks.

david-cermak · 2022-03-22T07:31:28Z

src/core/ipv4/ip4_napt.c

+
+#include "assert.h"
+#include "stdbool.h"
+#include "string.h"


Nitpick (same above)

Suggested change

#include "string.h"

#include <string.h>

src/core/ipv4/ip4_napt.c

david-cermak · 2022-03-22T07:50:52Z

src/core/ipv4/ip4_napt.c


 #define NO_IDX ((u16_t)-1)
 #define NT(x) ((x) == NO_IDX ? NULL : &ip_napt_table[x])

-struct napt_table {
+#pragma GCC diagnostic push


I would personally prefer using basic integer types to placing #pragma's into generic lwip code. These directives apparently work in clang, so okay with me, feel free to ignore.

One correction, this won't compile in clang:

clang -DIP_FORWARD=1 -DIP_FORWARD=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h -DLWIP_NOASSERT_ON_ERROR -I/usr/include/check -I../../../../src/../test/unit -Wno-gnu-zero-variadic-macro-arguments -g -DLWIP_DEBUG -Wall -pedantic -Werror -Wparentheses -Wsequence-point -Wswitch-default -Wextra -Wundef -Wshadow -Wpointer-arith -Wcast-qual -Wc++-compat -Wwrite-strings -Wold-style-definition -Wcast-align -Wmissing-prototypes -Wredundant-decls -Wnested-externs -Wunreachable-code -Wuninitialized -Wmissing-prototypes -Wredundant-decls -Waggregate-return -Wlogical-not-parentheses -fsanitize=address -fsanitize=undefined -fno-sanitize=alignment -Wdocumentation -Wno-documentation-deprecated-sync -I. -I../../.. -I../../../../src/include -I../../../ports/unix/port/include -c ../../../../src/core/ipv6/dhcp6.c ../../../../src/core/ipv4/ip4_napt.c:64:32: error: unknown warning group '-Wc90-c99-compat', ignored [-Werror,-Wunknown-warning-option] #pragma GCC diagnostic ignored "-Wc90-c99-compat" /* To allow u8_t bit fields */ ^ 1 error generated.

ci link

so when compiling with the extra bitchy flags GCC complains about the bitfield underlying type being uint8 instead of a unsigned (or signed) int, as per standard. fine. but in practice it works and i don't feel comfortable about adding extra 3 bytes to the size of the struct just to maintain purity.
clang doesn't complain about it, so i ifdefed this out for it, now it builds.

david-cermak · 2022-03-22T08:00:08Z

Nice, that you succeeded in running the tests eventually! Still some issues in the debug prints though:

../../../../src/core/ipv4/ip4_napt.c: In function ‘napt_debug_print’:
../../../../src/core/ipv4/ip4_napt.c:143:6: error: ISO C90 forbids mixed declarations and code [-Werror=c90-c99-compat]
  143 |      uint8_t p = t->proto;
      |      ^~~~~~~

Please note, that the test with debug logs are not executed in the CI:

esp-lwip/.gitlab-ci.yml

Lines 47 to 50 in 76303df

    
           # Please uncomment the below to test IP_FORWARD/IP_NAPT tests with debug output (only ip4_route test suite will be executed) 
        
           #- make clean 
        
           #- export EXTRA_CFLAGS="-DIP_FORWARD=1 -DESP_TEST_DEBUG=1 -DIP_NAPT=1 -DLWIP_ARCH_CC_H -include cc_esp_platform.h" && export CC="cc $EXTRA_CFLAGS" 
        
           #- make -j 4 check

just wanted to see how the NAPT tests would work with the newly introduced eviction mechanism you implemented, seems like the oldest record was removed here:

david-cermak · 2022-03-22T08:08:52Z

Note: I'm passing this PR to an internal review, which usually takes some time until accepted and merged.

1. Fix enable/disable to properly allocate and deallocate tables. Current algorithm is just broken. 2. Introduce eviction policy when table gets full: oldest connection is evicted, instead of new ones getting silently dropped. this results in much better behavior with small tables than before. When TCP connection is dropped, RSTs are sent both ways to inform parties instead of dropping silently. thiw requires additional 8 bytes per entry but is, again, a big improvement for clients in terms of usability. 3. FIxed handling of timestamp wraparound (every ~50 days of uptime). 3. Added ip_portmap_get() to retrieve current port mapping settings. 4. Added ip_napt_get_stats() for some insight into the state of NAT.

rojer · 2022-03-27T21:42:55Z

@david-cermak addressed comments, rebased onto most recent 2.1.2-esp

github-actions bot changed the title ~~napt: Fixes and improvements~~ napt: Fixes and improvements (IDFGH-6861) Mar 1, 2022

david-cermak requested changes Mar 10, 2022

View reviewed changes

src/core/ipv4/ip4_napt.c Outdated Show resolved Hide resolved

src/core/ipv4/ip4_napt.c Outdated Show resolved Hide resolved

src/core/timeouts.c Outdated Show resolved Hide resolved

src/core/ipv4/ip4_napt.c Outdated Show resolved Hide resolved

david-cermak mentioned this pull request Mar 14, 2022

Lwip with port forwarding on same netif (IDFGH-6860) #44

Closed

rojer force-pushed the napt_fixes branch from e64e84e to 73db2a6 Compare March 20, 2022 18:06

rojer force-pushed the napt_fixes branch from 73db2a6 to b3310ee Compare March 21, 2022 21:37

rojer force-pushed the napt_fixes branch from b3310ee to fc74c41 Compare March 21, 2022 21:57

david-cermak approved these changes Mar 22, 2022

View reviewed changes

rojer force-pushed the napt_fixes branch 2 times, most recently from c29735a to a46e0b4 Compare March 27, 2022 21:37

rojer force-pushed the napt_fixes branch from a46e0b4 to fb1f355 Compare March 27, 2022 21:42

igrr merged commit fb1f355 into espressif:2.1.2-esp Apr 14, 2022

rojer deleted the napt_fixes branch April 15, 2022 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

napt: Fixes and improvements (IDFGH-6861) #43

napt: Fixes and improvements (IDFGH-6861) #43

rojer commented Feb 24, 2022

Alvin1Zhang commented Mar 1, 2022

david-cermak left a comment

david-cermak commented Mar 10, 2022

rojer commented Mar 20, 2022

david-cermak commented Mar 21, 2022 •

edited

Loading

rojer commented Mar 21, 2022

rojer commented Mar 21, 2022

david-cermak left a comment

david-cermak Mar 22, 2022

david-cermak Mar 22, 2022

david-cermak Mar 24, 2022

rojer Mar 27, 2022

david-cermak commented Mar 22, 2022

david-cermak commented Mar 22, 2022

rojer commented Mar 27, 2022

napt: Fixes and improvements (IDFGH-6861) #43

napt: Fixes and improvements (IDFGH-6861) #43

Conversation

rojer commented Feb 24, 2022

Alvin1Zhang commented Mar 1, 2022

david-cermak left a comment

Choose a reason for hiding this comment

david-cermak commented Mar 10, 2022

rojer commented Mar 20, 2022

david-cermak commented Mar 21, 2022 • edited Loading

rojer commented Mar 21, 2022

rojer commented Mar 21, 2022

david-cermak left a comment

Choose a reason for hiding this comment

david-cermak Mar 22, 2022

Choose a reason for hiding this comment

david-cermak Mar 22, 2022

Choose a reason for hiding this comment

david-cermak Mar 24, 2022

Choose a reason for hiding this comment

rojer Mar 27, 2022

Choose a reason for hiding this comment

david-cermak commented Mar 22, 2022

david-cermak commented Mar 22, 2022

rojer commented Mar 27, 2022

david-cermak commented Mar 21, 2022 •

edited

Loading