The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by NAT or MASQUERADE.
However, the implementation only considered the IP_CT_DIR_ORIGINAL case
and incorrectly applies the same logic to packets in reply direction.
Therefore, an SNAT rule in the original direction causes icmp_ndo_send to
translate the source IP of reply-direction packets, even though no
translation is required. The source address is translated to the sender
address of the original direction, because the original tuple's source
address is used.
On the other hand, icmp_ndo_send incorrectly misses translating the
source address of packets in reply-direction, leading to incorrect rate
limiting. The generated ICMP error is translated by netfilter at a later
stage, therefore the ICMP error is sent correctly.
Fix this by translating the address based on the connection direction:
- CT_DIR_ORIGINAL: Use the original tuple's source address
(unchanged from current behavior)
- CT_DIR_REPLY: Use the reply tuple's source address
(fixing the incorrect translation)
Fixes: 0b41713b6066 ("icmp: introduce helper for nat'd source address in network device context")
Signed-off-by: Fabian Bläse <redacted>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
---
net/ipv4/icmp.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by NAT or MASQUERADE.
However, the implementation only considered the IP_CT_DIR_ORIGINAL case
and incorrectly applies the same logic to packets in reply direction.
Therefore, an SNAT rule in the original direction causes icmp_ndo_send to
translate the source IP of reply-direction packets, even though no
translation is required. The source address is translated to the sender
address of the original direction, because the original tuple's source
address is used.
On the other hand, icmp_ndo_send incorrectly misses translating the
source address of packets in reply-direction, leading to incorrect rate
limiting. The generated ICMP error is translated by netfilter at a later
stage, therefore the ICMP error is sent correctly.
Fix this by translating the address based on the connection direction:
- CT_DIR_ORIGINAL: Use the original tuple's source address
(unchanged from current behavior)
- CT_DIR_REPLY: Use the reply tuple's source address
(fixing the incorrect translation)
Fixes: 0b41713b6066 ("icmp: introduce helper for nat'd source address in network device context")
Signed-off-by: Fabian Bläse <redacted>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v1->v2:
- Implement fix for ICMPv6 as well
---
net/ipv4/icmp.c | 14 ++++++++++++--
net/ipv6/ip6_icmp.c | 14 ++++++++++++--
2 files changed, 24 insertions(+), 4 deletions(-)
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by NAT or MASQUERADE.
However, the implementation only considered the IP_CT_DIR_ORIGINAL case
and incorrectly applies the same logic to packets in reply direction.
Therefore, an SNAT rule in the original direction causes icmp_ndo_send to
translate the source IP of reply-direction packets, even though no
translation is required. The source address is translated to the sender
address of the original direction, because the original tuple's source
address is used.
On the other hand, icmp_ndo_send incorrectly misses translating the
source address of packets in reply-direction, leading to incorrect rate
limiting. The generated ICMP error is translated by netfilter at a later
stage, therefore the ICMP error is sent correctly.
Fix this by translating the address based on the connection direction:
- CT_DIR_ORIGINAL: Use the original tuple's source address
(unchanged from current behavior)
- CT_DIR_REPLY: Use the reply tuple's source address
(fixing the incorrect translation)
I don't understand this part. You talk about snat, yet you remove
the check for its absence here. Why?
If the connection isn't subject to snat, why to we need to mangle the
source address in the first place?
If you are worried about "dnat to", then please update the commit
message, which only mentions masquerade/snat.
... with ...
enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
ip_hdr(skb_in)->saddr = ct->tuplehash[dir].tuple.src.u3.ip;
... at least, thats what I gather from your commit message.
For original direction, there is no change compared to existing code (dir == 0).
For reply direction, saddr is replaced with the source of the reply tuple.
Without dnat, the reply tuple saddr == original tuple daddr.
With dnat, its the dnat targets' address (i.e., the real destination
the client is talking to).
Did I get anything wrong?
If the connection isn't subject to snat, why to we need to mangle the
source address in the first place?
It is not limited to SNAT/MASQUERADE.
DNAT also affects which source address should be used, depending on the packet
direction.
With DNAT, the *destination* of the original direction is changed.
In the reply direction, this becomes the *source* address.
So reply packets of a DNAT connection are effectively subject to source address
translation. If icmp_ndo_send doesn’t account for this, rate limiting breaks,
which is exactly the problem this function was meant to solve.
Don't understand this either. Why these checks?
AFAICS you can keep the original check in place, and then:
replace this
You are right: the code can be simplified. I'm not sure show this slipped through.
I will send an updated patch with this change — thanks for the suggestion.
However, the old check (IPS_SRC_NAT only) cannot be kept, because:
- Reply packets of a DNAT connection also need handling.
- Reply packets of a pure SNAT connection don’t need it, but replacing the
address is a no-op in that case (tuple == skb address).
To avoid unnecessary translations, I suggested the direction-specific checks.
Another option is to simplify them to:
if (!(ct->status & IPS_NAT_MASK)) { … }
This ensures we only ever touch connections with NAT, while keeping the code
straightforward.
Without dnat, the reply tuple saddr == original tuple daddr.
With dnat, its the dnat targets' address (i.e., the real destination
the client is talking to).
Yes, exactly.
If you are worried about "dnat to", then please update the commit
message, which only mentions masquerade/snat.
Correct — the change not only fixes SNAT-in-reply handling, but also adds
proper handling for DNAT in the reply direction, which was missing entirely.
I will update the commit message to reflect this.
Best regards,
Fabian
To avoid unnecessary translations, I suggested the direction-specific checks.
Another option is to simplify them to:
if (!(ct->status & IPS_NAT_MASK)) { … }
Yes, you can update the test from
if (!ct || !(ct->status & IPS_SRC_NAT)) {
to
if (!ct || !(ct->status & IPS_NAT_MASK)) {
Not related to your change:
I suspect there is a very small risk that kcsan could report a data
race here, given ct->status can be modified on other CPU.
But maybe, while at it, replace this with
READ_ONCE(ct->status) & ...
Correct — the change not only fixes SNAT-in-reply handling, but also adds
proper handling for DNAT in the reply direction, which was missing entirely.
I will update the commit message to reflect this.
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by SNAT.
However, the original implementation only considers the
IP_CT_DIR_ORIGINAL direction for SNAT and always replaced the packet's
source address with that of the original-direction tuple. This causes
two problems:
1. For SNAT:
Reply-direction packets were incorrectly translated using the source
address of the CT original direction, even though no translation is
required.
2. For DNAT:
Reply-direction packets were not handled at all. In DNAT, the original
direction's destination is translated. Therefore, in the reply
direction the source address must be set to the reply-direction
source, so rate limiting works as intended.
Fix this by using the connection direction to select the correct tuple
for source address translation, and adjust the pre-checks to handle
reply-direction packets in case of DNAT.
Additionally, wrap the `ct->status` access in READ_ONCE(). This avoids
possible KCSAN reports about concurrent updates to `ct->status`.
Fixes: 0b41713b6066 ("icmp: introduce helper for nat'd source address in network device context")
Signed-off-by: Fabian Bläse <redacted>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Florian Westphal <fw@strlen.de>
---
Changes v1->v2:
- Implement fix for ICMPv6 as well
Changes v2->v3:
- Collapse conditional tuple selection into a single direction lookup [Florian]
- Always apply source address translation if IPS_NAT_MASK is set [Florian]
- Wrap ct->status in READ_ONCE()
- Add a clearer explanation of the behaviour change for DNAT
---
net/ipv4/icmp.c | 6 ++++--
net/ipv6/ip6_icmp.c | 6 ++++--
2 files changed, 8 insertions(+), 4 deletions(-)
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2025-08-28 12:00:30
On Thu, Aug 28, 2025 at 11:14:35AM +0200, Fabian Bläse wrote:
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by SNAT.
However, the original implementation only considers the
IP_CT_DIR_ORIGINAL direction for SNAT and always replaced the packet's
source address with that of the original-direction tuple. This causes
two problems:
1. For SNAT:
Reply-direction packets were incorrectly translated using the source
address of the CT original direction, even though no translation is
required.
2. For DNAT:
Reply-direction packets were not handled at all. In DNAT, the original
direction's destination is translated. Therefore, in the reply
direction the source address must be set to the reply-direction
source, so rate limiting works as intended.
Fix this by using the connection direction to select the correct tuple
for source address translation, and adjust the pre-checks to handle
reply-direction packets in case of DNAT.
Additionally, wrap the `ct->status` access in READ_ONCE(). This avoids
possible KCSAN reports about concurrent updates to `ct->status`.
I think such concurrent update cannot not happen, NAT bits are only
set for the first packet of a connection, which sets up the nat
configuration, so READ_ONCE() can go away.
Florian?
quoted hunk
Fixes: 0b41713b6066 ("icmp: introduce helper for nat'd source address in network device context")
Signed-off-by: Fabian Bläse <redacted>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Florian Westphal <fw@strlen.de>
---
Changes v1->v2:
- Implement fix for ICMPv6 as well
Changes v2->v3:
- Collapse conditional tuple selection into a single direction lookup [Florian]
- Always apply source address translation if IPS_NAT_MASK is set [Florian]
- Wrap ct->status in READ_ONCE()
- Add a clearer explanation of the behaviour change for DNAT
---
net/ipv4/icmp.c | 6 ++++--
net/ipv6/ip6_icmp.c | 6 ++++--
2 files changed, 8 insertions(+), 4 deletions(-)
On Thu, Aug 28, 2025 at 11:14:35AM +0200, Fabian Bläse wrote:
quoted
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by SNAT.
However, the original implementation only considers the
IP_CT_DIR_ORIGINAL direction for SNAT and always replaced the packet's
source address with that of the original-direction tuple. This causes
two problems:
1. For SNAT:
Reply-direction packets were incorrectly translated using the source
address of the CT original direction, even though no translation is
required.
2. For DNAT:
Reply-direction packets were not handled at all. In DNAT, the original
direction's destination is translated. Therefore, in the reply
direction the source address must be set to the reply-direction
source, so rate limiting works as intended.
Fix this by using the connection direction to select the correct tuple
for source address translation, and adjust the pre-checks to handle
reply-direction packets in case of DNAT.
Additionally, wrap the `ct->status` access in READ_ONCE(). This avoids
possible KCSAN reports about concurrent updates to `ct->status`.
I think such concurrent update cannot not happen, NAT bits are only
set for the first packet of a connection, which sets up the nat
configuration, so READ_ONCE() can go away.
Yes, the NAT bits stay in place but not other flags in ->status, e.g.
DYING, ASSURED, etc.
So I believe its needed, concurrent update of ->status is possible and
KCSAN would warn. Other spots either use READ_ONCE or use test_bit().
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2025-08-28 12:33:28
On Thu, Aug 28, 2025 at 02:15:53PM +0200, Florian Westphal wrote:
Pablo Neira Ayuso [off-list ref] wrote:
quoted
On Thu, Aug 28, 2025 at 11:14:35AM +0200, Fabian Bläse wrote:
quoted
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by SNAT.
However, the original implementation only considers the
IP_CT_DIR_ORIGINAL direction for SNAT and always replaced the packet's
source address with that of the original-direction tuple. This causes
two problems:
1. For SNAT:
Reply-direction packets were incorrectly translated using the source
address of the CT original direction, even though no translation is
required.
2. For DNAT:
Reply-direction packets were not handled at all. In DNAT, the original
direction's destination is translated. Therefore, in the reply
direction the source address must be set to the reply-direction
source, so rate limiting works as intended.
Fix this by using the connection direction to select the correct tuple
for source address translation, and adjust the pre-checks to handle
reply-direction packets in case of DNAT.
Additionally, wrap the `ct->status` access in READ_ONCE(). This avoids
possible KCSAN reports about concurrent updates to `ct->status`.
I think such concurrent update cannot not happen, NAT bits are only
set for the first packet of a connection, which sets up the nat
configuration, so READ_ONCE() can go away.
Yes, the NAT bits stay in place but not other flags in ->status, e.g.
DYING, ASSURED, etc.
So I believe its needed, concurrent update of ->status is possible and
KCSAN would warn. Other spots either use READ_ONCE or use test_bit().
There are a more checks for ct->status & NAT_MASK in the tree that I
can see, if you are correct, then maybe a new helper function to check
for NAT_MASK is needed.
Anyway, as for this patch, READ_ONCE should not harm.
So I believe its needed, concurrent update of ->status is possible and
KCSAN would warn. Other spots either use READ_ONCE or use test_bit().
There are a more checks for ct->status & NAT_MASK in the tree that I
can see, if you are correct, then maybe a new helper function to check
for NAT_MASK is needed.
I think it would make sense to add a helper, yes.
Independent of this patch of course.
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by SNAT.
However, the original implementation only considers the
IP_CT_DIR_ORIGINAL direction for SNAT and always replaced the packet's
source address with that of the original-direction tuple. This causes
two problems:
1. For SNAT:
Reply-direction packets were incorrectly translated using the source
address of the CT original direction, even though no translation is
required.
2. For DNAT:
Reply-direction packets were not handled at all. In DNAT, the original
direction's destination is translated. Therefore, in the reply
direction the source address must be set to the reply-direction
source, so rate limiting works as intended.
Fix this by using the connection direction to select the correct tuple
for source address translation, and adjust the pre-checks to handle
reply-direction packets in case of DNAT.
Additionally, wrap the `ct->status` access in READ_ONCE(). This avoids
possible KCSAN reports about concurrent updates to `ct->status`.
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski [off-list ref]:
On Thu, 28 Aug 2025 11:14:35 +0200 you wrote:
The icmp_ndo_send function was originally introduced to ensure proper
rate limiting when icmp_send is called by a network device driver,
where the packet's source address may have already been transformed
by SNAT.
However, the original implementation only considers the
IP_CT_DIR_ORIGINAL direction for SNAT and always replaced the packet's
source address with that of the original-direction tuple. This causes
two problems:
[...]