mac80211 truesize bugs

mac80211 truesize bugs

[Johannes Berg]

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

Hi,

Whenever you run a monitor interface in mac80211, you can see lots of
truesize bugs:

SKB BUG: Invalid truesize (464) len=307, sizeof(sk_buff)=176

It appears to be caused by mac80211's re-injection of the transmitted
frame. For those not familiar, here's what happens:

When a frame comes in on say wlan0's hard_start_xmit(), it is prepared
for transmission by the code there (802.11 headers added etc.) and then
scheduled to the master interface. Once it arrives on the master
(wmaster0) interface's hard_start_xmit(), it is modified again and
finally handed to the driver.

When the driver has transmitted the frame (successfully or not) it
reports the status of the transmission to mac80211 including the skb the
driver was given. At that point, things go different depending on
circumstances.

If no monitor interfaces are present, mac80211 simply orphans the skb
and destroys it. If there are monitor interfaces, it pushes some data
into the skb (the radiotap transmit status) and hands clones of the skb
to netif_rx() for each monitor interface, or the skb itself for the last
interface in the list.

All this is in net/mac80211/main.c:ieee80211_tx_status.

Now, the thing is that the skb truesize bug ONLY occurs when the last
part here is done when a radiotap monitor interface is present, if you
add

	dev_kfree_skb(skb);
	return;

in that function somewhere before the skb_orphan() call it never
happens. Hence, I'm confused. Since I only have a single monitor
interface when this happens, it can't be due to af_packet either,
afaict.

Can anyone help me diagnose this?

johannes

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

Re: mac80211 truesize bugs

[Michael Buesch]

On Thursday 01 May 2008 04:02:48 Johannes Berg wrote:
> Hi,
> 
> Whenever you run a monitor interface in mac80211, you can see lots of
> truesize bugs:
> 
> SKB BUG: Invalid truesize (464) len=307, sizeof(sk_buff)=176
> 
> It appears to be caused by mac80211's re-injection of the transmitted
> frame. For those not familiar, here's what happens:
> 
> When a frame comes in on say wlan0's hard_start_xmit(), it is prepared
> for transmission by the code there (802.11 headers added etc.) and then
> scheduled to the master interface. Once it arrives on the master
> (wmaster0) interface's hard_start_xmit(), it is modified again and
> finally handed to the driver.
> 
> When the driver has transmitted the frame (successfully or not) it
> reports the status of the transmission to mac80211 including the skb the
> driver was given. At that point, things go different depending on
> circumstances.
> 
> If no monitor interfaces are present, mac80211 simply orphans the skb
> and destroys it. If there are monitor interfaces, it pushes some data
> into the skb (the radiotap transmit status) and hands clones of the skb
> to netif_rx() for each monitor interface, or the skb itself for the last
> interface in the list.

Hm, unrelated to this...
But I am wondering what happens if the driver adds a device header to the skb.
Is that header then also passed up netif_rx()?
This doesn't happen for b43, as we use the DMA fragmentation to transmit the header,
but it might happen for zd1211rw and others.

> All this is in net/mac80211/main.c:ieee80211_tx_status.
> 
> Now, the thing is that the skb truesize bug ONLY occurs when the last
> part here is done when a radiotap monitor interface is present, if you
> add
> 
> 	dev_kfree_skb(skb);
> 	return;
> 
> in that function somewhere before the skb_orphan() call it never
> happens. Hence, I'm confused. Since I only have a single monitor
> interface when this happens, it can't be due to af_packet either,
> afaict.
> 
> Can anyone help me diagnose this?

Seems the skb->destructor messes it up.

-- 
Greetings Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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


> Hm, unrelated to this...
> But I am wondering what happens if the driver adds a device header to the skb.
> Is that header then also passed up netif_rx()?
> This doesn't happen for b43, as we use the DMA fragmentation to transmit the header,
> but it might happen for zd1211rw and others.

Hmm. I thought we said that it was supposed to be removed again by the
hardware before TX status reporting. That's what most drivers seem to do
anyway.

I'm considering turning off this transmit status reporting for now but
that will make hostapd not work.


> Seems the skb->destructor messes it up.

Actually, it seems to be outside of mac80211, I put in a WARN_ON() and
got this:

Badness at include/linux/skbuff.h:392
NIP: c026ea14 LR: c0273d54 CTR: c026e9e4
REGS: edfc7c00 TRAP: 0700   Not tainted  (2.6.25-wl-06841-g6b3d5c6-dirty)
MSR: 00029032 <EE,ME,IR,DR>  CR: 82022444  XER: 00000000
TASK = edf50e20[3453] 'tcpdump' THREAD: edfc6000
GPR00: 00000001 edfc7cb0 edf50e20 edfd7700 edfd7700 00000002 edfc7e75 03230306 
GPR08: 02000100 00000168 4dff0200 00000150 22022442 100a6290 100783f8 10078e18 
GPR16: 10078e14 10078e10 100a0000 00000000 00000000 bfe2c9d2 1004d320 bfe2c4b0 
GPR24: 10165070 edfd7724 00000060 00000020 ed8157f0 edfd7700 ed8157f0 edfd7700 
NIP [c026ea14] sock_rfree+0x30/0x94
LR [c0273d54] skb_release_all+0x98/0x128
Call Trace:
[edfc7cb0] [10078e10] 0x10078e10 (unreliable)
[edfc7cc0] [c0273d54] skb_release_all+0x98/0x128
[edfc7cd0] [c0273034] __kfree_skb+0x18/0xc8
[edfc7ce0] [c02760d0] skb_free_datagram+0x1c/0x54
[edfc7cf0] [f264d068] packet_recvmsg+0x170/0x1e8 [af_packet]
[edfc7d40] [c026b69c] sock_recvmsg+0xb8/0xf0
[edfc7e30] [c026b9d0] sys_recvfrom+0x94/0x100
[edfc7f00] [c026ca08] sys_socketcall+0x114/0x1dc
[edfc7f40] [c00124cc] ret_from_syscall+0x0/0x38

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Thu, 01 May 2008 11:08:06 +0200

> > Seems the skb->destructor messes it up.
> 
> Actually, it seems to be outside of mac80211, I put in a WARN_ON() and
> got this:

You're just seeing who freed it last here.

It could have had it's ->truesize put into an illegal state
elsewhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Thu, 2008-05-01 at 02:20 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 01 May 2008 11:08:06 +0200
> 
> > > Seems the skb->destructor messes it up.
> > 
> > Actually, it seems to be outside of mac80211, I put in a WARN_ON() and
> > got this:
> 
> You're just seeing who freed it last here.
> 
> It could have had it's ->truesize put into an illegal state
> elsewhere.

Yes, I know, but it doesn't come from my skb_orphan() call. Hence, I
just netif_rx() the packet which makes it go onto the input_pkt_queue
and then to netif_receive_skb() which gives it to af_packet and all
others should ignore it since I set PACKET_OTHERHOST.

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 01 May 2008 11:32:29 +0200

> On Thu, 2008-05-01 at 02:20 -0700, David Miller wrote:
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Thu, 01 May 2008 11:08:06 +0200
> > 
> > > > Seems the skb->destructor messes it up.
> > > 
> > > Actually, it seems to be outside of mac80211, I put in a WARN_ON() and
> > > got this:
> > 
> > You're just seeing who freed it last here.
> > 
> > It could have had it's ->truesize put into an illegal state
> > elsewhere.
> 
> Yes, I know, but it doesn't come from my skb_orphan() call. Hence, I
> just netif_rx() the packet which makes it go onto the input_pkt_queue
> and then to netif_receive_skb() which gives it to af_packet and all
> others should ignore it since I set PACKET_OTHERHOST.

I looked at the mac80211 code, the problem is the skb_push() you
guys do in this situation.

Things like loopback, which also orphan then reinject, don't trigger
this problem because the re-input path trims things, never adds.

The good news is that this is easy to fix.

Since you've orphaned the SKB, simply adjust skb->truesize as you
do pushes.  Like this:

mac80211: Adjust truesize in ieee80211_tx_status() when reinjecting.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 9ad4e36..de2e904 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1485,6 +1485,9 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
 	rthdr = (struct ieee80211_tx_status_rtap_hdr*)
 				skb_push(skb, sizeof(*rthdr));
 
+	/* This is safe because the buffer has been orphaned.  */
+	skb->truesize += sizeof(*rthdr);
+
 	memset(rthdr, 0, sizeof(*rthdr));
 	rthdr->hdr.it_len = cpu_to_le16(sizeof(*rthdr));
 	rthdr->hdr.it_present =

Re: mac80211 truesize bugs

[Johannes Berg]

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


> I looked at the mac80211 code, the problem is the skb_push() you
> guys do in this situation.

Thanks.

> Things like loopback, which also orphan then reinject, don't trigger
> this problem because the re-input path trims things, never adds.
> 
> The good news is that this is easy to fix.
> 
> Since you've orphaned the SKB, simply adjust skb->truesize as you
> do pushes.  Like this:
> 
> mac80211: Adjust truesize in ieee80211_tx_status() when reinjecting.
> 
> Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
 
> +	/* This is safe because the buffer has been orphaned.  */
> +	skb->truesize += sizeof(*rthdr);

Hmm. The disconnect between truesize and skb->len+sizeof(*skb) was
usually 17 or 19 bytes and sizeof(*rthdr) is only 11. On the other hand,
I don't see where the other bytes should be coming from. I'll give this
a try, thanks.

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 01 May 2008 11:48:19 +0200

> > Signed-off-by: David S. Miller <davem@davemloft.net>
>  
> > +	/* This is safe because the buffer has been orphaned.  */
> > +	skb->truesize += sizeof(*rthdr);
> 
> Hmm. The disconnect between truesize and skb->len+sizeof(*skb) was
> usually 17 or 19 bytes and sizeof(*rthdr) is only 11. On the other hand,
> I don't see where the other bytes should be coming from. I'll give this
> a try, thanks.

Grrr, I bet it's coming from a combination of the
skb_set_mac_header(skb, 0); call done by mac80211 and the skb_push()
calls in net/packet/af_packet.c

davem@sunset:~/src/GIT/net-2.6$ egrep skb_push net/packet/af_packet.c
	skb_push(skb, skb->data - skb_mac_header(skb));
			skb_push(skb, skb->data - skb_mac_header(skb));
			skb_push(skb, skb->data - skb_mac_header(skb));
davem@sunset:~/src/GIT/net-2.6$ 

Re: mac80211 truesize bugs

[Johannes Berg]

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


> > Hmm. The disconnect between truesize and skb->len+sizeof(*skb) was
> > usually 17 or 19 bytes and sizeof(*rthdr) is only 11. On the other hand,
> > I don't see where the other bytes should be coming from. I'll give this
> > a try, thanks.

Even when I explicitly set truesize (rather than adjusting it as you
did) I still get a disconnect.

> Grrr, I bet it's coming from a combination of the
> skb_set_mac_header(skb, 0); call done by mac80211 and the skb_push()
> calls in net/packet/af_packet.c
> 
> davem@sunset:~/src/GIT/net-2.6$ egrep skb_push net/packet/af_packet.c
> 	skb_push(skb, skb->data - skb_mac_header(skb));
> 			skb_push(skb, skb->data - skb_mac_header(skb));
> 			skb_push(skb, skb->data - skb_mac_header(skb));

But mac80211 does set_mac_header(0) so this should just push zero bytes,
no?

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 01 May 2008 12:08:14 +0200

> > Grrr, I bet it's coming from a combination of the
> > skb_set_mac_header(skb, 0); call done by mac80211 and the skb_push()
> > calls in net/packet/af_packet.c
> > 
> > davem@sunset:~/src/GIT/net-2.6$ egrep skb_push net/packet/af_packet.c
> > 	skb_push(skb, skb->data - skb_mac_header(skb));
> > 			skb_push(skb, skb->data - skb_mac_header(skb));
> > 			skb_push(skb, skb->data - skb_mac_header(skb));
> 
> But mac80211 does set_mac_header(0) so this should just push zero bytes,
> no?

Right you are.

So, I wonder what's causing the problem...  Could you "remember" the
length and truesize at the skb_orphan() point in mac80211, right
after the skb_push(), then in the truesize warning, print those
"remembered" values as well as the current ones.

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Thu, 2008-05-01 at 03:32 -0700, David Miller wrote:

> Right you are.
> 
> So, I wonder what's causing the problem...  Could you "remember" the
> length and truesize at the skb_orphan() point in mac80211, right
> after the skb_push(), then in the truesize warning, print those
> "remembered" values as well as the current ones.

I was just playing with af_packet and added some debugging there that
prints out the len of all packets it gets (for a certain ifidx)

That's confusing me even more now. I get

[ 7650.792004] packet_recv eda9e8c0 (len=137)
[ 7650.792015] snaplen(eda9e8c0)=137
[ 7650.792027] free eda9e8c0, len = 137
[ 7650.792031] new skb: eda9e540
[ 7650.792039] packet_recv eda9e8c0 (len=137)
[ 7650.792044] snaplen(eda9e8c0)=137
[ 7650.792048] new skb: eda9e8c0
[ 7650.819464] packet_recv d1f4e9a0 (len=124)
[ 7650.819478] snaplen(d1f4e9a0)=124
[ 7650.819489] free d1f4e9a0, len = 124
[ 7650.819493] new skb: d1f4e8c0
[ 7650.819502] packet_recv d1f4e9a0 (len=124)
[ 7650.819507] snaplen(d1f4e9a0)=124
[ 7650.819511] new skb: d1f4e9a0
[ 7651.215631] packet_recv e9ecc2a0 (len=376)
[ 7651.215645] snaplen(e9ecc2a0)=376
[ 7651.215657] free e9ecc2a0, len = 376
[ 7651.215662] new skb: ede04b60
[ 7651.215671] packet_recv e9ecc2a0 (len=376)
[ 7651.215675] snaplen(e9ecc2a0)=376
[ 7651.215680] new skb: e9ecc2a0

[ 7651.760751] SKB BUG: Invalid truesize (528) len=357, sizeof(sk_buff)=176


528-176 is 352 which doesn't occur in that list... Maybe I should print
it in mac80211.


johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> 
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index 9ad4e36..de2e904 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -1485,6 +1485,9 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
>        rthdr = (struct ieee80211_tx_status_rtap_hdr*)
>                                skb_push(skb, sizeof(*rthdr));
> ^M
> +       /* This is safe because the buffer has been orphaned.  */
> +       skb->truesize += sizeof(*rthdr);

skb->truesize should always account the skb->head area in its
entirety so we should never need to adjust it when pushing or
pulling.  So I suggest we find the place that expanded the head
area and make the adjustment there.  Alternative we could adjust
it right after the orphan call if the expansion occurs where we
can't adjust the truesize.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 01 May 2008 18:36:49 +0800

> David Miller <davem@davemloft.net> wrote:
> > +       /* This is safe because the buffer has been orphaned.  */
> > +       skb->truesize += sizeof(*rthdr);
> 
> skb->truesize should always account the skb->head area in its
> entirety so we should never need to adjust it when pushing or
> pulling.  So I suggest we find the place that expanded the head
> area and make the adjustment there.  Alternative we could adjust
> it right after the orphan call if the expansion occurs where we
> can't adjust the truesize.

That makes more sense, good catch Herbert.

I guess it's the pskb_expand_head() calls done by net/mac80211/tx.c
I suspect we'll need to orphan early in order to accomodate these
adjustments, otherwise socket memory buffer allocations will
be corrupted.

Once that is cured, I think we can detect this better, by adding a
carefully constructed assertion to pskb_expand_head().  Basically, the
idea is, if "nhead" or "ntail" are non-zero, and there is a socket
still attached to the SKB, print a warning message.

Something like:

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4fe605f..9bfca08 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -699,6 +699,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	if (skb_shared(skb))
 		BUG();
 
+	if (unlikely((nhead || ntail) && skb->sk)) {
+		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
+		       "with socket attached\n",
+		       nhead, ntail);
+	}
+
 	size = SKB_DATA_ALIGN(size);
 
 	data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);

Re: mac80211 truesize bugs

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 01 May 2008 03:49:50 -0700 (PDT)

> I guess it's the pskb_expand_head() calls done by net/mac80211/tx.c
> I suspect we'll need to orphan early in order to accomodate these
> adjustments, otherwise socket memory buffer allocations will
> be corrupted.

I think the mac80211 encryption paths on TX could be doing this as
well, just something I noticed meanwhile.

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Thu, 2008-05-01 at 03:53 -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Thu, 01 May 2008 03:49:50 -0700 (PDT)
> 
> > I guess it's the pskb_expand_head() calls done by net/mac80211/tx.c
> > I suspect we'll need to orphan early in order to accomodate these
> > adjustments, otherwise socket memory buffer allocations will
> > be corrupted.
> 
> I think the mac80211 encryption paths on TX could be doing this as
> well, just something I noticed meanwhile.

Indeed. But then why did we never see this bug w/o monitor interfaces
and this reinjection?

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Thu, May 01, 2008 at 12:58:20PM +0200, Johannes Berg wrote:
>
> Indeed. But then why did we never see this bug w/o monitor interfaces
> and this reinjection?

The debugging only catches it if the expanded area actually
gets used, e.g., by skb_push.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Thu, 2008-05-01 at 19:03 +0800, Herbert Xu wrote:
> On Thu, May 01, 2008 at 12:58:20PM +0200, Johannes Berg wrote:
> >
> > Indeed. But then why did we never see this bug w/o monitor interfaces
> > and this reinjection?
> 
> The debugging only catches it if the expanded area actually
> gets used, e.g., by skb_push.

I'm confused. The area should be used say with encryption when it's
actually necessary. Maybe there's always enough headroom for some reason
now?

On another note, why is this truesize mismatch a bug anyway? I mean, the
field could just be called "socket_charged_size" and simply be required
to have the same value throughout the skb lifetime, the slight mismatch
between charged bytes and actually used bytes wouldn't usually matter
too much, would it?

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Fri, 02 May 2008 22:38:51 +0200

> I'm confused. The area should be used say with encryption when it's
> actually necessary. Maybe there's always enough headroom for some
> reason now?

That's possible.

There are paths, that, although they could take advantage of the
fact that all of the non-header packet data is going to come from
pages, they don't and allocate a full MTU skb->data area anyways.

> On another note, why is this truesize mismatch a bug anyway? I mean, the
> field could just be called "socket_charged_size" and simply be required
> to have the same value throughout the skb lifetime, the slight mismatch
> between charged bytes and actually used bytes wouldn't usually matter
> too much, would it?

Sure the name could change, but the problem wouldn't.

We can't update skb->truesize during arbitray skb->data reallocations,
because it could corrupt the socket accounting.

On the other hand, if we provide ways for users to subvert the socket
buffer limits, we might as well not try to limit anything.

Take a look at some ethernet drivers that implement TSO in a way that
requires munging the IP headers for whatever reason.  If they need to
COW the packet data in order to modify it, they always do this with
pskb_expand_head(skb, 0, 0, GFP_*) exactly so that they don't modify
the SKB data size, and exactly so that the skb->truesize value stays
accurate.

Try to find out exactly what's going wrong here, you seem to be
close.  Once we know the precise issue we can talk about real
changes to make this easier to cope with and debug in the future.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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


> > I'm confused. The area should be used say with encryption when it's
> > actually necessary. Maybe there's always enough headroom for some
> > reason now?
> 
> That's possible.
> 
> There are paths, that, although they could take advantage of the
> fact that all of the non-header packet data is going to come from
> pages, they don't and allocate a full MTU skb->data area anyways.

Ok.

> On the other hand, if we provide ways for users to subvert the socket
> buffer limits, we might as well not try to limit anything.

Well, not exactly, since we'd only do that (at least in mac80211) when
packets are about to be sent to the hardware so they wouldn't live much
longer.

> Take a look at some ethernet drivers that implement TSO in a way that
> requires munging the IP headers for whatever reason.  If they need to
> COW the packet data in order to modify it, they always do this with
> pskb_expand_head(skb, 0, 0, GFP_*) exactly so that they don't modify
> the SKB data size, and exactly so that the skb->truesize value stays
> accurate.

Right, but we might actually need more space. Say you have a device that
requires 82 bytes headroom (yes, there are such devices) for their own
transmit header. Then you need maybe up to 30 bytes of 802.11 header
plus 8 byte ICV, so minus the 14 ethernet header that we remove we now
need well over 100 bytes headroom. On the other hand, not even
accounting the actual data buffer (you proposed to skb_orphan the skb
early) seems wrong as well. Worse yet, the needed transmit header
headroom is variable depending on devices.

One of the worst devices is the Broadcom one with 82 header and nowadays
actually DMAs this header from a separate memory location, so there this
won't happen, but can we guarantee that all devices are programmable
that way? We've seen lots of rather strange devices unfortunately...

> Try to find out exactly what's going wrong here, you seem to be
> close.  Once we know the precise issue we can talk about real
> changes to make this easier to cope with and debug in the future.

I'll try. I don't really see myself being that close ;)

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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


> One of the worst devices is the Broadcom one with 82 header and nowadays
> actually DMAs this header from a separate memory location, so there this
> won't happen, but can we guarantee that all devices are programmable
> that way? We've seen lots of rather strange devices unfortunately...

The worst case is probably prism54 usb devices which by itself need 76
bytes headroom for the USB buffer, and then when we say run mesh on top
of it we'll need a total of 122 bytes. Needless to say, it cannot do s/g
operation.

The question is: how do we handle that? Do we reallocate the buffer in
the driver? That is well possible but makes it rather inconvenient for
driver authors. Also, mac80211 will still need 46 bytes of headroom and
12 bytes of tailroom in the worst case (so far, HT might require four
more.) If we skb_orphan() the skb right away we have essentially removed
all socket memory accounting, so that's pretty pointless.

Should we increase the LL_MAX_HEADER constant to 40 (no mesh networking)
or 46 for when 802.11 (with mesh networking) is configured into the
kernel? Most people probably don't run an IPIP tunnel over wireless yet
configure them in (especially distros) so that might be why we never saw
the problem before.

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Sat, 03 May 2008 16:25:20 +0200

[ Ok, I'm going through all of this, we'll solve the problems one at
  a time and get this all taken care of.  ]

> The worst case is probably prism54 usb devices which by itself need 76
> bytes headroom for the USB buffer, and then when we say run mesh on top
> of it we'll need a total of 122 bytes. Needless to say, it cannot do s/g
> operation.
 ...
> Should we increase the LL_MAX_HEADER constant to 40 (no mesh networking)
> or 46 for when 802.11 (with mesh networking) is configured into the
> kernel? Most people probably don't run an IPIP tunnel over wireless yet
> configure them in (especially distros) so that might be why we never saw
> the problem before.

Thanks for these numbers.

Based upon them I will adjust LL_MAX_HEADER as follows:

net: Set LL_MAX_HEADER properly for wireless.

Wireless networking, particularly with MESH enabled, has
quite strong requirements for link-layer header space.

Based upon some numbers and descriptions from Johannes Berg
we use 96 (same as AX25) for plain wireless, and with
mesh enabled we use 128.

In the process, simplify the cpp conditional logic here by
ordering the cases by those needing the most space down
to those needing the least case.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/linux/netdevice.h |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7469017..a3fb57f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -93,14 +93,16 @@ struct wireless_dev;
  *	used.
  */
  
-#if !defined(CONFIG_AX25) && !defined(CONFIG_AX25_MODULE) && !defined(CONFIG_TR)
-#define LL_MAX_HEADER	32
+#if defined(CONFIG_WLAN_80211) || defined(CONFIG_AX25) || defined(CONFIG_AX25_MODULE)
+# if defined(CONFIG_MAC80211_MESH)
+#  define LL_MAX_HEADER 128
+# else
+#  define LL_MAX_HEADER 96
+# endif
+#elif defined(CONFIG_TR)
+# define LL_MAX_HEADER 48
 #else
-#if defined(CONFIG_AX25) || defined(CONFIG_AX25_MODULE)
-#define LL_MAX_HEADER	96
-#else
-#define LL_MAX_HEADER	48
-#endif
+# define LL_MAX_HEADER 32
 #endif
 
 #if !defined(CONFIG_NET_IPIP) && !defined(CONFIG_NET_IPIP_MODULE) && \
-- 
1.5.5.1.57.g5909c

Re: mac80211 truesize bugs

[John W. Linville]

On Mon, May 12, 2008 at 08:17:51PM -0700, David Miller wrote:

> Based upon them I will adjust LL_MAX_HEADER as follows:

> @@ -93,14 +93,16 @@ struct wireless_dev;
>   *	used.
>   */
>   
> -#if !defined(CONFIG_AX25) && !defined(CONFIG_AX25_MODULE) && !defined(CONFIG_TR)
> -#define LL_MAX_HEADER	32
> +#if defined(CONFIG_WLAN_80211) || defined(CONFIG_AX25) || defined(CONFIG_AX25_MODULE)

Is WLAN_80211 really the right symbol here?  It applies to more than
just mac80211, and I doubt if full MAC devices need higher values.
It seems like MAC80211 would be the right symbol to add.

John
-- 
John W. Linville
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Tue, 2008-05-13 at 16:39 -0400, John W. Linville wrote:
> On Mon, May 12, 2008 at 08:17:51PM -0700, David Miller wrote:
> 
> > Based upon them I will adjust LL_MAX_HEADER as follows:
> 
> > @@ -93,14 +93,16 @@ struct wireless_dev;
> >   *	used.
> >   */
> >   
> > -#if !defined(CONFIG_AX25) && !defined(CONFIG_AX25_MODULE) && !defined(CONFIG_TR)
> > -#define LL_MAX_HEADER	32
> > +#if defined(CONFIG_WLAN_80211) || defined(CONFIG_AX25) || defined(CONFIG_AX25_MODULE)
> 
> Is WLAN_80211 really the right symbol here?  It applies to more than
> just mac80211, and I doubt if full MAC devices need higher values.
> It seems like MAC80211 would be the right symbol to add.

Most fullmac devices also need some space, whether they do things that
way or not is a completely different thing, the ieee80211 stack for
example copies every packet into a new skb anyway.

johannes

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

Re: mac80211 truesize bugs

[Tomas Winkler]

On Tue, May 13, 2008 at 11:59 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2008-05-13 at 16:39 -0400, John W. Linville wrote:
>  > On Mon, May 12, 2008 at 08:17:51PM -0700, David Miller wrote:
>  >
>  > > Based upon them I will adjust LL_MAX_HEADER as follows:
>  >
>  > > @@ -93,14 +93,16 @@ struct wireless_dev;
>  > >   * used.
>  > >   */
>  > >
>  > > -#if !defined(CONFIG_AX25) && !defined(CONFIG_AX25_MODULE) && !defined(CONFIG_TR)
>  > > -#define LL_MAX_HEADER      32
>  > > +#if defined(CONFIG_WLAN_80211) || defined(CONFIG_AX25) || defined(CONFIG_AX25_MODULE)
>  >
>  > Is WLAN_80211 really the right symbol here?  It applies to more than
>  > just mac80211, and I doubt if full MAC devices need higher values.
>  > It seems like MAC80211 would be the right symbol to add.
>
>  Most fullmac devices also need some space, whether they do things that
>  way or not is a completely different thing, the ieee80211 stack for
>  example copies every packet into a new skb anyway.

It's really depends on when is the 802.3 to 802.11 header translation
made it doesn't matter whether it's fullmac or softmac. I would say
that WLAN is OK.

Tomas

Re: mac80211 truesize bugs

[Johannes Berg]

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


> >  Most fullmac devices also need some space, whether they do things that
> >  way or not is a completely different thing, the ieee80211 stack for
> >  example copies every packet into a new skb anyway.
> 
> It's really depends on when is the 802.3 to 802.11 header translation
> made it doesn't matter whether it's fullmac or softmac. I would say
> that WLAN is OK.

True, some devices take 802.3 frames and do the translation and
everything themselves. I dunno. I guess it would make sense to make it
mac80211 anyway because ieee80211 doesn't care and the other drivers are
probably handling this in some other way anyway (if maybe even by using
different dma descriptors, or just not caring because transfer speeds
are low enough)

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Tue, 13 May 2008 23:37:23 +0200

> 
> > >  Most fullmac devices also need some space, whether they do things that
> > >  way or not is a completely different thing, the ieee80211 stack for
> > >  example copies every packet into a new skb anyway.
> > 
> > It's really depends on when is the 802.3 to 802.11 header translation
> > made it doesn't matter whether it's fullmac or softmac. I would say
> > that WLAN is OK.
> 
> True, some devices take 802.3 frames and do the translation and
> everything themselves. I dunno. I guess it would make sense to make it
> mac80211 anyway because ieee80211 doesn't care and the other drivers are
> probably handling this in some other way anyway (if maybe even by using
> different dma descriptors, or just not caring because transfer speeds
> are low enough)

We should fix ieee80211 (it's something I planned to look into today
in fact), and the drivers that copy or do things incorrectly.

It doesn't make sense to make this decision based upon which 80211
stack or which drivers are enabled.  All of them in essence want this
extra space, so let's give it to them.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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


> We can't update skb->truesize during arbitray skb->data reallocations,
> because it could corrupt the socket accounting.
> 
> On the other hand, if we provide ways for users to subvert the socket
> buffer limits, we might as well not try to limit anything.

Why don't we update the socket allocation when doing pskb_expand_head()?
Sure, it could become negative, but is that so bad?

> Take a look at some ethernet drivers that implement TSO in a way that
> requires munging the IP headers for whatever reason.  If they need to
> COW the packet data in order to modify it, they always do this with
> pskb_expand_head(skb, 0, 0, GFP_*) exactly so that they don't modify
> the SKB data size, and exactly so that the skb->truesize value stays
> accurate.

We need more space though. Should we then just increase the built-in
headroom?

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Sat, 03 May 2008 13:52:13 +0200

> 
> > We can't update skb->truesize during arbitray skb->data reallocations,
> > because it could corrupt the socket accounting.
> > 
> > On the other hand, if we provide ways for users to subvert the socket
> > buffer limits, we might as well not try to limit anything.
> 
> Why don't we update the socket allocation when doing pskb_expand_head()?
> Sure, it could become negative, but is that so bad?

The socket locked state at this time is variable and unknown.

The socket must be locked in order to modify these values.
And such locks cannot be taken, for example, from HW interrupt
context, amongst other restrictions.

> We need more space though. Should we then just increase the built-in
> headroom?

I simply don't know what to suggest at this point, that's why
we are having this discussion :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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


> > Why don't we update the socket allocation when doing pskb_expand_head()?
> > Sure, it could become negative, but is that so bad?
> 
> The socket locked state at this time is variable and unknown.
> 
> The socket must be locked in order to modify these values.
> And such locks cannot be taken, for example, from HW interrupt
> context, amongst other restrictions.

Ok, that makes sense.

> > We need more space though. Should we then just increase the built-in
> > headroom?
> 
> I simply don't know what to suggest at this point, that's why
> we are having this discussion :-)

I'm still not sure about the dependencies between LL_MAX_HEADER,
dev->hard_header_len and similar. Why, for example, does ipip set it to
LL_MAX_HEADER + sizeof(struct iphdr)? Because it doesn't know better
since the packets it creates could be routed anywhere?

Could mac80211 announce it needs a very long hard_header_len (say 48 (or
54) bytes)? Am I right in thinking that then we'd have to increase
LL_MAX_HEADER as well? I haven't found a check somewhere that warns you
if you set dev->hard_header_len > LL_MAX_HEADER, should there be one?

If I increase dev->hard_header_len, will that have any negative impact
on the caching since I'm still just using regular ethernet headers?


As far as I understand we have a few options:

 (a) go along with it as we do now, use pskb_expand_head, just call
     skb_orphan before. I assume this has a number of requirements just
     like sock size accounting would have, does this work from within a
     hard_start_xmit path? I haven't seen any problems with it so far
     anyway.
 (b) clone the skb and free the original. pretty much equivalent
 (c) increase hard_header_len/LL_MAX_HEADER constants to 48 (54).

Options (a) and (b) make the accounting pretty useless since that would
drop the charge to the socket quite early. (c) doesn't seem to work, I
tried just increasing LL_MAX_HEADER doesn't seem to help although
MAX_TCP_HEADER suggests it should be getting enough headroom then.

Ideally, we'd have enough headroom in the skb to start with, since right
now we're apparently reallocating a lot, especially encrypted frames.
Not that I understand why we don't get a truesize bug (without the
monitors) when we do that.

With smart hardware like b43 we could even think about putting the
802.11 header stuff into a separate buffer and have the hardware to
gather-dma but there are so many dumb usb devices that this won't help
much anyway.

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Sun, May 04, 2008 at 03:42:34AM +0200, Johannes Berg wrote:
>
> I'm still not sure about the dependencies between LL_MAX_HEADER,
> dev->hard_header_len and similar. Why, for example, does ipip set it to
> LL_MAX_HEADER + sizeof(struct iphdr)? Because it doesn't know better
> since the packets it creates could be routed anywhere?

The difference between LL_MAX_HEADER and hard_header_len is that
the former is a system-wide hint of how big the header can be
and the latter is a device-specific value.

In other words, we use the former when we don't know where we
create a packet since we don't know where it'll end up.  As such
we allocate a generous amount of header space so that hopefully
nobody will have to expand the header later.  However, it would
be OK for someone to expand it but obviously it this happened
for the majority of your traffic then you've done something wrong
since expanding it wastes CPU resources by copying the data.

The value of hard_header_len on the other hand is used for two
purposes at least.  First of all it is a hint of how much free
header space there should be in a packet before it goes into a
device (but don't rely on it).  More importantly it's the length
of the hardware header in the packet.  The definition of the
hardware header varies with each type of device.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 10:02 +0800, Herbert Xu wrote:
> On Sun, May 04, 2008 at 03:42:34AM +0200, Johannes Berg wrote:
> >
> > I'm still not sure about the dependencies between LL_MAX_HEADER,
> > dev->hard_header_len and similar. Why, for example, does ipip set it to
> > LL_MAX_HEADER + sizeof(struct iphdr)? Because it doesn't know better
> > since the packets it creates could be routed anywhere?
> 
> The difference between LL_MAX_HEADER and hard_header_len is that
> the former is a system-wide hint of how big the header can be
> and the latter is a device-specific value.
> 
> In other words, we use the former when we don't know where we
> create a packet since we don't know where it'll end up.  As such
> we allocate a generous amount of header space so that hopefully
> nobody will have to expand the header later.  However, it would
> be OK for someone to expand it but obviously it this happened
> for the majority of your traffic then you've done something wrong
> since expanding it wastes CPU resources by copying the data.
> 
> The value of hard_header_len on the other hand is used for two
> purposes at least.  First of all it is a hint of how much free
> header space there should be in a packet before it goes into a
> device (but don't rely on it).  More importantly it's the length
> of the hardware header in the packet.  The definition of the
> hardware header varies with each type of device.

Ok, thanks for the explanation. The latter probably means that I cannot
use hard_header_len since mac80211 really needs devices to show up as
ethernet devices. However, it wouldn't really help anyway if I cannot
rely on it being available.

So, what is mac80211 supposed to do? It needs up to 54 bytes of
available headroom (for an encrypted mesh packet which currently can't
really happen, but anyway) yet it cannot pskb_expand_head() either.
Cloning each packet seems even more expensive, and just like skb_orphan
subverts the purpose of the socket accounting.

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Sun, May 04, 2008 at 04:08:36AM +0200, Johannes Berg wrote:
>
> So, what is mac80211 supposed to do? It needs up to 54 bytes of
> available headroom (for an encrypted mesh packet which currently can't
> really happen, but anyway) yet it cannot pskb_expand_head() either.
> Cloning each packet seems even more expensive, and just like skb_orphan
> subverts the purpose of the socket accounting.

If all/the majority of your packets need the space then put it in
LL_MAX_HEADER.  In any case, you should always expand the packet
if necessary in your output routine since LL_MAX_HEADER is just a
hint.  Yes cloning is expensive compared to not having to do it,
but as long as this is only done for the exception then it's
irrelevant.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 10:12 +0800, Herbert Xu wrote:
> On Sun, May 04, 2008 at 04:08:36AM +0200, Johannes Berg wrote:
> >
> > So, what is mac80211 supposed to do? It needs up to 54 bytes of
> > available headroom (for an encrypted mesh packet which currently can't
> > really happen, but anyway) yet it cannot pskb_expand_head() either.
> > Cloning each packet seems even more expensive, and just like skb_orphan
> > subverts the purpose of the socket accounting.
> 
> If all/the majority of your packets need the space then put it in
> LL_MAX_HEADER.  

Yes, wireless always needs at least 24 bytes, but more likely 34
(encryption+QoS). However, I just increased LL_MAX_HEADER to 54 and that
doesn't seem to have helped.

> In any case, you should always expand the packet
> if necessary in your output routine since LL_MAX_HEADER is just a
> hint.  Yes cloning is expensive compared to not having to do it,
> but as long as this is only done for the exception then it's
> irrelevant.

What's wrong with, instead, doing skb_orphan() and then
pskb_expand_head()? That seems to have the same effect.

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Sun, May 04, 2008 at 04:22:20AM +0200, Johannes Berg wrote:
>
> Yes, wireless always needs at least 24 bytes, but more likely 34
> (encryption+QoS). However, I just increased LL_MAX_HEADER to 54 and that
> doesn't seem to have helped.

How did you test it?

> What's wrong with, instead, doing skb_orphan() and then
> pskb_expand_head()? That seems to have the same effect.

If you packet sticks around for long enough then this skews the
accounting.  In any case, thinking too much about optimising this
part is a waste of time because we should be thinking about having
enough head room in the packet so that we don't have to expand
it in the first place except for the odd packet.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 11:16 +0800, Herbert Xu wrote:
> On Sun, May 04, 2008 at 04:22:20AM +0200, Johannes Berg wrote:
> >
> > Yes, wireless always needs at least 24 bytes, but more likely 34
> > (encryption+QoS). However, I just increased LL_MAX_HEADER to 54 and that
> > doesn't seem to have helped.
> 
> How did you test it?

I stuck a WARN_ON((nhead || ntail) && skb->sk) into pskb_expand_head
(which never triggered except from mac80211). And mac80211 has code that
calculates the required header length and only calls pskb_expand_head()
from that place when it needs more header space (or the skb is cloned. I
should repeat this test)

> > What's wrong with, instead, doing skb_orphan() and then
> > pskb_expand_head()? That seems to have the same effect.
> 
> If you packet sticks around for long enough then this skews the
> accounting.  In any case, thinking too much about optimising this
> part is a waste of time because we should be thinking about having
> enough head room in the packet so that we don't have to expand
> it in the first place except for the odd packet.

Very true. How about tail length? :)

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 10:47 +0200, Johannes Berg wrote:
> On Sun, 2008-05-04 at 11:16 +0800, Herbert Xu wrote:
> > On Sun, May 04, 2008 at 04:22:20AM +0200, Johannes Berg wrote:
> > >
> > > Yes, wireless always needs at least 24 bytes, but more likely 34
> > > (encryption+QoS). However, I just increased LL_MAX_HEADER to 54 and that
> > > doesn't seem to have helped.
> > 
> > How did you test it?
> 
> I stuck a WARN_ON((nhead || ntail) && skb->sk) into pskb_expand_head
> (which never triggered except from mac80211). And mac80211 has code that
> calculates the required header length and only calls pskb_expand_head()
> from that place when it needs more header space (or the skb is cloned. I
> should repeat this test)

I just re-did the test, and I definitely need 29 more bytes on, for
example, the IPv6 autodiscovery packets and ICMP packets generated with
ping(8). Some of them even need additional tailroom.

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Sun, May 04, 2008 at 11:14:49AM +0200, Johannes Berg wrote:
>
> I just re-did the test, and I definitely need 29 more bytes on, for
> example, the IPv6 autodiscovery packets and ICMP packets generated with
> ping(8). Some of them even need additional tailroom.

Could you find out the code path that created the skb in the
kernel so we can see why there isn't enough head room?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 17:44 +0800, Herbert Xu wrote:
> On Sun, May 04, 2008 at 11:14:49AM +0200, Johannes Berg wrote:
> >
> > I just re-did the test, and I definitely need 29 more bytes on, for
> > example, the IPv6 autodiscovery packets and ICMP packets generated with
> > ping(8). Some of them even need additional tailroom.
> 
> Could you find out the code path that created the skb in the
> kernel so we can see why there isn't enough head room?

Yeah I was just considering putting a stacktrace into the skb when it's
allocated. Haven't done, lunchtime now.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 11:52 +0200, Johannes Berg wrote:
> On Sun, 2008-05-04 at 17:44 +0800, Herbert Xu wrote:
> > On Sun, May 04, 2008 at 11:14:49AM +0200, Johannes Berg wrote:
> > >
> > > I just re-did the test, and I definitely need 29 more bytes on, for
> > > example, the IPv6 autodiscovery packets and ICMP packets generated with
> > > ping(8). Some of them even need additional tailroom.
> > 
> > Could you find out the code path that created the skb in the
> > kernel so we can see why there isn't enough head room?
> 
> Yeah I was just considering putting a stacktrace into the skb when it's
> allocated. Haven't done, lunchtime now.

http://johannes.sipsolutions.net/patches/kernel/all/2008-05-04-11%3a23/027-skb-alloc-stackdump.patch

will report results after kernel rebuild.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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


> http://johannes.sipsolutions.net/patches/kernel/all/2008-05-04-11%3a23/027-skb-alloc-stackdump.patch

This was broken when cloning, this one works:

http://johannes.sipsolutions.net/patches/kernel/all/2008-05-04-12%3a19/027-skb-alloc-stackdump.patch

Now I see the problem. I increased the LL_MAX_HEADER constant, but all
code uses dev->hard_header_len to allocate the headroom (via
LL_RESERVED_SPACE), e.g. packet.c:

packet_sendmsg_spkt:
        skb = sock_wmalloc(sk, len + LL_RESERVED_SPACE(dev), 0, GFP_KERNEL);  
   
[...]
        /* FIXME: Save some space for broken drivers that write a
         * hard header at transmission time by themselves. PPP is the
         * notable one here. This should really be fixed at the driver level.
         */
        skb_reserve(skb, LL_RESERVED_SPACE(dev));

This one even complains about "broken drivers" (like PPP, but wireless
code behaves like this too). This is getting really really really
frustrating. All kinds of comments all over tell you how this all is
wrong but NEVER actually tell you how to do it correctly!

As far as I understand, I cannot change dev->hard_header_len because I
want/need an ethernet header and not more. But then I don't get enough
headroom.

Oddly enough, I even get a warning from tcp_connect() although that
actually does
        /* Reserve space for headers. */
        skb_reserve(buff, MAX_TCP_HEADER);

which should definitely be sufficient. Maybe the packet gets cloned
somewhere? On the other hand, IPv4 raw's raw_send_hdrinc reserves
LL_RESERVED_SPACE(rt->u.dst.dev) like af_packet....

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Sun, May 04, 2008 at 02:28:08PM +0200, Johannes Berg wrote:
>
> Now I see the problem. I increased the LL_MAX_HEADER constant, but all
> code uses dev->hard_header_len to allocate the headroom (via
> LL_RESERVED_SPACE), e.g. packet.c:

Right hard_header_len is being overloaded with two meanings.
On the one hand it's used to indicate the length of the header
managed by the hardware header cache, on the other hand we use
it to indicate the head room for those devices that don't have
a cache.

Since your device wants both, it fails miserably.

There are three ways out of this.  You could have your own cache
(I haven't looked at your headers so I don't know whether that
makes sense), have no cache at all, or add a new field that gets
added to hard_header_len for the purposes of calculating head
room.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: mac80211 truesize bugs

[Johannes Berg]

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


> Right hard_header_len is being overloaded with two meanings.
> On the one hand it's used to indicate the length of the header
> managed by the hardware header cache, on the other hand we use
> it to indicate the head room for those devices that don't have
> a cache.
> 
> Since your device wants both, it fails miserably.

Yeah I guess that is actually the root cause.

> There are three ways out of this.  You could have your own cache
> (I haven't looked at your headers so I don't know whether that
> makes sense), have no cache at all, or add a new field that gets
> added to hard_header_len for the purposes of calculating head
> room.

In fact, I should probably add another field for the needed tail length
too since we need that as well.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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


> There are three ways out of this.  You could have your own cache
> (I haven't looked at your headers so I don't know whether that
> makes sense)

This doesn't really make all that much sense since 802.11 has not only
variable length headers but also needs a fixed BSSID in the header and
is pretty much intermingled with 802.3.

> have no cache at all,

That wouldn't be good either I think since we can actually use the
ethernet header quite well.

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Sun, May 04, 2008 at 02:52:41PM +0200, Johannes Berg wrote:
>
> > have no cache at all,
> 
> That wouldn't be good either I think since we can actually use the
> ethernet header quite well.

Actually this is not as bad as it sounds.  Since you're constructing
the hardware header anyway, it may turn out to be easier to tack on
the Ethernet header while you're at it.

Please note that the hardware cache is not the same as the neighbour
cache.  Just because you lose the hardware cache doesn't mean that
you'd lose the neighbour (ARP) cache.

However, this would mean that you'd no longer have an Ethernet device
which is both good and bad.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 20:56 +0800, Herbert Xu wrote:

> Actually this is not as bad as it sounds.  Since you're constructing
> the hardware header anyway, it may turn out to be easier to tack on
> the Ethernet header while you're at it.

We actuall remove the ethernet header and rewrite it to a hardware
header as the 802.11 spec pretty much mandates.

> Please note that the hardware cache is not the same as the neighbour
> cache.  Just because you lose the hardware cache doesn't mean that
> you'd lose the neighbour (ARP) cache.

Ok.

> However, this would mean that you'd no longer have an Ethernet device
> which is both good and bad.

Yeah. I don't think that'll go down well with userspace.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

> There are three ways out of this.  You could have your own cache
> (I haven't looked at your headers so I don't know whether that
> makes sense), have no cache at all, or add a new field that gets
> added to hard_header_len for the purposes of calculating head
> room.

I tried the last option, patch below, didn't quite work out yet, but I
got:

[  256.241496] KERNEL: assertion (!atomic_read(&sk->sk_wmem_alloc))
failed at net/packet/af_packet.c (225)

any ideas about that?

johannes

---
 include/linux/netdevice.h  |   10 ++++++++--
 net/core/netpoll.c         |    2 +-
 net/econet/af_econet.c     |    2 +-
 net/ipv4/arp.c             |    2 +-
 net/ipv4/igmp.c            |    4 ++--
 net/ipv4/ip_forward.c      |    4 +++-
 net/ipv4/ipconfig.c        |    6 +++---
 net/ipv4/ipip.c            |    1 +
 net/ipv4/ipmr.c            |    1 +
 net/ipv4/ipvs/ip_vs_xmit.c |    1 +
 net/ipv4/raw.c             |   10 ++++------
 net/ipv6/ip6_output.c      |    3 ++-
 net/ipv6/mcast.c           |    4 ++--
 net/ipv6/ndisc.c           |    4 ++--
 net/ipv6/raw.c             |   10 ++++------
 net/ipv6/sit.c             |    2 ++
 net/packet/af_packet.c     |    2 +-
 net/xfrm/xfrm_output.c     |   11 ++++++-----
 18 files changed, 45 insertions(+), 34 deletions(-)

--- everything.orig/include/linux/netdevice.h	2008-05-04 14:48:47.000000000 +0200
+++ everything/include/linux/netdevice.h	2008-05-04 15:34:04.000000000 +0200
@@ -254,11 +254,16 @@ struct hh_cache
  *
  * We could use other alignment values, but we must maintain the
  * relationship HH alignment <= LL alignment.
+ *
+ * LL_ALLOCATED_SPACE also takes into account the tailroom the device
+ * may need.
  */
 #define LL_RESERVED_SPACE(dev) \
-	(((dev)->hard_header_len&~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
+	((((dev)->hard_header_len+(dev)->needed_headroom)&~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
 #define LL_RESERVED_SPACE_EXTRA(dev,extra) \
-	((((dev)->hard_header_len+extra)&~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
+	((((dev)->hard_header_len+(dev)->needed_headroom+(extra))&~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
+#define LL_ALLOCATED_SPACE(dev) \
+	((((dev)->hard_header_len+(dev)->needed_headroom+(dev)->needed_tailroom)&~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
 
 struct header_ops {
 	int	(*create) (struct sk_buff *skb, struct net_device *dev,
@@ -576,6 +581,7 @@ struct net_device
 	unsigned		mtu;	/* interface MTU value		*/
 	unsigned short		type;	/* interface hardware type	*/
 	unsigned short		hard_header_len;	/* hardware hdr length	*/
+	unsigned short		needed_headroom, needed_tailroom;
 
 	struct net_device	*master; /* Pointer to master device of a group,
 					  * which this device is member of.
--- everything.orig/net/core/netpoll.c	2008-05-04 14:53:54.000000000 +0200
+++ everything/net/core/netpoll.c	2008-05-04 15:02:53.000000000 +0200
@@ -421,7 +421,7 @@ static void arp_reply(struct sk_buff *sk
 		return;
 
 	size = sizeof(struct arphdr) + 2 * (skb->dev->addr_len + 4);
-	send_skb = find_skb(np, size + LL_RESERVED_SPACE(np->dev),
+	send_skb = find_skb(np, size + LL_ALLOCATED_SPACE(np->dev),
 			    LL_RESERVED_SPACE(np->dev));
 
 	if (!send_skb)
--- everything.orig/net/econet/af_econet.c	2008-05-04 14:53:53.000000000 +0200
+++ everything/net/econet/af_econet.c	2008-05-04 14:56:53.000000000 +0200
@@ -340,7 +340,7 @@ static int econet_sendmsg(struct kiocb *
 
 		dev_hold(dev);
 
-		skb = sock_alloc_send_skb(sk, len+LL_RESERVED_SPACE(dev),
+		skb = sock_alloc_send_skb(sk, len+LL_ALLOCATED_SPACE(dev),
 					  msg->msg_flags & MSG_DONTWAIT, &err);
 		if (skb==NULL)
 			goto out_unlock;
--- everything.orig/net/ipv4/arp.c	2008-05-04 14:53:54.000000000 +0200
+++ everything/net/ipv4/arp.c	2008-05-04 15:04:18.000000000 +0200
@@ -571,7 +571,7 @@ struct sk_buff *arp_create(int type, int
 	 */
 
 	skb = alloc_skb(sizeof(struct arphdr)+ 2*(dev->addr_len+4)
-				+ LL_RESERVED_SPACE(dev), GFP_ATOMIC);
+				+ LL_ALLOCATED_SPACE(dev), GFP_ATOMIC);
 	if (skb == NULL)
 		return NULL;
 
--- everything.orig/net/ipv4/igmp.c	2008-05-04 14:53:55.000000000 +0200
+++ everything/net/ipv4/igmp.c	2008-05-04 15:06:59.000000000 +0200
@@ -292,7 +292,7 @@ static struct sk_buff *igmpv3_newpack(st
 	struct iphdr *pip;
 	struct igmpv3_report *pig;
 
-	skb = alloc_skb(size + LL_RESERVED_SPACE(dev), GFP_ATOMIC);
+	skb = alloc_skb(size + LL_ALLOCATED_SPACE(dev), GFP_ATOMIC);
 	if (skb == NULL)
 		return NULL;
 
@@ -653,7 +653,7 @@ static int igmp_send_report(struct in_de
 		return -1;
 	}
 
-	skb=alloc_skb(IGMP_SIZE+LL_RESERVED_SPACE(dev), GFP_ATOMIC);
+	skb=alloc_skb(IGMP_SIZE+LL_ALLOCATED_SPACE(dev), GFP_ATOMIC);
 	if (skb == NULL) {
 		ip_rt_put(rt);
 		return -1;
--- everything.orig/net/ipv4/ip_forward.c	2008-05-04 14:53:54.000000000 +0200
+++ everything/net/ipv4/ip_forward.c	2008-05-04 15:05:43.000000000 +0200
@@ -93,7 +93,9 @@ int ip_forward(struct sk_buff *skb)
 		goto drop;
 	}
 
-	/* We are about to mangle packet. Copy it! */
+	/* We are about to mangle packet. Copy it!
+	 * FIXME: account for needed tail length?
+	 */
 	if (skb_cow(skb, LL_RESERVED_SPACE(rt->u.dst.dev)+rt->u.dst.header_len))
 		goto drop;
 	iph = ip_hdr(skb);
--- everything.orig/net/ipv4/ipconfig.c	2008-05-04 14:53:54.000000000 +0200
+++ everything/net/ipv4/ipconfig.c	2008-05-04 15:06:16.000000000 +0200
@@ -713,14 +713,14 @@ static void __init ic_bootp_send_if(stru
 	struct net_device *dev = d->dev;
 	struct sk_buff *skb;
 	struct bootp_pkt *b;
-	int hh_len = LL_RESERVED_SPACE(dev);
 	struct iphdr *h;
 
 	/* Allocate packet */
-	skb = alloc_skb(sizeof(struct bootp_pkt) + hh_len + 15, GFP_KERNEL);
+	skb = alloc_skb(sizeof(struct bootp_pkt) +
+			LL_ALLOCATED_SPACE(dev) + 15, GFP_KERNEL);
 	if (!skb)
 		return;
-	skb_reserve(skb, hh_len);
+	skb_reserve(skb, LL_RESERVED_SPACE(dev));
 	b = (struct bootp_pkt *) skb_put(skb, sizeof(struct bootp_pkt));
 	memset(b, 0, sizeof(struct bootp_pkt));
 
--- everything.orig/net/ipv4/ipip.c	2008-05-04 14:53:55.000000000 +0200
+++ everything/net/ipv4/ipip.c	2008-05-04 15:07:10.000000000 +0200
@@ -587,6 +587,7 @@ static int ipip_tunnel_xmit(struct sk_bu
 
 	/*
 	 * Okay, now see if we can stuff it in the buffer as-is.
+	 * FIXME: tail room?
 	 */
 	max_headroom = (LL_RESERVED_SPACE(tdev)+sizeof(struct iphdr));
 
--- everything.orig/net/ipv4/ipmr.c	2008-05-04 14:53:54.000000000 +0200
+++ everything/net/ipv4/ipmr.c	2008-05-04 15:06:32.000000000 +0200
@@ -1211,6 +1211,7 @@ static void ipmr_queue_xmit(struct sk_bu
 		goto out_free;
 	}
 
+	/* FIXME: what about needed tailroom? */
 	encap += LL_RESERVED_SPACE(dev) + rt->u.dst.header_len;
 
 	if (skb_cow(skb, encap)) {
--- everything.orig/net/ipv4/ipvs/ip_vs_xmit.c	2008-05-04 14:53:54.000000000 +0200
+++ everything/net/ipv4/ipvs/ip_vs_xmit.c	2008-05-04 15:04:34.000000000 +0200
@@ -363,6 +363,7 @@ ip_vs_tunnel_xmit(struct sk_buff *skb, s
 
 	/*
 	 * Okay, now see if we can stuff it in the buffer as-is.
+	 * FIXME: account for needed tailroom?
 	 */
 	max_headroom = LL_RESERVED_SPACE(tdev) + sizeof(struct iphdr);
 
--- everything.orig/net/ipv4/raw.c	2008-05-04 14:53:54.000000000 +0200
+++ everything/net/ipv4/raw.c	2008-05-04 15:30:46.000000000 +0200
@@ -329,7 +329,6 @@ static int raw_send_hdrinc(struct sock *
 			unsigned int flags)
 {
 	struct inet_sock *inet = inet_sk(sk);
-	int hh_len;
 	struct iphdr *iph;
 	struct sk_buff *skb;
 	unsigned int iphlen;
@@ -343,13 +342,12 @@ static int raw_send_hdrinc(struct sock *
 	if (flags&MSG_PROBE)
 		goto out;
 
-	hh_len = LL_RESERVED_SPACE(rt->u.dst.dev);
-
-	skb = sock_alloc_send_skb(sk, length+hh_len+15,
-				  flags&MSG_DONTWAIT, &err);
+	skb = sock_alloc_send_skb(
+		sk, length + LL_ALLOCATED_SPACE(rt->u.dst.dev) + 15,
+		flags & MSG_DONTWAIT, &err);
 	if (skb == NULL)
 		goto error;
-	skb_reserve(skb, hh_len);
+	skb_reserve(skb, LL_RESERVED_SPACE(rt->u.dst.dev));
 
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;
--- everything.orig/net/ipv6/ip6_output.c	2008-05-04 14:53:53.000000000 +0200
+++ everything/net/ipv6/ip6_output.c	2008-05-04 15:02:05.000000000 +0200
@@ -201,6 +201,7 @@ int ip6_xmit(struct sock *sk, struct sk_
 
 		/* First: exthdrs may take lots of space (~8K for now)
 		   MAX_HEADER is not enough.
+		   FIXME: LL_ALLOCATED_SPACE?
 		 */
 		head_room = opt->opt_nflen + opt->opt_flen;
 		seg_len += head_room;
@@ -780,7 +781,7 @@ slow_path:
 		 *	Allocate buffer.
 		 */
 
-		if ((frag = alloc_skb(len+hlen+sizeof(struct frag_hdr)+LL_RESERVED_SPACE(rt->u.dst.dev), GFP_ATOMIC)) == NULL) {
+		if ((frag = alloc_skb(len+hlen+sizeof(struct frag_hdr)+LL_ALLOCATED_SPACE(rt->u.dst.dev), GFP_ATOMIC)) == NULL) {
 			NETDEBUG(KERN_INFO "IPv6: frag: no memory for new fragment!\n");
 			IP6_INC_STATS(ip6_dst_idev(skb->dst),
 				      IPSTATS_MIB_FRAGFAILS);
--- everything.orig/net/ipv6/mcast.c	2008-05-04 14:53:53.000000000 +0200
+++ everything/net/ipv6/mcast.c	2008-05-04 15:01:21.000000000 +0200
@@ -1403,7 +1403,7 @@ static struct sk_buff *mld_newpack(struc
 		     IPV6_TLV_PADN, 0 };
 
 	/* we assume size > sizeof(ra) here */
-	skb = sock_alloc_send_skb(sk, size + LL_RESERVED_SPACE(dev), 1, &err);
+	skb = sock_alloc_send_skb(sk, size + LL_ALLOCATED_SPACE(dev), 1, &err);
 
 	if (!skb)
 		return NULL;
@@ -1776,7 +1776,7 @@ static void igmp6_send(struct in6_addr *
 	payload_len = len + sizeof(ra);
 	full_len = sizeof(struct ipv6hdr) + payload_len;
 
-	skb = sock_alloc_send_skb(sk, LL_RESERVED_SPACE(dev) + full_len, 1, &err);
+	skb = sock_alloc_send_skb(sk, LL_ALLOCATED_SPACE(dev) + full_len, 1, &err);
 
 	if (skb == NULL) {
 		rcu_read_lock();
--- everything.orig/net/ipv6/ndisc.c	2008-05-04 14:53:53.000000000 +0200
+++ everything/net/ipv6/ndisc.c	2008-05-04 15:01:37.000000000 +0200
@@ -494,7 +494,7 @@ static void __ndisc_send(struct net_devi
 
 	skb = sock_alloc_send_skb(sk,
 				  (MAX_HEADER + sizeof(struct ipv6hdr) +
-				   len + LL_RESERVED_SPACE(dev)),
+				   len + LL_ALLOCATED_SPACE(dev)),
 				  1, &err);
 	if (!skb) {
 		ND_PRINTK0(KERN_ERR
@@ -1494,7 +1494,7 @@ void ndisc_send_redirect(struct sk_buff 
 
 	buff = sock_alloc_send_skb(sk,
 				   (MAX_HEADER + sizeof(struct ipv6hdr) +
-				    len + LL_RESERVED_SPACE(dev)),
+				    len + LL_ALLOCATED_SPACE(dev)),
 				   1, &err);
 	if (buff == NULL) {
 		ND_PRINTK0(KERN_ERR
--- everything.orig/net/ipv6/raw.c	2008-05-04 14:53:53.000000000 +0200
+++ everything/net/ipv6/raw.c	2008-05-04 14:57:58.000000000 +0200
@@ -624,7 +624,6 @@ static int rawv6_send_hdrinc(struct sock
 	struct ipv6_pinfo *np = inet6_sk(sk);
 	struct ipv6hdr *iph;
 	struct sk_buff *skb;
-	unsigned int hh_len;
 	int err;
 
 	if (length > rt->u.dst.dev->mtu) {
@@ -634,13 +633,12 @@ static int rawv6_send_hdrinc(struct sock
 	if (flags&MSG_PROBE)
 		goto out;
 
-	hh_len = LL_RESERVED_SPACE(rt->u.dst.dev);
-
-	skb = sock_alloc_send_skb(sk, length+hh_len+15,
-				  flags&MSG_DONTWAIT, &err);
+	skb = sock_alloc_send_skb(
+		sk, length + LL_ALLOCATED_SPACE(rt->u.dst.dev) + 15,
+		flags & MSG_DONTWAIT, &err);
 	if (skb == NULL)
 		goto error;
-	skb_reserve(skb, hh_len);
+	skb_reserve(skb, LL_RESERVED_SPACE(rt->u.dst.dev));
 
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;
--- everything.orig/net/ipv6/sit.c	2008-05-04 14:53:53.000000000 +0200
+++ everything/net/ipv6/sit.c	2008-05-04 15:00:55.000000000 +0200
@@ -603,6 +603,8 @@ static int ipip6_tunnel_xmit(struct sk_b
 
 	/*
 	 * Okay, now see if we can stuff it in the buffer as-is.
+	 *
+	 * XXX: tailroom? LL_ALLOCATED_SPACE?
 	 */
 	max_headroom = LL_RESERVED_SPACE(tdev)+sizeof(struct iphdr);
 
--- everything.orig/net/packet/af_packet.c	2008-05-04 14:53:53.000000000 +0200
+++ everything/net/packet/af_packet.c	2008-05-04 15:02:33.000000000 +0200
@@ -743,7 +743,7 @@ static int packet_sendmsg(struct kiocb *
 	if (len > dev->mtu+reserve)
 		goto out_unlock;
 
-	skb = sock_alloc_send_skb(sk, len + LL_RESERVED_SPACE(dev),
+	skb = sock_alloc_send_skb(sk, len + LL_ALLOCATED_SPACE(dev),
 				msg->msg_flags & MSG_DONTWAIT, &err);
 	if (skb==NULL)
 		goto out_unlock;
--- everything.orig/net/xfrm/xfrm_output.c	2008-05-04 14:53:52.000000000 +0200
+++ everything/net/xfrm/xfrm_output.c	2008-05-04 15:46:11.000000000 +0200
@@ -23,13 +23,14 @@ static int xfrm_output2(struct sk_buff *
 static int xfrm_state_check_space(struct xfrm_state *x, struct sk_buff *skb)
 {
 	struct dst_entry *dst = skb->dst;
-	int nhead = dst->header_len + LL_RESERVED_SPACE(dst->dev)
-		- skb_headroom(skb);
+	int nhead = max_t(int, 0, dst->header_len + LL_RESERVED_SPACE(dst->dev)
+				  - skb_headroom(skb));
+	int ntail = max_t(int, 0, dst->dev->needed_tailroom
+				  - skb_tailroom(skb));
 
-	if (nhead > 0)
-		return pskb_expand_head(skb, nhead, 0, GFP_ATOMIC);
+	if (nhead || ntail)
+		return pskb_expand_head(skb, nhead, ntail, GFP_ATOMIC);
 
-	/* Check tail too... */
 	return 0;
 }
 


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 16:06 +0200, Johannes Berg wrote:
> > There are three ways out of this.  You could have your own cache
> > (I haven't looked at your headers so I don't know whether that
> > makes sense), have no cache at all, or add a new field that gets
> > added to hard_header_len for the purposes of calculating head
> > room.
> 
> I tried the last option, patch below, didn't quite work out yet, but I
> got:
> 
> [  256.241496] KERNEL: assertion (!atomic_read(&sk->sk_wmem_alloc))
> failed at net/packet/af_packet.c (225)

Except for that, the patch works fine with mac80211 setting the new
netdev fields. I still get lots of reallocations though because I get a
lot of cloned packets, not sure why.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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


> > [  256.241496] KERNEL: assertion (!atomic_read(&sk->sk_wmem_alloc))
> > failed at net/packet/af_packet.c (225)
> 
> Except for that, 

Oddly, I haven't been able to reproduce this, so I'll clean up the
patches now.


If I have a device that needs 100 bytes headroom (not unheard of),
there will of course still be packets that don't have enough headroom.
Should we skb_orphan them?

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Sun, 04 May 2008 16:06:30 +0200

> [  256.241496] KERNEL: assertion (!atomic_read(&sk->sk_wmem_alloc))
> failed at net/packet/af_packet.c (225)
> 
> any ideas about that?

This is exactly what happens if you change skb->truesize
when a socket is still attached to the skb.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sun, 2008-05-04 at 15:45 -0700, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Sun, 04 May 2008 16:06:30 +0200
> 
> > [  256.241496] KERNEL: assertion (!atomic_read(&sk->sk_wmem_alloc))
> > failed at net/packet/af_packet.c (225)
> > 
> > any ideas about that?
> 
> This is exactly what happens if you change skb->truesize
> when a socket is still attached to the skb.

Ok, that makes sense. Then it also makes sense I never got that warning
again, I had left one of my test patches in too long.

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 4 May 2008 11:16:52 +0800

> On Sun, May 04, 2008 at 04:22:20AM +0200, Johannes Berg wrote:
> >
> > Yes, wireless always needs at least 24 bytes, but more likely 34
> > (encryption+QoS). However, I just increased LL_MAX_HEADER to 54 and that
> > doesn't seem to have helped.
> 
> How did you test it?

I know what causes this problem, things like ARP.

They don't use LL_MAX_HEADER, and instead go:

	skb = alloc_skb(arp_hdr_len(dev) + LL_RESERVED_SPACE(dev), GFP_ATOMIC);

because they are reasonably sure what exact device they
are sending out of :-)

As mentioned elsewhere, there is a disconnect between how some
of these values are used.  But I'm pretty sure I know why some
of them are used this way.

Re: mac80211 truesize bugs

[Johannes Berg]

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


> Ideally, we'd have enough headroom in the skb to start with, since right
> now we're apparently reallocating a lot, especially encrypted frames.
> Not that I understand why we don't get a truesize bug (without the
> monitors) when we do that.

Hmm. I wonder if the reason might be that the skb itself is long enough,
but has so much tailroom that it doesn't have enough headroom, but when
we reallocate it to have more headroom we actually later don't use the
extra tailroom. That would explain why I don't get truesize bugs
although I do get a warning (from the modified pskb_expand_head()) that
I mustn't reallocate the buffer.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Thu, 2008-05-01 at 19:03 +0800, Herbert Xu wrote:
> On Thu, May 01, 2008 at 12:58:20PM +0200, Johannes Berg wrote:
> >
> > Indeed. But then why did we never see this bug w/o monitor interfaces
> > and this reinjection?
> 
> The debugging only catches it if the expanded area actually
> gets used, e.g., by skb_push.

Why, btw? It's not too hard to check the allocated size, no?

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Sat, May 03, 2008 at 02:38:01PM +0200, Johannes Berg wrote:
>
> Why, btw? It's not too hard to check the allocated size, no?

Yes that would be a meaningful improvement although we'd need to
audit/test this to make sure that we don't spam people's logs
with it.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sat, 2008-05-03 at 20:59 +0800, Herbert Xu wrote:
> On Sat, May 03, 2008 at 02:38:01PM +0200, Johannes Berg wrote:
> >
> > Why, btw? It's not too hard to check the allocated size, no?
> 
> Yes that would be a meaningful improvement although we'd need to
> audit/test this to make sure that we don't spam people's logs
> with it.

It does spam the log. A lot. And I don't know why, from this discussion
I only thought that it shouldn't.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

On Sat, 2008-05-03 at 18:03 +0200, Johannes Berg wrote:
> On Sat, 2008-05-03 at 20:59 +0800, Herbert Xu wrote:
> > On Sat, May 03, 2008 at 02:38:01PM +0200, Johannes Berg wrote:
> > >
> > > Why, btw? It's not too hard to check the allocated size, no?
> > 
> > Yes that would be a meaningful improvement although we'd need to
> > audit/test this to make sure that we don't spam people's logs
> > with it.
> 
> It does spam the log. A lot. And I don't know why, from this discussion
> I only thought that it shouldn't.

This was a stupid mistake, if you do it correctly it actually works and
so far has only triggered a single warning on my system:

[  217.507048] SKB BUG: Invalid truesize (4294964120) size=432, sizeof(sk_buff)=176

that was with my patch though to update skb->truesize during !skb->sk
pskb_expand_head() calls.

johannes

---
 include/linux/skbuff.h |    8 ++++++--
 net/core/skbuff.c      |   10 ++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

--- everything.orig/include/linux/skbuff.h	2008-05-03 15:47:00.000000000 +0200
+++ everything/include/linux/skbuff.h	2008-05-04 00:30:34.000000000 +0200
@@ -387,9 +387,13 @@ extern void	      skb_truesize_bug(struc
 
 static inline void skb_truesize_check(struct sk_buff *skb)
 {
-	int len = sizeof(struct sk_buff) + skb->len;
+#ifdef NET_SKBUFF_DATA_USES_OFFSET
+	int len = sizeof(struct sk_buff) + skb->end;
+#else
+	int len = sizeof(struct sk_buff) + (skb->end - skb->head);
+#endif
 
-	if (unlikely((int)skb->truesize < len))
+	if (unlikely((int)skb->truesize != len))
 		skb_truesize_bug(skb);
 }
 
--- everything.orig/net/core/skbuff.c	2008-05-03 16:29:23.000000000 +0200
+++ everything/net/core/skbuff.c	2008-05-04 00:31:32.000000000 +0200
@@ -151,9 +151,15 @@ void skb_under_panic(struct sk_buff *skb
 
 void skb_truesize_bug(struct sk_buff *skb)
 {
+#ifdef NET_SKBUFF_DATA_USES_OFFSET
+	int len = sizeof(struct sk_buff) + skb->end;
+#else
+	int len = sizeof(struct sk_buff) + (skb->end - skb->head);
+#endif
+
 	printk(KERN_ERR "SKB BUG: Invalid truesize (%u) "
-	       "len=%u, sizeof(sk_buff)=%Zd\n",
-	       skb->truesize, skb->len, sizeof(struct sk_buff));
+	       "size=%u, sizeof(sk_buff)=%Zd\n",
+	       skb->truesize, len, sizeof(struct sk_buff));
 }
 EXPORT_SYMBOL(skb_truesize_bug);
 


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Sun, 04 May 2008 00:56:23 +0200

> This was a stupid mistake, if you do it correctly it actually works and
> so far has only triggered a single warning on my system:
> 
> [  217.507048] SKB BUG: Invalid truesize (4294964120) size=432, sizeof(sk_buff)=176
> 
> that was with my patch though to update skb->truesize during !skb->sk
> pskb_expand_head() calls.

Thanks a lot for tracking this down Johannes.

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Sat, 2008-05-03 at 16:07 -0700, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Sun, 04 May 2008 00:56:23 +0200
> 
> > This was a stupid mistake, if you do it correctly it actually works and
> > so far has only triggered a single warning on my system:
> > 
> > [  217.507048] SKB BUG: Invalid truesize (4294964120) size=432, sizeof(sk_buff)=176
> > 
> > that was with my patch though to update skb->truesize during !skb->sk
> > pskb_expand_head() calls.
> 
> Thanks a lot for tracking this down Johannes.

No, no, I haven't. The only thing that was a stupid mistake was my
change to check the actual allocated skb size in the truesize bug rather
than do the < len check... That works, when you do it as in that patch,
but the mac80211 stuff still stands (along with all the questions I
posted elsewhere in this thread)

johannes

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

Re: mac80211 truesize bugs

[Herbert Xu]

On Thu, May 01, 2008 at 03:49:50AM -0700, David Miller wrote:
>
> Once that is cured, I think we can detect this better, by adding a
> carefully constructed assertion to pskb_expand_head().  Basically, the
> idea is, if "nhead" or "ntail" are non-zero, and there is a socket
> still attached to the SKB, print a warning message.
> 
> Something like:
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4fe605f..9bfca08 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -699,6 +699,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  	if (skb_shared(skb))
>  		BUG();
>  
> +	if (unlikely((nhead || ntail) && skb->sk)) {
> +		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
> +		       "with socket attached\n",
> +		       nhead, ntail);
> +	}

This could actually work :) I was worried about tunnelling doing
a genuine expansion on such a packet but it turns that it does a
skb_clone first.  So perhaps we should just require that if a
packet is to be expanded while attached to a socket then it must
be cloned first.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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


> That makes more sense, good catch Herbert.
> 
> I guess it's the pskb_expand_head() calls done by net/mac80211/tx.c
> I suspect we'll need to orphan early in order to accomodate these
> adjustments, otherwise socket memory buffer allocations will
> be corrupted.
> 
> Once that is cured, I think we can detect this better, by adding a
> carefully constructed assertion to pskb_expand_head().  Basically, the
> idea is, if "nhead" or "ntail" are non-zero, and there is a socket
> still attached to the SKB, print a warning message.
> 
> Something like:
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4fe605f..9bfca08 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -699,6 +699,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  	if (skb_shared(skb))
>  		BUG();
>  
> +	if (unlikely((nhead || ntail) && skb->sk)) {
> +		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
> +		       "with socket attached\n",
> +		       nhead, ntail);
> +	}
> +
>  	size = SKB_DATA_ALIGN(size);

Ok I think I'm starting to understand this a little better. However,
shouldn't this function update skb->truesize so if the skb is later
attached to a different socket again it has the right size?

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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


> > @@ -699,6 +699,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
> >  	if (skb_shared(skb))
> >  		BUG();
> >  
> > +	if (unlikely((nhead || ntail) && skb->sk)) {
> > +		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
> > +		       "with socket attached\n",
> > +		       nhead, ntail);
> > +	}
> > +
> >  	size = SKB_DATA_ALIGN(size);
> 
> Ok I think I'm starting to understand this a little better. However,
> shouldn't this function update skb->truesize so if the skb is later
> attached to a different socket again it has the right size?

Judging from some of the callers, the caller should. Why?!

johannes

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

Re: mac80211 truesize bugs

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Sun, 04 May 2008 01:24:48 +0200

> 
> > > @@ -699,6 +699,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
> > >  	if (skb_shared(skb))
> > >  		BUG();
> > >  
> > > +	if (unlikely((nhead || ntail) && skb->sk)) {
> > > +		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
> > > +		       "with socket attached\n",
> > > +		       nhead, ntail);
> > > +	}
> > > +
> > >  	size = SKB_DATA_ALIGN(size);
> > 
> > Ok I think I'm starting to understand this a little better. However,
> > shouldn't this function update skb->truesize so if the skb is later
> > attached to a different socket again it has the right size?
> 
> Judging from some of the callers, the caller should. Why?!

Relax :-)

We certainly could check that there is no socket attached here,
and make the truesize adjustment right at this spot.

It just never happened before in practice in a way that matters.

That's why we have the truesize assertion, to discover situations
like this and thus be able to fix it.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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


> > > > +	if (unlikely((nhead || ntail) && skb->sk)) {
> > > > +		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
> > > > +		       "with socket attached\n",
> > > > +		       nhead, ntail);
> > > > +	}
> > > > +
> > > >  	size = SKB_DATA_ALIGN(size);
> > > 
> > > Ok I think I'm starting to understand this a little better. However,
> > > shouldn't this function update skb->truesize so if the skb is later
> > > attached to a different socket again it has the right size?
> > 
> > Judging from some of the callers, the caller should. Why?!
> 
> Relax :-)

:)

Hm. The only caller that does seem to do it seems to be in af_netlink.c.

> We certainly could check that there is no socket attached here,
> and make the truesize adjustment right at this spot.
> 
> It just never happened before in practice in a way that matters.
> 
> That's why we have the truesize assertion, to discover situations
> like this and thus be able to fix it.

Except unfortunately the truesize assertion is rather useless since you
have no idea where it comes from. FWIW, some caller that does the
adjustment must be going wrong, whenever I start vpnc I get a single one
like this:

[  162.108556] SKB BUG: Invalid truesize (408) size=432,
sizeof(sk_buff)=176

This is again without the patch to pskb_expand_head that did the
truesize adjustment, I only put in a WARN_ON (similar to the code you
had above but is, I think, more useful since it has a stack dump and
other useful info)

Right now I think I'm too lazy to dig into where this happens. I don't
hit the warning in pskb_expand_head so it must be one of the other
20-odd places where truesize is adjusted. Maybe I'll just make each of
them print out the info.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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

>  
> +	if (unlikely((nhead || ntail) && skb->sk)) {
> +		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
> +		       "with socket attached\n",
> +		       nhead, ntail);
> +	}
> +

Also this could be a WARN_ON since here we really want to know who
called it.

johannes

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

Re: mac80211 truesize bugs

[Johannes Berg]

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


> That makes more sense, good catch Herbert.
> 
> I guess it's the pskb_expand_head() calls done by net/mac80211/tx.c
> I suspect we'll need to orphan early in order to accomodate these
> adjustments, otherwise socket memory buffer allocations will
> be corrupted.
> 
> Once that is cured, I think we can detect this better, by adding a
> carefully constructed assertion to pskb_expand_head().  Basically, the
> idea is, if "nhead" or "ntail" are non-zero, and there is a socket
> still attached to the SKB, print a warning message.

I'm confused now. I added this patch:

--- everything.orig/net/core/skbuff.c	2008-05-01 13:21:52.000000000 +0200
+++ everything/net/core/skbuff.c	2008-05-01 13:29:57.000000000 +0200
@@ -683,6 +683,14 @@ int pskb_expand_head(struct sk_buff *skb
 	if (!data)
 		goto nodata;
 
+	if (unlikely((nhead || ntail) && skb->sk)) {
+		printk(KERN_ERR "SKB BUG: Illegal pskb expand (%d:%d) "
+				"with socket attached\n",
+		       nhead, ntail);
+                dump_stack();
+	} else
+		skb->truesize = size + sizeof(struct sk_buff);
+
 	/* Copy only real data... and, alas, header. This should be
 	 * optimized for the cases when header is void. */
 #ifdef NET_SKBUFF_DATA_USES_OFFSET
--- everything.orig/net/mac80211/tx.c	2008-05-01 13:01:09.000000000 +0200
+++ everything/net/mac80211/tx.c	2008-05-01 13:16:50.000000000 +0200
@@ -1279,6 +1279,8 @@ int ieee80211_master_start_xmit(struct s
 	int headroom;
 	int ret;
 
+	skb_orphan(skb);
+
 	if (info->flags & IEEE80211_TX_CTL_READY_FOR_TX) {
 		/*
 		 * We set the IEEE80211_TX_CTL_READY_FOR_TX bit in all skbs
@@ -1581,6 +1583,7 @@ int ieee80211_subif_start_xmit(struct sk
 	 * us broadcast frames. */
 
 	if (head_need > 0 || skb_cloned(skb)) {
+		skb_orphan(skb);
 #if 0
 		printk(KERN_DEBUG "%s: need to reallocate buffer for %d bytes "
 		       "of headroom\n", dev->name, head_need);


and the assertion never triggers, however I get a number of bugs like this:

SKB BUG: Invalid truesize (4294963740) len=44, sizeof(sk_buff)=176, skb=0xeecb6620

and definitely cannot explain that number (-3556).

johannes

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

Re: mac80211 truesize bugs

[Michael Buesch]

On Thursday 01 May 2008 11:08:06 Johannes Berg wrote:
> 
> > Hm, unrelated to this...
> > But I am wondering what happens if the driver adds a device header to the skb.
> > Is that header then also passed up netif_rx()?
> > This doesn't happen for b43, as we use the DMA fragmentation to transmit the header,
> > but it might happen for zd1211rw and others.
> 
> Hmm. I thought we said that it was supposed to be removed again by the
> hardware before TX status reporting. That's what most drivers seem to do
> anyway.

Ok. I was not aware of that. Is that documented somewhere? I guess we can't WARN_ON()?

> > Seems the skb->destructor messes it up.
> 
> Actually, it seems to be outside of mac80211, I put in a WARN_ON() and
> got this:

Yeah looks like the destructor messes with the data/sizes and disagrees
with the way mac80211 handles stuff, in some way.

> Badness at include/linux/skbuff.h:392
> NIP: c026ea14 LR: c0273d54 CTR: c026e9e4
> REGS: edfc7c00 TRAP: 0700   Not tainted  (2.6.25-wl-06841-g6b3d5c6-dirty)
> MSR: 00029032 <EE,ME,IR,DR>  CR: 82022444  XER: 00000000
> TASK = edf50e20[3453] 'tcpdump' THREAD: edfc6000
> GPR00: 00000001 edfc7cb0 edf50e20 edfd7700 edfd7700 00000002 edfc7e75 03230306 
> GPR08: 02000100 00000168 4dff0200 00000150 22022442 100a6290 100783f8 10078e18 
> GPR16: 10078e14 10078e10 100a0000 00000000 00000000 bfe2c9d2 1004d320 bfe2c4b0 
> GPR24: 10165070 edfd7724 00000060 00000020 ed8157f0 edfd7700 ed8157f0 edfd7700 
> NIP [c026ea14] sock_rfree+0x30/0x94
> LR [c0273d54] skb_release_all+0x98/0x128
> Call Trace:
> [edfc7cb0] [10078e10] 0x10078e10 (unreliable)
> [edfc7cc0] [c0273d54] skb_release_all+0x98/0x128
> [edfc7cd0] [c0273034] __kfree_skb+0x18/0xc8
> [edfc7ce0] [c02760d0] skb_free_datagram+0x1c/0x54
> [edfc7cf0] [f264d068] packet_recvmsg+0x170/0x1e8 [af_packet]
> [edfc7d40] [c026b69c] sock_recvmsg+0xb8/0xf0
> [edfc7e30] [c026b9d0] sys_recvfrom+0x94/0x100
> [edfc7f00] [c026ca08] sys_socketcall+0x114/0x1dc
> [edfc7f40] [c00124cc] ret_from_syscall+0x0/0x38
> 
> johannes
> 



-- 
Greetings Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac80211 truesize bugs

[Johannes Berg]

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

On Thu, 2008-05-01 at 11:32 +0200, Michael Buesch wrote:
> On Thursday 01 May 2008 11:08:06 Johannes Berg wrote:
> > 
> > > Hm, unrelated to this...
> > > But I am wondering what happens if the driver adds a device header to the skb.
> > > Is that header then also passed up netif_rx()?
> > > This doesn't happen for b43, as we use the DMA fragmentation to transmit the header,
> > > but it might happen for zd1211rw and others.
> > 
> > Hmm. I thought we said that it was supposed to be removed again by the
> > hardware before TX status reporting. That's what most drivers seem to do
> > anyway.
> 
> Ok. I was not aware of that. Is that documented somewhere? I guess we can't WARN_ON()?

Not sure. We could try to analyse the packet header and see if it's
802.11, or put a length into skb->cb (after my rework to put things into
skb->cb we have a single byte free so we could check skb->len % 256)

johannes

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

frame status API? (was: mac80211 truesize bugs)

[Johannes Berg]

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


> Whenever you run a monitor interface in mac80211, you can see lots of
> truesize bugs:
> 
> SKB BUG: Invalid truesize (464) len=307, sizeof(sk_buff)=176
> 
> It appears to be caused by mac80211's re-injection of the transmitted
> frame.

Regardless of the fact that I'm pretty sure that we have bugs with
respect to pskb_expand_head in mac80211 that for some reason never show
up unless you add monitors, this re-injection of frames is quite a hack.

For one, it means that on a monitor interface you see every outgoing
frame, even if it wasn't sent on that interface. That's actually quite
nice, especially for debugging, but I think we can already see all
transmitted packets on the master interface.

Secondly, it means that actually injected packets show up twice, first
via dev_queue_xmit_nit and then via the status reporting.

Also, if we ever want to move towards an API where the driver need not
give a TX status report for every frame (some hardware cannot do this)
then this is at best a hack since we do not know the exact status for a
frame.

On the other hand, hostapd requires knowing whether a specific frame was
acknowledged.

Does anyone have any ideas how to implement such a status query for a
frame that was sent? What hostapd does is open a raw socket and simply
send a frame, but it needs to know whether that frame was acknowledged
so currently it also listens on a monitor and checks all outgoing
frames, which is suboptimal anyway.

The only thing I could so far think of is like the socket timestamps but
that seems rather bad to do for such a special case.

johannes

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