L2TP: skb truesize bug in recent kernels

L2TP: skb truesize bug in recent kernels

[James Chapman]

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)

Index: linux-2.6.25-new/drivers/net/pppol2tp.c
===================================================================
--- linux-2.6.25.orig/drivers/net/pppol2tp.c
+++ linux-2.6.25/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?


-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: L2TP: skb truesize bug in recent kernels

[David Miller]

From: James Chapman <jchapman@katalix.com>
Date: Wed, 14 May 2008 11:07:16 +0100

> 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.

Re: L2TP: skb truesize bug in recent kernels

[James Chapman]

David Miller wrote:
> From: James Chapman <jchapman@katalix.com>
> Date: Wed, 14 May 2008 11:07:16 +0100
> 
>> 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 :-)

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

> 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.

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.


-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development