Re: [PATCH v2] icmp: fix icmp_ndo_send address translation for reply direction

[Florian Westphal <fw@strlen.de>]

Fabian Bläse <fabian@blaese.de> 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 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)

>  	ct = nf_ct_get(skb_in, &ctinfo);
> -	if (!ct || !(ct->status & IPS_SRC_NAT)) {
> +	if (!ct) {
> +		__icmp_send(skb_in, type, code, info, &opts);
> +		return;
> +	}

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.

> +	if ( !(ct->status & IPS_SRC_NAT && CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL)
> +		&& !(ct->status & IPS_DST_NAT && CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY)) {
>  		__icmp_send(skb_in, type, code, info, &opts);
>  		return;
>  	}

Don't understand this either.  Why these checks?
AFAICS you can keep the original check in place, and then:

replace this
>  	orig_ip = ip_hdr(skb_in)->saddr;
> -	ip_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.ip;

... 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?