[PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[Changli Gao]

fix the wrong checksum when addr isn't in old_addr/mask

For TCP and UDP packets, when addr isn't in old_addr/mask we don't do SNAT or
DNAT, and we should not update layer 4 checksum.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 net/sched/act_nat.c |    4 ++++
 1 file changed, 4 insertions(+)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index d885ba3..5709494 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -159,6 +159,9 @@ static int tcf_nat(struct sk_buff *skb, struct tc_action *a,
 			iph->daddr = new_addr;
 
 		csum_replace4(&iph->check, addr, new_addr);
+	} else if ((iph->frag_off & htons(IP_OFFSET)) ||
+		   iph->protocol != IPPROTO_ICMP) {
+		goto out;
 	}
 
 	ihl = iph->ihl * 4;
@@ -247,6 +250,7 @@ static int tcf_nat(struct sk_buff *skb, struct tc_action *a,
 		break;
 	}
 
+out:
 	return action;
 
 drop:

Re: [PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[jamal]

Copying Herbert, taking linux-kernel off...

On Sun, 2010-05-30 at 08:26 +0800, Changli Gao wrote:
> fix the wrong checksum when addr isn't in old_addr/mask
> 
> For TCP and UDP packets, when addr isn't in old_addr/mask we don't do SNAT or
> DNAT, and we should not update layer 4 checksum.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
>  net/sched/act_nat.c |    4 ++++
>  1 file changed, 4 insertions(+)
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index d885ba3..5709494 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -159,6 +159,9 @@ static int tcf_nat(struct sk_buff *skb, struct tc_action *a,
>  			iph->daddr = new_addr;
>  
>  		csum_replace4(&iph->check, addr, new_addr);
> +	} else if ((iph->frag_off & htons(IP_OFFSET)) ||
> +		   iph->protocol != IPPROTO_ICMP) {
> +		goto out;
>  	}
>  
>  	ihl = iph->ihl * 4;
> @@ -247,6 +250,7 @@ static int tcf_nat(struct sk_buff *skb, struct tc_action *a,
>  		break;
>  	}
>  
> +out:
>  	return action;
>  
>  drop:

Re: [PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[Herbert Xu]

On Sun, May 30, 2010 at 08:43:41AM -0400, jamal wrote:
> 
> Copying Herbert, taking linux-kernel off...

Thanks Jamal.

> On Sun, 2010-05-30 at 08:26 +0800, Changli Gao wrote:
> > fix the wrong checksum when addr isn't in old_addr/mask
> > 
> > For TCP and UDP packets, when addr isn't in old_addr/mask we don't do SNAT or
> > DNAT, and we should not update layer 4 checksum.
> > 
> > Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> > ----
> >  net/sched/act_nat.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> > index d885ba3..5709494 100644
> > --- a/net/sched/act_nat.c
> > +++ b/net/sched/act_nat.c
> > @@ -159,6 +159,9 @@ static int tcf_nat(struct sk_buff *skb, struct tc_action *a,
> >  			iph->daddr = new_addr;
> >  
> >  		csum_replace4(&iph->check, addr, new_addr);
> > +	} else if ((iph->frag_off & htons(IP_OFFSET)) ||
> > +		   iph->protocol != IPPROTO_ICMP) {
> > +		goto out;
> >  	}

Yes the patch is correct.

However, the fact that you need this patch means that your act_nat
setup isn't perfect.  Ideally all the unNATed packets should be
filtered out before you hit act_nat.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[Changli Gao]

On Sun, May 30, 2010 at 8:58 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sun, May 30, 2010 at 08:43:41AM -0400, jamal wrote:
>>
>> Copying Herbert, taking linux-kernel off...
>
> Thanks Jamal.
>
>> On Sun, 2010-05-30 at 08:26 +0800, Changli Gao wrote:
>> > fix the wrong checksum when addr isn't in old_addr/mask
>> >
>> > For TCP and UDP packets, when addr isn't in old_addr/mask we don't do SNAT or
>> > DNAT, and we should not update layer 4 checksum.
>> >
>> > Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> > ----
>> >  net/sched/act_nat.c |    4 ++++
>> >  1 file changed, 4 insertions(+)
>> > diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
>> > index d885ba3..5709494 100644
>> > --- a/net/sched/act_nat.c
>> > +++ b/net/sched/act_nat.c
>> > @@ -159,6 +159,9 @@ static int tcf_nat(struct sk_buff *skb, struct tc_action *a,
>> >                     iph->daddr = new_addr;
>> >
>> >             csum_replace4(&iph->check, addr, new_addr);
>> > +   } else if ((iph->frag_off & htons(IP_OFFSET)) ||
>> > +              iph->protocol != IPPROTO_ICMP) {
>> > +           goto out;
>> >     }
>
> Yes the patch is correct.
>
> However, the fact that you need this patch means that your act_nat
> setup isn't perfect.  Ideally all the unNATed packets should be
> filtered out before you hit act_nat.

Thinking about this topologic:

client -> DNAT -> router -> server.

DNAT is used to map a public IP to server's private IP. If a
DEST_UNREACH ICMP packet is sent out by router, in order to handle
this ICMP packet correctly, I have to pass it to act_nat.c. How can I
filter out the other packets? By inspecting the inner IP destination
address of this ICMP packet? Maybe I can use u32 with complicate
parameters.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[Changli Gao]

On Sun, May 30, 2010 at 9:33 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> On Sun, May 30, 2010 at 8:58 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>>
>> Yes the patch is correct.
>>
>> However, the fact that you need this patch means that your act_nat
>> setup isn't perfect.  Ideally all the unNATed packets should be
>> filtered out before you hit act_nat.
>
> Thinking about this topologic:
>
> client -> DNAT -> router -> server.
>
> DNAT is used to map a public IP to server's private IP. If a
> DEST_UNREACH ICMP packet is sent out by router, in order to handle
> this ICMP packet correctly, I have to pass it to act_nat.c. How can I
> filter out the other packets? By inspecting the inner IP destination
> address of this ICMP packet? Maybe I can use u32 with complicate
> parameters.
>

Oh, I can pass all the ICMP packets, and the packets to the public IP
and the packets from the private IP.

-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[Herbert Xu]

On Sun, May 30, 2010 at 09:33:22PM +0800, Changli Gao wrote:
>
> Thinking about this topologic:
> 
> client -> DNAT -> router -> server.
> 
> DNAT is used to map a public IP to server's private IP. If a
> DEST_UNREACH ICMP packet is sent out by router, in order to handle
> this ICMP packet correctly, I have to pass it to act_nat.c. How can I
> filter out the other packets? By inspecting the inner IP destination
> address of this ICMP packet? Maybe I can use u32 with complicate
> parameters.

You should filter out all the non-ICMP packets, just like you
do here.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] act_nat: fix the wrong checksum when addr isn't in old_addr/mask

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 30 May 2010 22:58:11 +1000

> Yes the patch is correct.
> 
> However, the fact that you need this patch means that your act_nat
> setup isn't perfect.  Ideally all the unNATed packets should be
> filtered out before you hit act_nat.

Regardless of whether said configuration is optimal, we should
handle this case correctly.

So I have applied this patch.