Skip to contentSkip to conversation

L2TP: skb truesize bug in recent kernels

PATCH James Chapman 2 participants 3 messages May 14, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

James Chapman

author
#0 lore raw

A user of L2TP reports skb truesize bugs being logged. His config is GRE over PPP over L2TP. All we know so far is that 2.6.24.4 works and 2.6.25.2 doesn't. There are no other reports of this problem, though this might be the only user using GRE over L2TP tunnels at this time.

The truesize bugs don't occur for every packet:

SKB BUG: Invalid truesize (272) len=72, sizeof(sk_buff)=208 SKB BUG: Invalid truesize (272) len=81, sizeof(sk_buff)=208

The pppol2tp driver uses skb_cow_head() to make headroom for IP, UDP, L2TP and PPP headers. As GRE is being used, it is more likely that there will be insufficient headroom. Does the pppol2tp driver need to adjust truesize if pskb_expand_head() is called?

I tried the following hack which stopped the skb truesize bug but caused a kernel assert when the socket was closed:

KERN: assertion (!atomic_read(&sk->sk_wmem_alloc)) failed at net/ipv4/af_inet.c (155)

drivers/net/pppol2tp.c +6 -0
Index: linux-2.6.25-new/drivers/net/pppol2tp.c
===================================================================
@@ -980,6 +980,8 @@ static int pppol2tp_xmit(struct ppp_chan
 	__wsum csum = 0;
 	struct udphdr *uh;
 	unsigned int len;
+	int old_headroom;
+	int new_headroom;
 
 	if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED))
 		goto abort;

@@ -1008,9 +1010,13 @@ static int pppol2tp_xmit(struct ppp_chan
 	 */
 	headroom = NET_SKB_PAD + sizeof(struct iphdr) +
 		sizeof(struct udphdr) + hdr_len + sizeof(ppph);
+	old_headroom = skb_headroom(skb);
 	if (skb_cow_head(skb, headroom))
 		goto abort;
 
+	new_headroom = skb_headroom(skb);
+	skb->truesize += new_headroom - old_headroom;
+
 	/* Setup PPP header */
 	__skb_push(skb, sizeof(ppph));
 	skb->data[0] = ppph[0];

Does the driver need to mess with truesize?

Signature · 4 lines
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
maintainer
#1 lore raw

From: James Chapman <[email protected]>
Date: Wed, 14 May 2008 11:07:16 +0100

Quoted 10 lines

The pppol2tp driver uses skb_cow_head() to make headroom for IP, UDP, L2TP and PPP headers. As GRE is being used, it is more likely that there will be insufficient headroom. Does the pppol2tp driver need to adjust truesize if pskb_expand_head() is called?

I tried the following hack which stopped the skb truesize bug but caused a kernel assert when the socket was closed:

KERN: assertion (!atomic_read(&sk->sk_wmem_alloc)) failed at net/ipv4/af_inet.c (155)

You can't adjust the truesize when there is a socket associated with the SKB.

We just had a weeklong thread on this list about these issues wrt. the wireless stack :-)

skb->truesize records how much memory was charged to the assosicated socket, so when the socket is freed, the destructor goes

	atomic_dec(&sk->sk_{r,w}mem_alloc, skb->truesize);

so if you increase truesize, the counter will be decremented more than it was initially incremented.

You cannot change the size of the packet substantially when there is a socket associated with it, because this makes the truesize inaccurate, and thus provides a vector for a user's socket to use up more memory than we were originally going to let it use based upon it's send and receive buffer limits.

author
#2 lore raw
Quoted 7 lines · David Miller

From: James Chapman <[email protected]>
Date: Wed, 14 May 2008 11:07:16 +0100

Quoted 4 lines

The pppol2tp driver uses skb_cow_head() to make headroom for IP, UDP, L2TP and PPP headers. As GRE is being used, it is more likely that there will be insufficient headroom. Does the pppol2tp driver need to adjust truesize if pskb_expand_head() is called?

I tried the following hack which stopped the skb truesize bug but caused a kernel assert when the socket was closed:

Quoted 7 lines
Quoted 1 line

KERN: assertion (!atomic_read(&sk->sk_wmem_alloc)) failed at net/ipv4/af_inet.c (155)

You can't adjust the truesize when there is a socket associated with the SKB.

We just had a weeklong thread on this list about these issues wrt. the wireless stack :-)

Yeah, I saw that thread but thought it was a different problem. :)

Quoted 5 lines

skb->truesize records how much memory was charged to the assosicated socket, so when the socket is freed, the destructor goes

	atomic_dec(&sk->sk_{r,w}mem_alloc, skb->truesize);
Quoted 8 lines

so if you increase truesize, the counter will be decremented more than it was initially incremented.

You cannot change the size of the packet substantially when there is a socket associated with it, because this makes the truesize inaccurate, and thus provides a vector for a user's socket to use up more memory than we were originally going to let it use based upon it's send and receive buffer limits.

I see. Thanks for the explanation. Presumably kernels 2.6.24.4 and earlier aren't checking truesize for UDP sockets.

I'll change pppol2tp to allow some slack in its sock_wmalloc() call.

Signature · 4 lines
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development