netdev - Re: [RFC] GSO: Questions for IPVS and GSO for IPv6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53D86359.2000604@fb.com>
Date:	Tue, 29 Jul 2014 20:15:37 -0700
From:	Alex Gartrell <agartrell@...com>
To:	Julian Anastasov <ja@....bg>, <netdev@...r.kernel.org>
CC:	<lvs-devel@...r.kernel.org>, kernel-team <Kernel-team@...com>
Subject: Re: [RFC] GSO: Questions for IPVS and GSO for IPv6

Thanks for putting this up Julian.  As a meta-point, working with 
upstream isn't necessarily something that has come easily or obviously 
to us corporate shills at Internet companies, but it's pretty obvious at 
this point that it was foolish for us to go it alone :)

> [PATCH net] ipvs: properly declare tunnel encapsulation
>
> The tunneling method should properly use tunnel encapsulation.
> Fixes problem with CHECKSUM_PARTIAL packets when TCP/UDP csum
> offload is supported.
>
> Thanks to Alex Gartrell for reporting the problem, providing
> solution and for all suggestions.
>
> Reported-by: Alex Gartrell <agartrell@...com>
> TODO: waiting for final ack from Alex before adding Signed-off-by line

nit-picky comments below, but this looks good to me.

> Signed-off-by: Julian Anastasov <ja@....bg>
> ---
>   net/netfilter/ipvs/ip_vs_xmit.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> index 73ba1cc..8c1172c 100644
> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> @@ -38,6 +38,7 @@
>   #include <net/route.h>                  /* for ip_route_output */
>   #include <net/ipv6.h>
>   #include <net/ip6_route.h>
> +#include <net/ip_tunnels.h>
>   #include <net/addrconf.h>
>   #include <linux/icmpv6.h>
>   #include <linux/netfilter.h>
> @@ -862,14 +863,19 @@ ip_vs_tunnel_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
>   		old_iph = ip_hdr(skb);
>   	}
>
> -	skb->transport_header = skb->network_header;
> -
>   	/* fix old IP header checksum */
>   	ip_send_check(old_iph);
>
> +	skb = iptunnel_handle_offloads(skb, false, SKB_GSO_IPIP);
> +	if (IS_ERR(skb))
> +		goto tx_error;
> +
> +	skb->transport_header = skb->network_header;
> +
>   	skb_push(skb, sizeof(struct iphdr));
>   	skb_reset_network_header(skb);
>   	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
> +	skb_clear_hash(skb);

We need this in ipv4 but not in ipv6?

>
>   	/*
>   	 *	Push down and install the IPIP header.
> @@ -900,7 +906,8 @@ ip_vs_tunnel_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
>   	return NF_STOLEN;
>
>     tx_error:
> -	kfree_skb(skb);
> +	if (!IS_ERR(skb))
> +		kfree_skb(skb);
>   	rcu_read_unlock();
>   	LeaveFunction(10);
>   	return NF_STOLEN;
> @@ -953,6 +960,10 @@ ip_vs_tunnel_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
>   		old_iph = ipv6_hdr(skb);
>   	}
>
> +	skb = iptunnel_handle_offloads(skb, false, SKB_GSO_SIT);

It sounds like this is going to take a fair amount of work.  Can we set 
gso_type_mask to 0 with a scary comment and push it as a fix and then do 
the right thing as a feature for a future release?

> +	if (IS_ERR(skb))
> +		goto tx_error;
> +
>   	skb->transport_header = skb->network_header;
>
>   	skb_push(skb, sizeof(struct ipv6hdr));
> @@ -988,7 +999,8 @@ ip_vs_tunnel_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
>   	return NF_STOLEN;
>
>   tx_error:
> -	kfree_skb(skb);
> +	if (!IS_ERR(skb))
> +		kfree_skb(skb);
>   	rcu_read_unlock();
>   	LeaveFunction(10);
>   	return NF_STOLEN;
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html