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

[mtnbkr88]

As currently written in ESP-IDF 4.4.0 (lwip 2.1.2), lwip cannot possibly forward back onto the same network (what I need) without errors. I will explain. If A is the source, B the port forwarder and C the destination, as currently written ip4_forward only modifies the destination in the packet. So a packet will be forwarded with source as A and destination as C. This is okay if the packet is going to a different network than the source. If the destination is on the same network (netif) as the source then when C receives the packet it will return directly to A. Then A will drop the packet because it does not have any known connections with C. A had a connection open to B, not C. (I verified this behavior with wireshark.) My update to lwip fixes this. If the packet is forwarded to the same network as the source, my update will have destination C return to B which will then return to A. All traffic is as expected, all connections are properly handled. I also updated some of the debug messages to aid in debugging this issue.

Note that IP_FORWARD, IP_NAPT and IP_FORWARD_ALLOW_TX_ON_RX_NETIF must all be enabled in opt.h (something I had to do manually since menuconfig does not currently handle this) in order for the port forwarding functionality to work.

[Alvin1Zhang]

Thanks for your contribution.

[david-cermak]

Thanks again for posting your changes! I would suggest updating your newlines from CRLF to LF for us to easier review the changes.

Also, thanks for explaining the usecase of a local link port forwarder. Do you think this feature would be useful for anything else? Did you check that the standard NAPT functionality still works?

As mentioned in another PR, it would be good if you can run the current set of unit tests to check your changes didn't break anything. Please see here the lists of command to compile and execute the tests on linux:

https://github.com/espressif/esp-lwip/blob/76303df2386902e0d7873be4217f1d9d1b50f982/.gitlab-ci.yml#L33-L46

Please note that LWIP_CONTRIB_MIRROR is at https://git.savannah.nongnu.org/cgit/lwip/lwip-contrib.git

[mtnbkr88]

david-cermak - Thank you for taking a look at my request.

First to answer your questions... 1) the use case for me was the need to handle port forwarding back onto the same network (netif). My local router can only handle 20 port forwarding rules for routing outside (WAN) ports to local (LAN) port/IPs. I needed more. I can use one rule to port forward a range of ports to one local IP. So I needed that IP (the ESP32 in this case) to then be able to forward those ports to other port/IPs on my local network. I'm not aware of other use cases for this feature. 2) I did check that the original NAPT functionality still works.

Now for your requested changes... 1) If you look at the LWIP_DEBUGF statements in the original ip4.c and ip4_napt.c files they use \n to terminate lines that should start a new line. I used the exact same syntax in the LWIP_DEBUGF statements I added or modified. The debug output prints clean and correct when I use "idf.py -p COM11 monitor" from the ESP-IDF CMD prompt on my Windows 10 PC. If you are asking me to change this then you are asking me to change all the debug statements in the two files mentioned, most of which were written by previous authors, not me. If you can give me a specific example of a CRLF that looks bad to you, I will take a look at it. 2) Regarding the unit testing, my testing shows no errors. I don't know how to use or run any "current set" of unit tests. I use a Windows 10 ESP-IDF CMD prompt with ipf.py to build, flash and monitor serial output and notepad for my development environment. The linux example test commands you gave I don't think will work for me in my Windows CMD prompt. 3) The "const struct ip_hdr *iphdr;" error referenced above is not my code. That is original to the ip4.c that comes with espressif:2.1.2-esp. I don't think it's my place to change that.

Please remove your change request since it references other people's work in the current 2.1.2 version of lwip, not my changes.

This is my first attempt to contribute to this really cool ESP32 stuff. So this whole process is new to me. Consequently I don't know the correct method to get my response back to you.

Thanks.

[david-cermak]

@mtnbkr88 Thanks for explaining the usecase and checking the original NAPT functionality!

If you look at the LWIP_DEBUGF statements in the original ip4.c and ip4_napt.c files they use \n to terminate lines

1) Sorry, didn't mean the newlines in debug messages, but windows vs. linux line-endings. Please check these diffs:

• corrected newlines
• your original changes Note, that the second shows the file are 100% difference, which is very hard to review

Regarding the unit testing, my testing shows no errors.

2) My previous comment described how you can run and test the current suite. just clone the contrib repo and follow the steps line by line, If you're on windows, I'd suggest running this in WSL2.

The "const struct ip_hdr *iphdr;" error referenced above is not my code.

3) You haven't changed that particular line, but added a debug message before. This is invalid in C-90 standard.

ip4_input(struct pbuf *p, struct netif *inp)
{
+  LWIP_DEBUGF(IP_DEBUG, ("ip4_input: start\n"));

  const struct ip_hdr *iphdr;

[mtnbkr88]

@david-cermak

I corrected the windows vs unix new-lines in ip4.c ip4,_napt.c and ip4_napt.h.

It took me a long time but I finally figured out how to run the current set of lwip unit tests in WSL2. I found and fixed an ISO C90 error in dhcp.c and a segmentation fault error in test_ip4_route.c so I uploaded those as well. After some minor updates my three files pass the lwip unit tests with no errors. The test results are shown below. I believe I've satisfied your change request.

Let me know if you need anything else.

Attempt to build and use lwip unit tests on original files:

sudo apt install -y check cd ~/esp git clone --recursive https://git.savannah.gnu.org/git/lwip/lwip-contrib.git

Copy lwip directory structure (lwip/src,lwip/test,etc) to ~/esp

cd ~/esp/esp-idf/components/lwip tar cvf lwip.tar lwip mv lwip.tar ~/esp cd ~/esp tar xvf lwip.tar rm lwip.tar

Build and run default lwip tests

cd ~/esp/lwip-contrib/ports/unix/check/ export LWIPDIR=../../../../lwip/src && export CK_DEFAULT_TIMEOUT=200 export EXTRA_CFLAGS="" && export CC="cc $EXTRA_CFLAGS" && export CCDEP=cc make -j 4 check

First time failed because of error: lwip/src/core/ipv4/dhcp.c:374:3 error: ISO C90 does not support ‘for’ loop initial declarations had to edit dhcp.c and move declaration of n out of the for loop to the top of the block

make -j 4 check

Results: 100%: Checks: 124, Failures: 0, Errors: 0 Retest with IP_FORWARD enabled

make clean export EXTRA_CFLAGS="-DIP_FORWARD=1" && export CC="cc $EXTRA_CFLAGS" make -j 4 check

Results: 100%: Checks: 124, Failures: 0, Errors: 0 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

Failed with error: lwip/src/../test/unit/core/test_ip4_route.c:502:3: error: ISO C90 does not support ‘for’ loop initial declarations had to edit test_ip4_route.c and move declaration of i out of the for loop to the top of the block Failed with error: lwip/src/core/ipv4/ip4_napt.c:486:3: error: ISO C90 does not support ‘for’ loop initial declarations had to edit ip4_napt.c and move declaration of i out of the for loop to the top of the block

make -j 4 check

Results: 100%: Checks: 128, Failures: 0, Errors: 0 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

Results: 100%: Checks: 5, Failures: 0, Errors: 0

Now run unit tests using my three updated files...

cd ~/esp/lwip/src/core/ipv4 cp /mnt/c/tmp/lwip_modified/.c . cd ~/esp/lwip/src/include/lwip cp /mnt/c/tmp/lwip_modified/.h .

cd ~/esp/lwip-contrib/ports/unix/check/ export LWIPDIR=../../../../lwip/src && export CK_DEFAULT_TIMEOUT=200 export EXTRA_CFLAGS="" && export CC="cc $EXTRA_CFLAGS" && export CCDEP=cc make clean make -j 4 check

First time failed because of error: lwip/src/core/ipv4/ip4.c:641:3: error: ISO C90 forbids mixed declarations and code had to edit ip4.c and move LWIP_DEBUGF statement after declarations

make -j 4 check

Results: 100%: Checks: 124, Failures: 0, Errors: 0 Retest with IP_FORWARD enabled

make clean export EXTRA_CFLAGS="-DIP_FORWARD=1" && export CC="cc $EXTRA_CFLAGS" make -j 4 check

Results: 100%: Checks: 124, Failures: 0, Errors: 0 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

Results: 100%: Checks: 128, Failures: 0, Errors: 0 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

Failed with error: lwip/src/../test/unit/core/test_ip4_route.c:395:E:IP4_ROUTE:test_ip4_route_netif_napt_udp:0: (after this point) Received signal 11 (Segmentation fault) Had to edit test_ip4_route.c and move the disabling of napt on ap netif before the netif is removed

make -j 4 check

Results: 100%: Checks: 5, Failures: 0, Errors: 0

[david-cermak]

@mtnbkr88 Thanks for the updates and fixing the issues!

The main trouble lies in here:

Failed with error: Received signal 11 (Segmentation fault) Had to edit test_ip4_route.c and move the disabling of napt on ap netif before the netif is removed

So if we accepted this patch, all other applications that use napt (and initialize it the same way as in the test) would start failing and users would have to update the initialization.

Thanks for fixing the for loops, too (the reason our current pipeline passes is that we use STABLE-2_1_0_RELEASE tag from the contrib repo, not the main branch), but we have to fix this anyway.

Another thing is that we would like to accept this PR https://github.com/espressif/esp-lwip/pull/43 first, as it actually fixes the most common NAPT usecases and might cause some merge conflict with your changes.

[mtnbkr88]

@david-cermak

I put back the original test_ip4_route.c file with the exception of correcting the ISO C90 error (int i declared in for loop). I found and fixed an error in a debug statement in ip_napt_enable function in ip_napt.c which fixed the segmentation fault issue. All tests pass with no errors. I've uploaded the latest good files so I consider my updates ready to be merged. Bad timing for me I guess since you want to merge PR #43 first. Please let me know when this is done and where I can clone it from. I'm willing to redo the work to add my local port forwarding updates to it.

[rojer]

i will address comments and update my PR shortly

[david-cermak]

@mtnbkr88 Please note that https://github.com/espressif/esp-lwip/pull/43 has been merged, so you can rebase and update your changes. Thanks! (need to still process some followup fixes, but very minor ones, could be done simultaneously with your PR)

[mtnbkr88]

@david-cermak Thanks for letting me know. I'll get on it and post here when my changes are ready for your review in this new baseline.

[mtnbkr88]

@david-cermak I added my changes to enable port forwarding onto the same/local netif to ip4.c, ip4_napt.c and ip4_napt.h in the most recent lwip. I ran all the unit tests you had me run before and all passed with no errors. I uploaded my changed. I hope find them and all looks good. Let me know if you need more. Thanks.

[david-cermak]

@mtnbkr88 Could you please

• rebase your changes against the lastest 2.1.2-esp ?
• cleanup the history and make it only one commit on top of the default branch?

The reason is that it's very hard to review and accept the changes, and we're not allowed to accept non-fast-forward merges, and (most importantly) there're many many conflicts.

Note, the the default branch has been updated today and we're not expecting any update in the near future.

[mtnbkr88]

@david-cermak I just checked. According to the history on the default branch of espressif/esp-lwip 2.1.2-esp was last updated by you on April 25, 2022. I'm not finding any new default updated as of today. Should I start with the April 25 baseline?

[david-cermak]

Should I start with the April 25 baseline?

Yes, please. The head of our default branch is (as of today) this commit: 475d658a47b63223a50fd61f4032476e99d0885a (it was authored in April, but committed a bit later and pushed today)

[mtnbkr88]

@david-cermak I've completed and tested my updates to the most recent default lwip 2.1.2 branch. Unfortunately, I can't figure out how to reset mtnbkr88:lwip-with-port-forwarding-on-same-netif to the same as the current default lwip 2.1.2 so I can put my changes as one commit on the top of the default branch as you requested. Please tell me how to do this.