[RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[Johannes Berg]

This makes pskb_expand_head warn when called when a socket is attached.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Based on a different patch by davem.

Will also trigger with mac80211.

 net/core/skbuff.c |    2 ++
 1 file changed, 2 insertions(+)

--- everything.orig/net/core/skbuff.c	2008-05-04 01:18:49.000000000 +0200
+++ everything/net/core/skbuff.c	2008-05-04 01:23:50.000000000 +0200
@@ -689,6 +689,8 @@ int pskb_expand_head(struct sk_buff *skb
 	if (!data)
 		goto nodata;
 
+	WARN_ON((nhead || ntail) && skb->sk);
+
 	/* Copy only real data... and, alas, header. This should be
 	 * optimized for the cases when header is void. */
 #ifdef NET_SKBUFF_DATA_USES_OFFSET

Re: [RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[Johannes Berg]

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

On Sun, 2008-05-04 at 20:16 +0200, Johannes Berg wrote:
> This makes pskb_expand_head warn when called when a socket is attached.
 
> +	WARN_ON((nhead || ntail) && skb->sk);
> +

This is triggering now. I think it's a false positive, trace below.

[23194.608011] Badness at net/core/skbuff.c:726
[23194.608014] NIP: c02735c8 LR: c02735a0 CTR: 00000000
[23194.608017] REGS: ccf9baf0 TRAP: 0700   Not tainted  (2.6.25-wl-06933-g0bacadf-dirty)
[23194.608020] MSR: 00029032 <EE,ME,IR,DR>  CR: 48242448  XER: 20000000
[23194.608028] TASK = e6430e20[15208] 'distcc' THREAD: ccf9a000
[23194.608031] GPR00: 00000001 ccf9bba0 e6430e20 fffffff4 cccccccc 00000004 c1497120 c1497148 
[23194.608040] GPR08: 0000005a 00000000 0000005a c08bc200 28242442 1002d36c ccf9bd18 ccf9bd28 
[23194.608049] GPR16: 000005a8 00000001 00000000 ccf6860c 00000000 00000000 00000000 00000000 
[23194.608057] GPR24: 00000000 00000000 000005a8 00000720 00000628 00000000 c1496120 d0e67110 
[23194.608067] NIP [c02735c8] pskb_expand_head+0x80/0x1f8
[23194.608071] LR [c02735a0] pskb_expand_head+0x58/0x1f8
[23194.608074] Call Trace:
[23194.608077] [ccf9bba0] [c02735a0] pskb_expand_head+0x58/0x1f8 (unreliable)
[23194.608082] [ccf9bbc0] [c02737a4] __pskb_pull_tail+0x64/0x374
[23194.608087] [ccf9bbf0] [c027b94c] dev_queue_xmit+0x1e4/0x3cc
[23194.608093] [ccf9bc10] [c02a5c54] ip_finish_output+0x1e8/0x354
[23194.608098] [ccf9bc30] [c02a61b0] ip_local_out+0x34/0x48
[23194.608104] [ccf9bc40] [c02a699c] ip_queue_xmit+0x1fc/0x38c
[23194.608109] [ccf9bcc0] [c02b94d4] tcp_transmit_skb+0x454/0x878
[23194.608115] [ccf9bcf0] [c02bbfb4] tcp_push_one+0x124/0x1f0
[23194.608120] [ccf9bd10] [c02b014c] tcp_sendpage+0x6ec/0x6f4
[23194.608125] [ccf9bd80] [c0269ec0] sock_sendpage+0x34/0x44
[23194.608130] [ccf9bd90] [c00c6dc8] pipe_to_sendpage+0x84/0xa4
[23194.608136] [ccf9bdc0] [c00c802c] __splice_from_pipe+0x118/0x290
[23194.608142] [ccf9be00] [c00c8328] splice_from_pipe+0x64/0x8c
[23194.608147] [ccf9be40] [c00c72d4] do_splice_from+0x80/0x9c
[23194.608152] [ccf9be60] [c00c7aac] splice_direct_to_actor+0xb8/0x1e0
[23194.608158] [ccf9bea0] [c00c7c18] do_splice_direct+0x44/0x80
[23194.608163] [ccf9bed0] [c00a0494] do_sendfile+0x1ec/0x2b0
[23194.608169] [ccf9bf10] [c00a0740] sys_sendfile+0xb4/0x108
[23194.608174] [ccf9bf40] [c00124cc] ret_from_syscall+0x0/0x38


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[Johannes Berg]

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

On Mon, 2008-05-05 at 17:41 +0200, Johannes Berg wrote:
> On Sun, 2008-05-04 at 20:16 +0200, Johannes Berg wrote:
> > This makes pskb_expand_head warn when called when a socket is attached.
>  
> > +	WARN_ON((nhead || ntail) && skb->sk);
> > +
> 
> This is triggering now. I think it's a false positive, trace below.
> 
> [23194.608011] Badness at net/core/skbuff.c:726


> [23194.608077] [ccf9bba0] [c02735a0] pskb_expand_head+0x58/0x1f8 (unreliable)
> [23194.608082] [ccf9bbc0] [c02737a4] __pskb_pull_tail+0x64/0x374

It's actually not really a false positive. What is happening is that
__pskb_pull_tail does (follow 'eat'):

        /* If skb has not enough free space at tail, get new one
         * plus 128 bytes for future expansions. If we have enough
         * room at tail, reallocate without expansion only if skb is cloned.
         */
        int i, k, eat = (skb->tail + delta) - skb->end;

        if (eat > 0 || skb_cloned(skb)) {
                if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
                                     GFP_ATOMIC))
                        return NULL;
        }

which of course changes the true size of the skb without accounting it
to the socket. Now, the reason this hasn't been known before is that the
data size doesn't change because the stuff that is copied into the
header is removed from the data_len... or something like that, I think.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 05 May 2008 18:01:14 +0200

> On Mon, 2008-05-05 at 17:41 +0200, Johannes Berg wrote:
> > [23194.608077] [ccf9bba0] [c02735a0] pskb_expand_head+0x58/0x1f8 (unreliable)
> > [23194.608082] [ccf9bbc0] [c02737a4] __pskb_pull_tail+0x64/0x374
> 
> It's actually not really a false positive. What is happening is that
> __pskb_pull_tail does (follow 'eat'):
 ...
> which of course changes the true size of the skb without accounting it
> to the socket. Now, the reason this hasn't been known before is that the
> data size doesn't change because the stuff that is copied into the
> header is removed from the data_len... or something like that, I think.

FWIW, the only practical case where this can occur is for an SG+CSUM
device which cannot handle DMA'ing highmem pages, and we get such a
page via sendfile() or similar.

All other cases are extremely rare, such as the route changing
mid-connection from a device that can, to a device which cannot do
SG+CSUM.

I think we need to do some more fixups and auditing before we can
enable this pskb_expand_head() assertion, and the same goes for
your more-accurate skb_truesize_check().

Re: [RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[Johannes Berg]

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


> > > [23194.608077] [ccf9bba0] [c02735a0] pskb_expand_head+0x58/0x1f8 (unreliable)
> > > [23194.608082] [ccf9bbc0] [c02737a4] __pskb_pull_tail+0x64/0x374
> > 
> > It's actually not really a false positive. What is happening is that
> > __pskb_pull_tail does (follow 'eat'):
>  ...
> > which of course changes the true size of the skb without accounting it
> > to the socket. Now, the reason this hasn't been known before is that the
> > data size doesn't change because the stuff that is copied into the
> > header is removed from the data_len... or something like that, I think.

> Is this from a kernel with your GSO wireless patches applied by
> chance? :-)

Yes, but I'm pretty sure it wasn't using that code path since the
interface was down when this occurred.

> FWIW, the only practical case where this can occur is for an SG+CSUM
> device which cannot handle DMA'ing highmem pages, and we get such a
> page via sendfile() or similar.

That's well possible since I'm using sungem and it says "no highdma" but
"sg/hwcsum" for my hardware, and I do have highmem.

> I think we need to do some more fixups and auditing before we can
> enable this pskb_expand_head() assertion, and the same goes for
> your more-accurate skb_truesize_check().

Yeah, looks like.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 13 May 2008 10:32:08 +0200

> > FWIW, the only practical case where this can occur is for an SG+CSUM
> > device which cannot handle DMA'ing highmem pages, and we get such a
> > page via sendfile() or similar.
> 
> That's well possible since I'm using sungem and it says "no highdma" but
> "sg/hwcsum" for my hardware, and I do have highmem.

It appears only some sungem parts support full 64-bit DMA, so
yes that's indeed possible.

Re: [RFC/T] [NET] make pskb_expand_head warn when called with invalid state

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 05 May 2008 17:40:47 +0200

> On Sun, 2008-05-04 at 20:16 +0200, Johannes Berg wrote:
> > This makes pskb_expand_head warn when called when a socket is attached.
>  
> > +	WARN_ON((nhead || ntail) && skb->sk);
> > +
> 
> This is triggering now. I think it's a false positive, trace below.

Hmmm, I wonder how you're getting this trace in the first
place :-)

> [23194.608074] Call Trace:
> [23194.608077] [ccf9bba0] [c02735a0] pskb_expand_head+0x58/0x1f8 (unreliable)
> [23194.608082] [ccf9bbc0] [c02737a4] __pskb_pull_tail+0x64/0x374
> [23194.608087] [ccf9bbf0] [c027b94c] dev_queue_xmit+0x1e4/0x3cc
> [23194.608093] [ccf9bc10] [c02a5c54] ip_finish_output+0x1e8/0x354
> [23194.608098] [ccf9bc30] [c02a61b0] ip_local_out+0x34/0x48
> [23194.608104] [ccf9bc40] [c02a699c] ip_queue_xmit+0x1fc/0x38c
> [23194.608109] [ccf9bcc0] [c02b94d4] tcp_transmit_skb+0x454/0x878
> [23194.608115] [ccf9bcf0] [c02bbfb4] tcp_push_one+0x124/0x1f0
> [23194.608120] [ccf9bd10] [c02b014c] tcp_sendpage+0x6ec/0x6f4
> [23194.608125] [ccf9bd80] [c0269ec0] sock_sendpage+0x34/0x44
> [23194.608130] [ccf9bd90] [c00c6dc8] pipe_to_sendpage+0x84/0xa4
> [23194.608136] [ccf9bdc0] [c00c802c] __splice_from_pipe+0x118/0x290
> [23194.608142] [ccf9be00] [c00c8328] splice_from_pipe+0x64/0x8c
> [23194.608147] [ccf9be40] [c00c72d4] do_splice_from+0x80/0x9c
> [23194.608152] [ccf9be60] [c00c7aac] splice_direct_to_actor+0xb8/0x1e0
> [23194.608158] [ccf9bea0] [c00c7c18] do_splice_direct+0x44/0x80
> [23194.608163] [ccf9bed0] [c00a0494] do_sendfile+0x1ec/0x2b0
> [23194.608169] [ccf9bf10] [c00a0740] sys_sendfile+0xb4/0x108
> [23194.608174] [ccf9bf40] [c00124cc] ret_from_syscall+0x0/0x38

There are only a few ways this code path can happen.

1) Route changed from a device with SG+CSUM capabilities to one
   which does not have those features.  I think you are not
   hitting such a case.

2) SKB is fraglist'd, but device lacks NETIF_F_FRAGLIST.  Since
   this is TCP, this case is also exceedingly unlikely.

If the device lacks SG, the sendpage path will use
sock_no_sendpage() instead of tcp_sendpage().  Since
tcp_sendpage() is in the backtrace, whatever device this
is must have NETIF_F_SG set.

Is this from a kernel with your GSO wireless patches applied by
chance? :-)