net/nat: Let ICMP echo id zero be a valid NAT id.

[ankohuu]

Summary

ICMP echo identifiers are 16-bit values defined by RFC 792, and zero is
a valid identifier value. NAT currently uses nat_port_select() return
value zero to mean port selection failure, which makes ICMP echo id zero
ambiguous with failure.

https://datatracker.ietf.org/doc/html/rfc792

Identifier

    If code = 0, an identifier to aid in matching echos and replies,
    may be zero.

Change nat_port_select() to return a status code and pass the selected
external port or ICMP identifier through an output parameter. This lets
NAT preserve and create entries for ICMP echo id zero while keeping real
selection failures separate.

Impact

bugfix

Testing

The issue was identified while I try to set up a NAT environment using the NuttX simulator. The test setup consists of a sim instance with two network interfaces, both backed by TAP devices.

Nuttx SIMulator:

NuttShell (NSH) NuttX-12.13.0
nsh> ifconfig
eth0    Link encap:Ethernet HWaddr 42:b3:0d:d6:82:81 at RUNNING mtu 1500
        inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0

eth1    Link encap:Ethernet HWaddr 42:8a:90:eb:ef:03 at RUNNING mtu 1500
        inet addr:10.0.1.2 DRaddr:10.0.1.1 Mask:255.255.255.0

nsh> iptables -t nat -L
Chain POSTROUTING (policy ACCEPT)
target        prot  idev  odev  source              destination
MASQUERADE    all   any   eth1  anywhere            anywhere           MASQUERADE

Linux:

 ip addr show dev tap0
130: tap0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UNKNOWN group default qlen 1000
    link/ether c2:ad:53:d1:33:d4 brd ff:ff:ff:ff:ff:ff
    inet 10.0.0.1/24 scope global tap0
       valid_lft forever preferred_lft forever
    inet6 fe80::4492:20ff:fe0d:b2cf/64 scope link
       valid_lft forever preferred_lft forever

sudo ip netns exec lan ip addr show dev tap1
131: tap1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UNKNOWN group default qlen 1000
    link/ether c2:dc:48:70:0c:7b brd ff:ff:ff:ff:ff:ff
    inet 10.0.1.1/24 scope global tap1
       valid_lft forever preferred_lft forever
    inet6 fe80::c0dc:48ff:fe70:c7b/64 scope link
       valid_lft forever preferred_lft forever

With the current port selection algorithm, a local ID of 0 is the most likely to result in an external ID of 0.

Testing logs before change:

ping -e 1 -c 5 -W 2 10.0.1.1
PING 10.0.1.1 (10.0.1.1) 56(84) bytes of data.
64 bytes from 10.0.1.1: icmp_seq=1 ttl=63 time=7.36 ms
64 bytes from 10.0.1.1: icmp_seq=2 ttl=63 time=5.73 ms
64 bytes from 10.0.1.1: icmp_seq=3 ttl=63 time=3.75 ms
64 bytes from 10.0.1.1: icmp_seq=4 ttl=63 time=1.93 ms
64 bytes from 10.0.1.1: icmp_seq=5 ttl=63 time=0.708 ms

--- 10.0.1.1 ping statistics ---
5 packets transmitted, 5 received, 0% packet loss, time 4007ms
rtt min/avg/max/mdev = 0.708/3.896/7.361/2.426 ms

ping -e 0 -c 5 -W 2 10.0.1.1
PING 10.0.1.1 (10.0.1.1) 56(84) bytes of data.

--- 10.0.1.1 ping statistics ---
5 packets transmitted, 0 received, 100% packet loss, time 4080ms

captured:
sudo ip netns exec lan tshark -i tap1 icmp
Running as user "root" and group "root". This could be dangerous.
Capturing on 'tap1'
  1 0.000000000     10.0.1.2 → 10.0.1.1     ICMP 98 Echo (ping) request  id=0x0001, seq=1/256, ttl=63
  2 0.000015900     10.0.1.1 → 10.0.1.2     ICMP 98 Echo (ping) reply    id=0x0001, seq=1/256, ttl=64 (request in 1)
  3 1.000082339     10.0.1.2 → 10.0.1.1     ICMP 98 Echo (ping) request  id=0x0001, seq=2/512, ttl=63
  4 1.000106110     10.0.1.1 → 10.0.1.2     ICMP 98 Echo (ping) reply    id=0x0001, seq=2/512, ttl=64 (request in 3)
  5 2.000042143     10.0.1.2 → 10.0.1.1     ICMP 98 Echo (ping) request  id=0x0001, seq=3/768, ttl=63
  6 2.000064222     10.0.1.1 → 10.0.1.2     ICMP 98 Echo (ping) reply    id=0x0001, seq=3/768, ttl=64 (request in 5)
  7 3.000125124     10.0.1.2 → 10.0.1.1     ICMP 98 Echo (ping) request  id=0x0001, seq=4/1024, ttl=63
  8 3.000149002     10.0.1.1 → 10.0.1.2     ICMP 98 Echo (ping) reply    id=0x0001, seq=4/1024, ttl=64 (request in 7)
  9 4.000075849     10.0.1.2 → 10.0.1.1     ICMP 98 Echo (ping) request  id=0x0001, seq=5/1280, ttl=63
 10 4.000099183     10.0.1.1 → 10.0.1.2     ICMP 98 Echo (ping) reply    id=0x0001, seq=5/1280, ttl=64 (request in 9)

Testing logs after change:

ping -e 0 -c 5 -W 2 10.0.1.1
PING 10.0.1.1 (10.0.1.1) 56(84) bytes of data.
64 bytes from 10.0.1.1: icmp_seq=1 ttl=63 time=9.04 ms
64 bytes from 10.0.1.1: icmp_seq=2 ttl=63 time=7.94 ms
64 bytes from 10.0.1.1: icmp_seq=3 ttl=63 time=6.85 ms
64 bytes from 10.0.1.1: icmp_seq=4 ttl=63 time=4.95 ms
64 bytes from 10.0.1.1: icmp_seq=5 ttl=63 time=3.95 ms

[acassis]

@ankohuu Nice finding!!!
Could you please update the Documentation https://nuttx.apache.org/docs/latest/components/net/nat.html to include your test?

It seems they have a similar test there, but yours looks a little different, and there are only vague examples without further explanation about the tests.

It should be nice to give more details for beginners to understand the whole process.

[ankohuu]

Hi @acassis ,

@ankohuu Nice finding!!! Could you please update the Documentation https://nuttx.apache.org/docs/latest/components/net/nat.html to include your test?

It seems they have a similar test there, but yours looks a little different

• My test is basically the same as the below one in the documentation. The only difference is that I used ping to test ICMP, while the documented test uses iperf to test TCP.
• With the same topology, a single host if0 -> if1 NAT case should be enough to demonstrate the behavior? Of course, I am also happy to document all the cases if that is preferred.

Do anything in the LAN namespace will go through NAT

    # Host side
    iperf -B 10.0.1.1 -s -i 1
    # LAN side
    sudo ip netns exec LAN iperf -B 10.0.10.1 -c 10.0.1.1 -i 1

It seems they have a similar test there, but yours looks a little different, and there are only vague examples without further explanation about the tests.

It should be nice to give more details for beginners to understand the whole process.

I also think the current documentation already has a fair amount of tech detail, but it may be made clearer. Can I handle this rst update in a separate patch?

[acassis]

Hi @acassis ,

@ankohuu Nice finding!!! Could you please update the Documentation https://nuttx.apache.org/docs/latest/components/net/nat.html to include your test?
It seems they have a similar test there, but yours looks a little different

* My test is basically the same as the below one in the documentation. The only difference is that I used `ping` to test `ICMP`, while the documented test uses `iperf` to test `TCP`.

* With the same topology, a single host if0 -> if1 NAT case should be enough to demonstrate the behavior? Of course, I am also happy to document all the cases if that is preferred.

Do anything in the LAN namespace will go through NAT

    # Host side
    iperf -B 10.0.1.1 -s -i 1
    # LAN side
    sudo ip netns exec LAN iperf -B 10.0.10.1 -c 10.0.1.1 -i 1

It seems they have a similar test there, but yours looks a little different, and there are only vague examples without further explanation about the tests.
It should be nice to give more details for beginners to understand the whole process.

I also think the current documentation already has a fair amount of tech detail, but it may be made clearer. Can I handle this rst update in a separate patch?

Normally we suggest to include the Documentation in the same PR, history shows that mostly the time people forget about it after the main PR is merged, just an example here: #17033 (comment)

But I will trust that you will do it and I will remove the Change Request.

[ankohuu]

@acassis

Big thank you for your help and for all your contributions to NuttX.

In fact, I was only trying to express two points:

• It seems that adding another ping example may not be necessary, since there is already a similar one.
• If the goal is to make the NAT rst document a bit clearer, that seems like a separate PR that should be handled independently.

That is why I planned to proceed in these two steps. From my perspective, the above is not a request that should delay the document changes that are already needed for this PR.

P.S. I do have some practical difficulty in updating the documentation immediately. I need a bit of time to read the current documents carefully and understand the existing style and conventions I should follow. :)

[acassis]

@acassis

Big thank you for your help and for all your contributions to NuttX.

In fact, I was only trying to express two points:

* It seems that adding another `ping` example may not be **necessary**, since there is already a similar one.

* If the goal is to make the NAT rst document a bit clearer, that seems like a **separate** PR that should be handled independently.

That is why I planned to proceed in these two steps. From my perspective, the above is not a request that should delay the document changes that are already needed for this PR.

P.S. I do have some practical difficulty in updating the documentation immediately. I need a bit of time to read the current documents carefully and understand the existing style and conventions I should follow. :)

Hi @ankohuu perfect! If you think your example is already covered there, then I think we just need to make the document more clearer and explain to the details to let new users to get a better understanding. Having the commands there is already a good thing, for some commands and features we don't have anything.

I think Documentation is a big gap of the NuttX project