Patchwork WARNING: at net/core/skbuff.c:154 with tcpdump and ipsec

login
register
mail settings
Submitter Jarek Poplawski
Date Feb. 13, 2009, 12:14 p.m.
Message ID <20090213121424.GA8717@ff.dom.local>
Download mbox | patch
Permalink /patch/23108/
State RFC
Delegated to: David Miller
Headers show

Comments

Jarek Poplawski - Feb. 13, 2009, 12:14 p.m.
On 11-02-2009 16:55, Krzysztof Oledzki wrote:
> 
> On Wed, 11 Feb 2009, Marco Berizzi wrote:
> 
>> Hi Folks,
>>
>> I'm getting this error on 2.6.28.4 when I run tcpdump on
>> the interface where ipsec packets are enc/decrypted.
>>
>> TIA
>>
>>> Feb 11 10:53:55 Pleiadi kernel: ------------[ cut here ]------------
>>> Feb 11 10:53:55 Pleiadi kernel: WARNING: at net/core/skbuff.c:154 skb_truesize_bug+0x2e/0x33()
>>> Feb 11 10:53:55 Pleiadi kernel: SKB BUG: Invalid truesize (268) len=134, sizeof(sk_buff)=172
> <CUT>
> 
> This annoying problem is quite old (appeared in 2.6.25) and already known:
>   http://bugzilla.kernel.org/show_bug.cgi?id=10996
> 
> Sadly, no one is interested in fixing it. :(

Here is a debugging patch doing these checks a bit earlier, so maybe
we get something new and interesting. ;)

Thanks,
Jarek P.
---

 include/linux/skbuff.h |    8 ++++++--
 net/packet/af_packet.c |    9 +++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich - Feb. 13, 2009, 6:56 p.m.
Jarek Poplawski wrote:
> On 11-02-2009 16:55, Krzysztof Oledzki wrote:
>> On Wed, 11 Feb 2009, Marco Berizzi wrote:
>>
>>> Hi Folks,
>>>
>>> I'm getting this error on 2.6.28.4 when I run tcpdump on
>>> the interface where ipsec packets are enc/decrypted.
>>>
>>> TIA
>>>
>>>> Feb 11 10:53:55 Pleiadi kernel: ------------[ cut here ]------------
>>>> Feb 11 10:53:55 Pleiadi kernel: WARNING: at net/core/skbuff.c:154 skb_truesize_bug+0x2e/0x33()
>>>> Feb 11 10:53:55 Pleiadi kernel: SKB BUG: Invalid truesize (268) len=134, sizeof(sk_buff)=172
>> <CUT>
>>
>> This annoying problem is quite old (appeared in 2.6.25) and already known:
>>   http://bugzilla.kernel.org/show_bug.cgi?id=10996
>>
>> Sadly, no one is interested in fixing it. :(
> 
> Here is a debugging patch doing these checks a bit earlier, so maybe
> we get something new and interesting. ;)
> 
> Thanks,
> Jarek P.
> ---
> 

I did notice that pskb_expand_head() doesn't change the skb->truesize even
though it could grow the skb.  I saw this problem with tcpdump while
experimenting with some SCTP code.

This is not to say that it is the problem in this case, but it's one of
them that I've seen.

-vlad
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski - Feb. 13, 2009, 7:24 p.m.
On Fri, Feb 13, 2009 at 01:56:01PM -0500, Vlad Yasevich wrote:
...
> I did notice that pskb_expand_head() doesn't change the skb->truesize even
> though it could grow the skb.  I saw this problem with tcpdump while
> experimenting with some SCTP code.
> 
> This is not to say that it is the problem in this case, but it's one of
> them that I've seen.

Yes, I've read Herbert Xu's message pointing especially to
xfrm_state_check_space(). So I would like to make sure if there is no
other reason it triggers in packet_recvmsg() on these several reports.

If af_packet code is OK, I guess we could update truesize for it:
there is no reason to warn here about bugs from other, well known
places.

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich - Feb. 13, 2009, 7:42 p.m.
Jarek Poplawski wrote:
> On Fri, Feb 13, 2009 at 01:56:01PM -0500, Vlad Yasevich wrote:
> ...
>> I did notice that pskb_expand_head() doesn't change the skb->truesize even
>> though it could grow the skb.  I saw this problem with tcpdump while
>> experimenting with some SCTP code.
>>
>> This is not to say that it is the problem in this case, but it's one of
>> them that I've seen.
> 
> Yes, I've read Herbert Xu's message pointing especially to
> xfrm_state_check_space(). So I would like to make sure if there is no
> other reason it triggers in packet_recvmsg() on these several reports.
> 
> If af_packet code is OK, I guess we could update truesize for it:
> there is no reason to warn here about bugs from other, well known
> places.
> 

Personally, I think pskb_expand_head should fix the skb->truesize.  This
way any subsequent clones will not trigger this warning.

Another alternative is to audit the pskb_expand_head() usages and adjust
truesize in each case needed, which is just ugly.

-vlad

> Jarek P.
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski - Feb. 13, 2009, 8 p.m.
On Fri, Feb 13, 2009 at 02:42:07PM -0500, Vlad Yasevich wrote:
...
> Personally, I think pskb_expand_head should fix the skb->truesize.  This
> way any subsequent clones will not trigger this warning.

Personally, I think there is no reason to call skb_truesize_bug() in
anything but some #ifdef CONFIG_XX_DEBUG, if we ignore these reports
for so long.

> Another alternative is to audit the pskb_expand_head() usages and adjust
> truesize in each case needed, which is just ugly.

I guess, it's a lot of work to do it right (if it's possible at all).
Yes, I think about something really ugly here. ;-)

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 14, 2009, 2:24 a.m.
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Fri, 13 Feb 2009 21:00:14 +0100

> On Fri, Feb 13, 2009 at 02:42:07PM -0500, Vlad Yasevich wrote:
> ...
> > Personally, I think pskb_expand_head should fix the skb->truesize.  This
> > way any subsequent clones will not trigger this warning.
> 
> Personally, I think there is no reason to call skb_truesize_bug() in
> anything but some #ifdef CONFIG_XX_DEBUG, if we ignore these reports
> for so long.

If skb->sk is non-NULL, fixing up the truesize will corrupt
socket memory accounting.

Anyways, Herbert and I have talked about %100 removing the
warning.
--
To unsubscribe from this list: send the line "unsubscribe netdev" 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/include/linux/skbuff.h b/include/linux/skbuff.h
index cf2cb50..20c3182 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -418,12 +418,16 @@  extern void	      skb_under_panic(struct sk_buff *skb, int len,
 				      void *here);
 extern void	      skb_truesize_bug(struct sk_buff *skb);
 
-static inline void skb_truesize_check(struct sk_buff *skb)
+static inline int skb_truesize_check(struct sk_buff *skb)
 {
 	int len = sizeof(struct sk_buff) + skb->len;
 
-	if (unlikely((int)skb->truesize < len))
+	if (unlikely((int)skb->truesize < len)) {
 		skb_truesize_bug(skb);
+		return 1;
+	}
+
+	return 0;
 }
 
 extern int skb_append_datato_frags(struct sock *sk, struct sk_buff *skb,
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1fc4a78..08200be 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -307,6 +307,9 @@  static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,  struct
 	if (dev_net(dev) != sock_net(sk))
 		goto out;
 
+	if (skb_truesize_check(skb))
+		goto out;
+
 	if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL)
 		goto oom;
 
@@ -495,6 +498,9 @@  static int packet_rcv(struct sk_buff *skb, struct net_device *dev, struct packet
 	if (dev_net(dev) != sock_net(sk))
 		goto drop;
 
+	if (skb_truesize_check(skb))
+		goto drop;
+
 	skb->dev = dev;
 
 	if (dev->header_ops) {
@@ -617,6 +623,9 @@  static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, struct packe
 	if (dev_net(dev) != sock_net(sk))
 		goto drop;
 
+	if (skb_truesize_check(skb))
+		goto drop;
+
 	if (dev->header_ops) {
 		if (sk->sk_type != SOCK_DGRAM)
 			skb_push(skb, skb->data - skb_mac_header(skb));