[PATCH net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[PATCH net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[David Shwatrz]

[-- Attachment #1: Type: text/plain, Size: 133 bytes --]

Hi,
 The patch removes unused member in xfrm_encap_tmpl.

Regards,
David Shwartz


Signed-off-by: David Shwartz <dshwatrz@gmail.com>

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 710 bytes --]

diff --git a/include/linux/xfrm.h b/include/linux/xfrm.h
index b971e38..7312707 100644
--- a/include/linux/xfrm.h
+++ b/include/linux/xfrm.h
@@ -235,7 +235,6 @@ struct xfrm_encap_tmpl {
 	__u16		encap_type;
 	__be16		encap_sport;
 	__be16		encap_dport;
-	xfrm_address_t	encap_oa;
 };
 
 /* AEVENT flags  */
diff --git a/net/key/af_key.c b/net/key/af_key.c
index d87c22d..4851deb 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1245,7 +1245,6 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 				ext_hdrs[SADB_X_EXT_NAT_T_DPORT-1];
 			natt->encap_dport = n_port->sadb_x_nat_t_port_port;
 		}
-		memset(&natt->encap_oa, 0, sizeof(natt->encap_oa));
 	}
 
 	err = xfrm_init_state(x);

Re: [net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[Timo Teräs]

On 01/-10/-28163 09:59 PM, David Shwatrz wrote:
> Hi,
>  The patch removes unused member in xfrm_encap_tmpl.
> 
> Regards,
> David Shwartz
> 
> 
> Signed-off-by: David Shwartz <dshwatrz@gmail.com>
> 
> 
> diff --git a/include/linux/xfrm.h b/include/linux/xfrm.h
> index b971e38..7312707 100644
> --- a/include/linux/xfrm.h
> +++ b/include/linux/xfrm.h
> @@ -235,7 +235,6 @@ struct xfrm_encap_tmpl {
>  	__u16		encap_type;
>  	__be16		encap_sport;
>  	__be16		encap_dport;
> -	xfrm_address_t	encap_oa;
>  };
>  
>  /* AEVENT flags  */

struct xfrm_encap_tmpl is exposed to userland via netlink. This would
break ABI.

Re: [net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Mon, 29 Nov 2010 20:07:46 +0200

> On 01/-10/-28163 09:59 PM, David Shwatrz wrote:
>> Hi,
>>  The patch removes unused member in xfrm_encap_tmpl.
>> 
>> Regards,
>> David Shwartz
>> 
>> 
>> Signed-off-by: David Shwartz <dshwatrz@gmail.com>
>> 
>> 
>> diff --git a/include/linux/xfrm.h b/include/linux/xfrm.h
>> index b971e38..7312707 100644
>> --- a/include/linux/xfrm.h
>> +++ b/include/linux/xfrm.h
>> @@ -235,7 +235,6 @@ struct xfrm_encap_tmpl {
>>  	__u16		encap_type;
>>  	__be16		encap_sport;
>>  	__be16		encap_dport;
>> -	xfrm_address_t	encap_oa;
>>  };
>>  
>>  /* AEVENT flags  */
> 
> struct xfrm_encap_tmpl is exposed to userland via netlink. This would
> break ABI.

RIght.

Re: [net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[David Shwatrz]

Hi,
Thanks, Timo.

But isn't something wrong here ?

According to RFC 3948:
...
3.1.2.  Transport Mode Decapsulation NAT Procedure

When a transport mode has been used to transmit packets, contained
   TCP or UDP headers will have incorrect checksums due to the change of
   parts of the IP header during transit.  This procedure defines how to
   fix these checksums
...
incrementally recompute the
       TCP/UDP checksum:

       *  Subtract the IP source address in the received packet from the
          checksum.
       *  Add the real IP source address received via IKE to the
          checksum (obtained from the NAT-OA)
...

So where do we pass the NAT-OA, received from IKE messages,  to this
checksum recalculation process, which should be done in the kernel
(layer 4 TCP/UDP I suppose) ?

Should'nt this process be done in the kernel ?

Isn't there something missing here ?

Rgs,
DS

2010/11/29 Timo Teräs <timo.teras@iki.fi>:
> On 01/-10/-28163 09:59 PM, David Shwatrz wrote:
>> Hi,
>>  The patch removes unused member in xfrm_encap_tmpl.
>>
>> Regards,
>> David Shwartz
>>
>>
>> Signed-off-by: David Shwartz <dshwatrz@gmail.com>
>>
>>
>> diff --git a/include/linux/xfrm.h b/include/linux/xfrm.h
>> index b971e38..7312707 100644
>> --- a/include/linux/xfrm.h
>> +++ b/include/linux/xfrm.h
>> @@ -235,7 +235,6 @@ struct xfrm_encap_tmpl {
>>       __u16           encap_type;
>>       __be16          encap_sport;
>>       __be16          encap_dport;
>> -     xfrm_address_t  encap_oa;
>>  };
>>
>>  /* AEVENT flags  */
>
> struct xfrm_encap_tmpl is exposed to userland via netlink. This would
> break ABI.
>

Re: [net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[Timo Teräs]

On 11/29/2010 09:15 PM, David Shwatrz wrote:
> But isn't something wrong here ?
> 
> According to RFC 3948:
> ...
> 3.1.2.  Transport Mode Decapsulation NAT Procedure
> 
> When a transport mode has been used to transmit packets, contained
>    TCP or UDP headers will have incorrect checksums due to the change of
>    parts of the IP header during transit.  This procedure defines how to
>    fix these checksums
> ...
> incrementally recompute the
>        TCP/UDP checksum:
> 
>        *  Subtract the IP source address in the received packet from the
>           checksum.
>        *  Add the real IP source address received via IKE to the
>           checksum (obtained from the NAT-OA)
> ...
> 
> So where do we pass the NAT-OA, received from IKE messages,  to this
> checksum recalculation process, which should be done in the kernel
> (layer 4 TCP/UDP I suppose) ?
> 
> Should'nt this process be done in the kernel ?
> 
> Isn't there something missing here ?

That's what the field was intended for. Also it's passed around by e.g.
'ip xfrm' command. The header change would break compilation of iproute2
too.

Alternatively the other RFCs say that the checksum can be just
recalculated. That's what the linux stack does: it throws the old
checksum away (ignored on local receive and recalculated on send /
forward paths). The ESP/AH packets are usually also configured to
contain a cryptographic hash, so the packet modifications are detected
even before trying to check the TCP/UDP checksum (making the check
redundant).

There's also probably some legacy reasons why the nat-oa field is useful
in kernel; and why the tcp/udp is not updated according the above
mentioned scheme. I guess doing that might speed up forwarding from one
tunnel to another where hardware checksum acceleration is not available;
maybe no one just had the time to implement it.

- Timo

Re: [net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[David Shwatrz]

Timo,
Thanks for your answer.

> Alternatively the other RFCs say that the checksum can be just
> recalculated. That's what the linux stack does: it throws the old
> checksum away (ignored on local receive and recalculated on send
> forward paths).

Sorry, something here seems to me still wrong; maybe I miss something.
We are talking about transport mode. Let's take TCP for example.
the packet is received by another peer. It is on port 4500 because of
NAT-T. Since it is ESP encrypted , it is it is decrypted. It reaches
the TCP layer. In TCP (as opposed to UDP), the checksum is mandatory.
But the checksum in that TCP header of the received
packet will not be correct, since the IP header of that packet is
**NOT** the original address ; the IP address was changed by the NAT.
The NAT  cannot change the TCP checksum since it is encrypted. So
wouldn't we have a checksum error in the case of TCP ?  It seems to me
that the purpose of NAT-OA was to pass the
original address, so that there will be no error in such a case.
But again, maybe I miss something, since I did not heard that
transport mode has any problems with NAT-T. Or maybe this was not
tested?


Rgs,
DS



2010/11/29 Timo Teräs <timo.teras@iki.fi>:
> On 11/29/2010 09:15 PM, David Shwatrz wrote:
>> But isn't something wrong here ?
>>
>> According to RFC 3948:
>> ...
>> 3.1.2.  Transport Mode Decapsulation NAT Procedure
>>
>> When a transport mode has been used to transmit packets, contained
>>    TCP or UDP headers will have incorrect checksums due to the change of
>>    parts of the IP header during transit.  This procedure defines how to
>>    fix these checksums
>> ...
>> incrementally recompute the
>>        TCP/UDP checksum:
>>
>>        *  Subtract the IP source address in the received packet from the
>>           checksum.
>>        *  Add the real IP source address received via IKE to the
>>           checksum (obtained from the NAT-OA)
>> ...
>>
>> So where do we pass the NAT-OA, received from IKE messages,  to this
>> checksum recalculation process, which should be done in the kernel
>> (layer 4 TCP/UDP I suppose) ?
>>
>> Should'nt this process be done in the kernel ?
>>
>> Isn't there something missing here ?
>
> That's what the field was intended for. Also it's passed around by e.g.
> 'ip xfrm' command. The header change would break compilation of iproute2
> too.
>
> Alternatively the other RFCs say that the checksum can be just
> recalculated. That's what the linux stack does: it throws the old
> checksum away (ignored on local receive and recalculated on send /
> forward paths). The ESP/AH packets are usually also configured to
> contain a cryptographic hash, so the packet modifications are detected
> even before trying to check the TCP/UDP checksum (making the check
> redundant).
>
> There's also probably some legacy reasons why the nat-oa field is useful
> in kernel; and why the tcp/udp is not updated according the above
> mentioned scheme. I guess doing that might speed up forwarding from one
> tunnel to another where hardware checksum acceleration is not available;
> maybe no one just had the time to implement it.
>
> - Timo
>

Re: [net-next-2.6] XFRM: remove unused member in xfrm_encap_tmpl.

[Timo Teräs]

On 11/29/2010 10:09 PM, David Shwatrz wrote:
> Timo,
> Thanks for your answer.
> 
>> Alternatively the other RFCs say that the checksum can be just
>> recalculated. That's what the linux stack does: it throws the old
>> checksum away (ignored on local receive and recalculated on send
>> forward paths).
> 
> Sorry, something here seems to me still wrong; maybe I miss something.
> We are talking about transport mode. Let's take TCP for example.
> the packet is received by another peer. It is on port 4500 because of
> NAT-T. Since it is ESP encrypted , it is it is decrypted. It reaches
> the TCP layer. In TCP (as opposed to UDP), the checksum is mandatory.
> But the checksum in that TCP header of the received
> packet will not be correct, since the IP header of that packet is
> **NOT** the original address ; the IP address was changed by the NAT.
> The NAT  cannot change the TCP checksum since it is encrypted. So
> wouldn't we have a checksum error in the case of TCP ?  It seems to me
> that the purpose of NAT-OA was to pass the
> original address, so that there will be no error in such a case.
> But again, maybe I miss something, since I did not heard that
> transport mode has any problems with NAT-T. Or maybe this was not
> tested?

Yes, it's the primary use case for NAT-OA, to allow "fast" update of the
checksum.

The Linux way works too. It's tested.

Like I said, the IPsec stack says to TCP/UDP stack part "I've already
check the checksum, do not look at it". If the packet is forwarded the
kernel (or NIC hardware) *recalculates* the proper checksum; as if it
was generating the packet in first place.

Using NAT-OA to update checksum is purely an optimisation; mostly useful
only when forwarding form one IPsec tunnel to another one which does not
happen often in transport mode (perhaps only in some very special NAT
setups).