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

login
register
mail settings
Submitter Daniel Borkmann
Date Feb. 5, 2013, 4:21 p.m.
Message ID <1360081291-30698-1-git-send-email-dborkman@redhat.com>
Download mbox | patch
Permalink /patch/218288/
State Not Applicable
Headers show

Comments

Daniel Borkmann - Feb. 5, 2013, 4:21 p.m.
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.

Cc: Simon Horman <horms@verge.net.au>
Cc: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/netfilter/ipvs/ip_vs_proto_sctp.c |   35 ++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 18 deletions(-)
Julian Anastasov - Feb. 5, 2013, 9:41 p.m.
Hello,

On Tue, 5 Feb 2013, Daniel Borkmann wrote:

> 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.
> 
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

	Looks good to me. Simon, please apply as bugfix.
It seems we have this problem from day 1 (2.6.34).

Acked-by: Julian Anastasov <ja@ssi.bg>

> ---
>  net/netfilter/ipvs/ip_vs_proto_sctp.c |   35 ++++++++++++++++-----------------
>  1 files 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.1

Regards

--
Julian Anastasov <ja@ssi.bg>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
 }