[GIT PULL nf] IPVS fixes #2

[GIT PULL nf] IPVS fixes #2

[Simon Horman]

Hi Pablo,

Another fix. This one seems suitable for stable all the way back to
2.6.34. I suspect this change may not apply cleanly all the way back there.
Please let me know if I should prepare some backports.

----------------------------------------------------------------
The following changes since commit b425df4cdd953a400d814b4474c9d3ec04481858:

  ipvs: freeing uninitialized pointer on error (2013-01-28 10:14:37 +0900)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git tags/ipvs-fixes2-for-v3.8

for you to fetch changes up to 4b47bc9a9e69141ed3a854c57601f548e82c78ba:

  ipvs: sctp: fix checksumming on snat and dnat handlers (2013-02-06 09:56:50 +0900)

----------------------------------------------------------------
Second fix for IPVS targeted at 3.8

SCTP checksumming correction by Daniel Borkmann

This appears to have been present since SCTP support was added to IPVS
and is thus present since 2.6.34.

----------------------------------------------------------------
Daniel Borkmann (1):
      ipvs: sctp: fix checksumming on snat and dnat handlers

 net/netfilter/ipvs/ip_vs_proto_sctp.c |   35 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 18 deletions(-)

[PATCH] ipvs: sctp: fix checksumming on snat and dnat handlers

[Simon Horman]

From: Daniel Borkmann <dborkman@redhat.com>

In our test lab, we have a simple SCTP client connecting to a SCTP
server via an IPVS load balancer. On some machines, load balancing
works, but on others the initial handshake just fails, thus no
SCTP connection whatsoever can be established!

We observed that the SCTP INIT-ACK handshake reply from the IPVS
machine to the client had a correct IP checksum, but corrupt SCTP
checksum when forwarded, thus on the client-side the packet was
dropped and an intial handshake retriggered until all attempts
run into the void.

To fix this issue, this patch i) adds a missing CHECKSUM_UNNECESSARY
after the full checksum (re-)calculation (as done in IPVS TCP and UDP
code as well), ii) calculates the checksum in little-endian format
(as fixed with the SCTP code in commit 4458f04c: sctp: Clean up sctp
checksumming code) and iii) refactors duplicate checksum code into a
common function. Tested by myself.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 net/netfilter/ipvs/ip_vs_proto_sctp.c |   35 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index 746048b..ae8ec6f 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -61,14 +61,27 @@ sctp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
 	return 1;
 }
 
+static void sctp_nat_csum(struct sk_buff *skb, sctp_sctphdr_t *sctph,
+			  unsigned int sctphoff)
+{
+	__u32 crc32;
+	struct sk_buff *iter;
+
+	crc32 = sctp_start_cksum((__u8 *)sctph, skb_headlen(skb) - sctphoff);
+	skb_walk_frags(skb, iter)
+		crc32 = sctp_update_cksum((u8 *) iter->data,
+					  skb_headlen(iter), crc32);
+	sctph->checksum = sctp_end_cksum(crc32);
+
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+}
+
 static int
 sctp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
 		  struct ip_vs_conn *cp, struct ip_vs_iphdr *iph)
 {
 	sctp_sctphdr_t *sctph;
 	unsigned int sctphoff = iph->len;
-	struct sk_buff *iter;
-	__be32 crc32;
 
 #ifdef CONFIG_IP_VS_IPV6
 	if (cp->af == AF_INET6 && iph->fragoffs)
@@ -92,13 +105,7 @@ sctp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
 	sctph = (void *) skb_network_header(skb) + sctphoff;
 	sctph->source = cp->vport;
 
-	/* Calculate the checksum */
-	crc32 = sctp_start_cksum((u8 *) sctph, skb_headlen(skb) - sctphoff);
-	skb_walk_frags(skb, iter)
-		crc32 = sctp_update_cksum((u8 *) iter->data, skb_headlen(iter),
-				          crc32);
-	crc32 = sctp_end_cksum(crc32);
-	sctph->checksum = crc32;
+	sctp_nat_csum(skb, sctph, sctphoff);
 
 	return 1;
 }
@@ -109,8 +116,6 @@ sctp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
 {
 	sctp_sctphdr_t *sctph;
 	unsigned int sctphoff = iph->len;
-	struct sk_buff *iter;
-	__be32 crc32;
 
 #ifdef CONFIG_IP_VS_IPV6
 	if (cp->af == AF_INET6 && iph->fragoffs)
@@ -134,13 +139,7 @@ sctp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
 	sctph = (void *) skb_network_header(skb) + sctphoff;
 	sctph->dest = cp->dport;
 
-	/* Calculate the checksum */
-	crc32 = sctp_start_cksum((u8 *) sctph, skb_headlen(skb) - sctphoff);
-	skb_walk_frags(skb, iter)
-		crc32 = sctp_update_cksum((u8 *) iter->data, skb_headlen(iter),
-					  crc32);
-	crc32 = sctp_end_cksum(crc32);
-	sctph->checksum = crc32;
+	sctp_nat_csum(skb, sctph, sctphoff);
 
 	return 1;
 }
-- 
1.7.10.4

RE: [PATCH] ipvs: sctp: fix checksumming on snat and dnat handlers

[David Laight]

> From: Daniel Borkmann <dborkman@redhat.com>
> 
> In our test lab, we have a simple SCTP client connecting to a SCTP
> server via an IPVS load balancer. On some machines, load balancing
> works, but on others the initial handshake just fails, thus no
> SCTP connection whatsoever can be established!
> 
> We observed that the SCTP INIT-ACK handshake reply from the IPVS
> machine to the client had a correct IP checksum, but corrupt SCTP
> checksum when forwarded, thus on the client-side the packet was
> dropped and an intial handshake retriggered until all attempts
> run into the void.
> 
> To fix this issue, this patch i) adds a missing CHECKSUM_UNNECESSARY
> after the full checksum (re-)calculation (as done in IPVS TCP and UDP
> code as well), ii) calculates the checksum in little-endian format
> (as fixed with the SCTP code in commit 4458f04c: sctp: Clean up sctp
> checksumming code) and iii) refactors duplicate checksum code into a
> common function. Tested by myself.

If the NAT code has only modified a few bytes inside one of the SCTP
chunks, then the crc can be modified by knowing the changed bits and
the number of bytes to the end of the chunk (without looking at
any other data bytes).
That would (probably) be faster (may not really matter for INIT-ACK.

	David

RE: [PATCH] ipvs: sctp: fix checksumming on snat and dnat handlers

[David Laight]

> From: Daniel Borkmann <dborkman@redhat.com>
> 
> In our test lab, we have a simple SCTP client connecting to a SCTP
> server via an IPVS load balancer. On some machines, load balancing
> works, but on others the initial handshake just fails, thus no
> SCTP connection whatsoever can be established!
> 
> We observed that the SCTP INIT-ACK handshake reply from the IPVS
> machine to the client had a correct IP checksum, but corrupt SCTP
> checksum when forwarded, thus on the client-side the packet was
> dropped and an intial handshake retriggered until all attempts
> run into the void.
> 
> To fix this issue, this patch i) adds a missing CHECKSUM_UNNECESSARY
> after the full checksum (re-)calculation (as done in IPVS TCP and UDP
> code as well), ii) calculates the checksum in little-endian format
> (as fixed with the SCTP code in commit 4458f04c: sctp: Clean up sctp
> checksumming code) and iii) refactors duplicate checksum code into a
> common function. Tested by myself.

If the NAT code has only modified a few bytes inside one of the SCTP
chunks, then the crc can be modified by knowing the changed bits and
the number of bytes to the end of the chunk (without looking at
any other data bytes).
That would (probably) be faster (may not really matter for INIT-ACK.

	David

Re: [PATCH] ipvs: sctp: fix checksumming on snat and dnat handlers

[Daniel Borkmann]

On 02/06/2013 10:18 AM, David Laight wrote:
> If the NAT code has only modified a few bytes inside one of the SCTP
> chunks, then the crc can be modified by knowing the changed bits and
> the number of bytes to the end of the chunk (without looking at
> any other data bytes).
> That would (probably) be faster (may not really matter for INIT-ACK.

If applicable, this could be a follow-up optimization after the bug fix.

Re: [GIT PULL nf] IPVS fixes #2

[Pablo Neira Ayuso]

On Wed, Feb 06, 2013 at 10:24:37AM +0900, Simon Horman wrote:
> Hi Pablo,
> 
> Another fix. This one seems suitable for stable all the way back to
> 2.6.34. I suspect this change may not apply cleanly all the way back there.
> Please let me know if I should prepare some backports.
> 
> ----------------------------------------------------------------
> The following changes since commit b425df4cdd953a400d814b4474c9d3ec04481858:
> 
>   ipvs: freeing uninitialized pointer on error (2013-01-28 10:14:37 +0900)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git tags/ipvs-fixes2-for-v3.8

Pulled, thanks Simon.

Re: [GIT PULL nf] IPVS fixes #2

[Pablo Neira Ayuso]

Hi again Simon,

On Thu, Feb 07, 2013 at 07:12:13PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Feb 06, 2013 at 10:24:37AM +0900, Simon Horman wrote:
> > Hi Pablo,
> > 
> > Another fix. This one seems suitable for stable all the way back to
> > 2.6.34. I suspect this change may not apply cleanly all the way back there.
> > Please let me know if I should prepare some backports.

While reviewing patches that I'll pass to -stable, I noticed that:

4b47bc9 ipvs: sctp: fix checksumming on snat and dnat handlers

depends on:

commit d4383f04d145cce8b855c463f40020639ef83ea0
Author: Jesper Dangaard Brouer <brouer@redhat.com>
Date:   Wed Sep 26 14:07:17 2012 +0200

    ipvs: API change to avoid rescan of IPv6 exthdr

which is too big to add as dependency (it does not fulfill -stable
rule of not bigger patches than 100 lines).

If you want to me pass this sctp fix to -stable, you or Daniel will
have to send me a backport.

Regards.

Re: [GIT PULL nf] IPVS fixes #2

[Daniel Borkmann]

On 02/13/2013 09:13 PM, Pablo Neira Ayuso wrote:
> On Thu, Feb 07, 2013 at 07:12:13PM +0100, Pablo Neira Ayuso wrote:
>> On Wed, Feb 06, 2013 at 10:24:37AM +0900, Simon Horman wrote:
>>> Hi Pablo,
>>>
>>> Another fix. This one seems suitable for stable all the way back to
>>> 2.6.34. I suspect this change may not apply cleanly all the way back there.
>>> Please let me know if I should prepare some backports.
>
> While reviewing patches that I'll pass to -stable, I noticed that:
>
> 4b47bc9 ipvs: sctp: fix checksumming on snat and dnat handlers
>
> depends on:
>
> commit d4383f04d145cce8b855c463f40020639ef83ea0
> Author: Jesper Dangaard Brouer <brouer@redhat.com>
> Date:   Wed Sep 26 14:07:17 2012 +0200
>
>      ipvs: API change to avoid rescan of IPv6 exthdr
>
> which is too big to add as dependency (it does not fulfill -stable
> rule of not bigger patches than 100 lines).
>
> If you want to me pass this sctp fix to -stable, you or Daniel will
> have to send me a backport.

I'm currently working on it. Will send you a patch by today.

Thanks.