[PATCH] l2tp: avoid checksum offload for fragmented packets

[PATCH] l2tp: avoid checksum offload for fragmented packets

[Tom Parkin]

UDP encapsulated L2TP data is currently subject to checksum errors if hardware
checksum offload is used and the packet is fragmented through the L2TP tunnel.

This patch works around this issue by falling back to software checksumming if
the packet will fragment.

This patch is based on the previous RFC patch, with the following ammendments:

	* Ben LaHaise's comments on header length calculation and IPv6
	  checksumming have been integrated

	* l2tp_xmit_ipv6_csum has been updated in a similar manner to the new
	  l2tp_xmit_ipv4_csum

Tom Parkin (1):
  l2tp: avoid checksum offload for fragmented packets

 net/l2tp/l2tp_core.c |   65 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 19 deletions(-)

-- 
1.7.9.5

[PATCH] l2tp: avoid checksum offload for fragmented packets

[Tom Parkin]

Hardware offload for UDP datagram checksum calculation doesn't work with
fragmented IP packets -- the device will note the fragmentation and leave the
UDP checksum well alone.

As such, if we expect the L2TP packet to be fragmented by the IP layer we need
to perform the UDP checksum ourselves in software (ref: net/ipv4/udp.c).

This change modifies the L2TP xmit path to fallback to software checksum
calculation if the L2TP packet + IP header exceeds the tunnel device MTU.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c |   65 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6984c3a..2b08920 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1114,9 +1114,18 @@ static void l2tp_xmit_ipv6_csum(struct sock *sk, struct sk_buff *skb,
 {
 	struct ipv6_pinfo *np = inet6_sk(sk);
 	struct udphdr *uh = udp_hdr(skb);
+	struct ipv6_txoptions *opt = np->opt;
+	size_t ip6hlen;
+
+	ip6hlen = sizeof(struct ipv6hdr);
+	if (opt)
+		ip6hlen += opt->opt_nflen + opt->opt_flen;
+	if (skb_dst(skb) && skb_dst(skb)->dev)
+		ip6hlen += LL_RESERVED_SPACE(skb_dst(skb)->dev);
 
 	if (!skb_dst(skb) || !skb_dst(skb)->dev ||
-	    !(skb_dst(skb)->dev->features & NETIF_F_IPV6_CSUM)) {
+	    !(skb_dst(skb)->dev->features & NETIF_F_IPV6_CSUM) ||
+	    (udp_len + ip6hlen) > dst_mtu(skb_dst(skb))) {
 		__wsum csum = skb_checksum(skb, 0, udp_len, 0);
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 		uh->check = csum_ipv6_magic(&np->saddr, &np->daddr, udp_len,
@@ -1133,6 +1142,40 @@ static void l2tp_xmit_ipv6_csum(struct sock *sk, struct sk_buff *skb,
 }
 #endif
 
+static void l2tp_xmit_ipv4_csum(struct sock *sk, struct sk_buff *skb,
+				int udp_len)
+{
+	struct udphdr *uh = udp_hdr(skb);
+	struct inet_sock *inet = inet_sk(sk);
+	struct ip_options_rcu *inet_opt;
+	size_t iphlen;
+
+	rcu_read_lock();
+	inet_opt = rcu_dereference(inet->inet_opt);
+	iphlen = sizeof(struct iphdr) + (inet_opt ? inet_opt->opt.optlen : 0);
+	rcu_read_unlock();
+
+	if (!skb_dst(skb) || !skb_dst(skb)->dev ||
+	    !(skb_dst(skb)->dev->features & NETIF_F_V4_CSUM) ||
+	    (udp_len + iphlen) > dst_mtu(skb_dst(skb))) {
+		__wsum csum;
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+		csum = skb_checksum(skb, 0, udp_len, 0);
+		uh->check = csum_tcpudp_magic(inet->inet_saddr,
+				inet->inet_daddr,
+				udp_len, IPPROTO_UDP, csum);
+		if (uh->check == 0)
+			uh->check = CSUM_MANGLED_0;
+	} else {
+		skb->ip_summed = CHECKSUM_PARTIAL;
+		skb->csum_start = skb_transport_header(skb) - skb->head;
+		skb->csum_offset = offsetof(struct udphdr, check);
+		uh->check = ~csum_tcpudp_magic(inet->inet_saddr,
+				inet->inet_daddr,
+				udp_len, IPPROTO_UDP, 0);
+	}
+}
+
 /* If caller requires the skb to have a ppp header, the header must be
  * inserted in the skb data before calling this function.
  */
@@ -1144,7 +1187,6 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 	struct flowi *fl;
 	struct udphdr *uh;
 	struct inet_sock *inet;
-	__wsum csum;
 	int headroom;
 	int uhlen = (tunnel->encap == L2TP_ENCAPTYPE_UDP) ? sizeof(struct udphdr) : 0;
 	int udp_len;
@@ -1204,23 +1246,8 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
 #endif
 		if (sk->sk_no_check == UDP_CSUM_NOXMIT)
 			skb->ip_summed = CHECKSUM_NONE;
-		else if ((skb_dst(skb) && skb_dst(skb)->dev) &&
-			 (!(skb_dst(skb)->dev->features & NETIF_F_V4_CSUM))) {
-			skb->ip_summed = CHECKSUM_COMPLETE;
-			csum = skb_checksum(skb, 0, udp_len, 0);
-			uh->check = csum_tcpudp_magic(inet->inet_saddr,
-						      inet->inet_daddr,
-						      udp_len, IPPROTO_UDP, csum);
-			if (uh->check == 0)
-				uh->check = CSUM_MANGLED_0;
-		} else {
-			skb->ip_summed = CHECKSUM_PARTIAL;
-			skb->csum_start = skb_transport_header(skb) - skb->head;
-			skb->csum_offset = offsetof(struct udphdr, check);
-			uh->check = ~csum_tcpudp_magic(inet->inet_saddr,
-						       inet->inet_daddr,
-						       udp_len, IPPROTO_UDP, 0);
-		}
+		else
+			l2tp_xmit_ipv4_csum(sk, skb, udp_len);
 		break;
 
 	case L2TP_ENCAPTYPE_IP:
-- 
1.7.9.5

Re: [PATCH] l2tp: avoid checksum offload for fragmented packets

[Ben Hutchings]

On Mon, 2013-06-03 at 08:49 +0100, Tom Parkin wrote:
> Hardware offload for UDP datagram checksum calculation doesn't work with
> fragmented IP packets -- the device will note the fragmentation and leave the
> UDP checksum well alone.
> 
> As such, if we expect the L2TP packet to be fragmented by the IP layer we need
> to perform the UDP checksum ourselves in software (ref: net/ipv4/udp.c).
>
> This change modifies the L2TP xmit path to fallback to software checksum
> calculation if the L2TP packet + IP header exceeds the tunnel device MTU.
[...]

Surely this should be done in the IP stack when fragmenting, not in any
particular client?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] l2tp: avoid checksum offload for fragmented packets

[Tom Parkin]

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

On Mon, Jun 03, 2013 at 03:44:12PM +0100, Ben Hutchings wrote:
> On Mon, 2013-06-03 at 08:49 +0100, Tom Parkin wrote:
> > Hardware offload for UDP datagram checksum calculation doesn't work with
> > fragmented IP packets -- the device will note the fragmentation and leave the
> > UDP checksum well alone.
> > 
> > As such, if we expect the L2TP packet to be fragmented by the IP layer we need
> > to perform the UDP checksum ourselves in software (ref: net/ipv4/udp.c).
> >
> > This change modifies the L2TP xmit path to fallback to software checksum
> > calculation if the L2TP packet + IP header exceeds the tunnel device MTU.
> [...]
> 
> Surely this should be done in the IP stack when fragmenting, not in any
> particular client?
> 

Hmm, that's a good question.

I'm not sure it makes sense to push this down into the IP layer, though.  Since 
it's the UDP checksum we're calculating, it seems reasonable to handle it at 
the UDP layer (which is where L2TP sits when using UDP encapsulation).

If you're worried about reproducing similar code in both UDP and L2TP
I can see where you're coming from, but since UDP uses corking and
L2TP doesn't the data transmit path is quite dissimilar.  We could
probably do some work to share the code, but it doesn't seem worth it
for the amount of sharing we'd be able to achieve.
-- 
Tom Parkin
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] l2tp: avoid checksum offload for fragmented packets

[Ben Hutchings]

On Wed, 2013-06-05 at 10:41 +0100, Tom Parkin wrote:
> On Mon, Jun 03, 2013 at 03:44:12PM +0100, Ben Hutchings wrote:
> > On Mon, 2013-06-03 at 08:49 +0100, Tom Parkin wrote:
> > > Hardware offload for UDP datagram checksum calculation doesn't work with
> > > fragmented IP packets -- the device will note the fragmentation and leave the
> > > UDP checksum well alone.
> > > 
> > > As such, if we expect the L2TP packet to be fragmented by the IP layer we need
> > > to perform the UDP checksum ourselves in software (ref: net/ipv4/udp.c).
> > >
> > > This change modifies the L2TP xmit path to fallback to software checksum
> > > calculation if the L2TP packet + IP header exceeds the tunnel device MTU.
> > [...]
> > 
> > Surely this should be done in the IP stack when fragmenting, not in any
> > particular client?
> > 
> 
> Hmm, that's a good question.
> 
> I'm not sure it makes sense to push this down into the IP layer, though.  Since 
> it's the UDP checksum we're calculating, it seems reasonable to handle it at 
> the UDP layer (which is where L2TP sits when using UDP encapsulation).

TCP, UDP and similar checksums can be handled generically, e.g. if
dev_hard_start_xmit() finds the device doesn't actually do checksum
offload then it calls skb_checksum_help() to fill it in.  I was thinking
that since the IP layer makes the decision to fragment then it should
also be responsible for filling in the checksum before doing so.  Why
should the transport layer protocol have to guess?

> If you're worried about reproducing similar code in both UDP and L2TP
> I can see where you're coming from, but since UDP uses corking and
> L2TP doesn't the data transmit path is quite dissimilar.  We could
> probably do some work to share the code, but it doesn't seem worth it
> for the amount of sharing we'd be able to achieve.

OK, so long as you've thought about the options.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] l2tp: avoid checksum offload for fragmented packets

[Tom Parkin]

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

On Wed, Jun 05, 2013 at 01:57:57PM +0100, Ben Hutchings wrote:
> On Wed, 2013-06-05 at 10:41 +0100, Tom Parkin wrote:
> > On Mon, Jun 03, 2013 at 03:44:12PM +0100, Ben Hutchings wrote:
> > > On Mon, 2013-06-03 at 08:49 +0100, Tom Parkin wrote:
> > > > Hardware offload for UDP datagram checksum calculation doesn't work with
> > > > fragmented IP packets -- the device will note the fragmentation and leave the
> > > > UDP checksum well alone.
> > > > 
> > > > As such, if we expect the L2TP packet to be fragmented by the IP layer we need
> > > > to perform the UDP checksum ourselves in software (ref: net/ipv4/udp.c).
> > > >
> > > > This change modifies the L2TP xmit path to fallback to software checksum
> > > > calculation if the L2TP packet + IP header exceeds the tunnel device MTU.
> > > [...]
> > > 
> > > Surely this should be done in the IP stack when fragmenting, not in any
> > > particular client?
> > > 
> > 
> > Hmm, that's a good question.
> > 
> > I'm not sure it makes sense to push this down into the IP layer, though.  Since 
> > it's the UDP checksum we're calculating, it seems reasonable to handle it at 
> > the UDP layer (which is where L2TP sits when using UDP encapsulation).
> 
> TCP, UDP and similar checksums can be handled generically, e.g. if
> dev_hard_start_xmit() finds the device doesn't actually do checksum
> offload then it calls skb_checksum_help() to fill it in.  I was thinking
> that since the IP layer makes the decision to fragment then it should
> also be responsible for filling in the checksum before doing so.  Why
> should the transport layer protocol have to guess?

Fair point!

I suppose an argument can be made either way, so really it comes down to a
question of taste and a feel for how the net tree "should" handle
this.

Dave -- could you give me a steer?  Are you happy to keep this kind of
calculation in the transport layer, or should I look to push something
generic into the IP code?

Thanks,
Tom
-- 
Tom Parkin
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] l2tp: avoid checksum offload for fragmented packets

[David Miller]

From: Tom Parkin <tparkin@katalix.com>
Date: Tue, 11 Jun 2013 09:00:38 +0100

> Dave -- could you give me a steer?  Are you happy to keep this kind of
> calculation in the transport layer, or should I look to push something
> generic into the IP code?

I think it's an IP layer issue, so should be handled there.

Thanks.