From: Eric Leblond <hidden> Date: 2012-12-07 18:56:47
This patch is adding a check on skb before trying to defrag the
packet for the hash computation in fanout mode. The goal of this
patch is to avoid an kernel crash in pskb_expand_head.
It appears that under some specific condition there is a shared
skb reaching the defrag code and this lead to a crash due to the
following code:
if (skb_shared(skb))
BUG();
I've observed this crash under the following condition:
1. a program is listening to an wifi interface (let say wlan0)
2. it is using fanout capture in flow load balancing mode
3. defrag option is on on the fanout socket
4. the interface disconnect (radio down for example)
5. the interface reconnect (radio switched up)
6. once reconnected a single packet is seen with skb->users=2
7. the kernel crash in pskb_expand_head at skbuff.c:1035
[BBB55:744364] [<ffffffff812a2761>] ? __pskb_pull_tail+0x43x0x26f
[BB8S5.744395] [<ffffffff812d29Tb>] ? ip_check_defrag+ox3a/0x14a
[BBB55.744422] [<ffffffffB1344459>] ? packet_rcv_fanout+ox5e/oxf9
[BBBS5.7444S0] [<ffffffffB12aaS9b>] ? __netif_receive_skb+ox444/ox4f9
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? netif_receive_skb+ox6d/0x?3
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_deliver_skb+0xbd/0xfa [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_h_data+0x1e0/0x21a [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_handlers+0x3d5/0x480 [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? __wake_up
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? evdev_eventr+0xc0/0xcf [evdev]
Signed-off-by: Eric Leblond <redacted>
---
net/packet/af_packet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: David Miller <davem@davemloft.net> Date: 2012-12-07 19:10:56
From: Eric Leblond <redacted>
Date: Fri, 7 Dec 2012 19:56:01 +0100
This patch is adding a check on skb before trying to defrag the
packet for the hash computation in fanout mode. The goal of this
patch is to avoid an kernel crash in pskb_expand_head.
It appears that under some specific condition there is a shared
skb reaching the defrag code and this lead to a crash due to the
following code:
if (skb_shared(skb))
BUG();
I've observed this crash under the following condition:
1. a program is listening to an wifi interface (let say wlan0)
2. it is using fanout capture in flow load balancing mode
3. defrag option is on on the fanout socket
4. the interface disconnect (radio down for example)
5. the interface reconnect (radio switched up)
6. once reconnected a single packet is seen with skb->users=2
7. the kernel crash in pskb_expand_head at skbuff.c:1035
...
Signed-off-by: Eric Leblond <redacted>
Thanks Eric. I'll try to figure out if we should instead
change the wireless code to avoid sending shared SKBs into
the input path like that.
From: David Miller <davem@davemloft.net> Date: 2012-12-07 20:31:37
From: Eric Leblond <redacted>
Date: Fri, 7 Dec 2012 19:56:01 +0100
Wireless folks, please take a look. The issue is that,
under the circumstances listed below, we get SKBs in
the AF_PACKET input path that are shared.
Given the logic present in ieee80211_deliver_skb() I think
the mac80211 code doesn't expect this either.
More commentary from me below:
This patch is adding a check on skb before trying to defrag the
packet for the hash computation in fanout mode. The goal of this
patch is to avoid an kernel crash in pskb_expand_head.
It appears that under some specific condition there is a shared
skb reaching the defrag code and this lead to a crash due to the
following code:
if (skb_shared(skb))
BUG();
I've observed this crash under the following condition:
1. a program is listening to an wifi interface (let say wlan0)
2. it is using fanout capture in flow load balancing mode
3. defrag option is on on the fanout socket
4. the interface disconnect (radio down for example)
5. the interface reconnect (radio switched up)
6. once reconnected a single packet is seen with skb->users=2
7. the kernel crash in pskb_expand_head at skbuff.c:1035
[BBB55:744364] [<ffffffff812a2761>] ? __pskb_pull_tail+0x43x0x26f
[BB8S5.744395] [<ffffffff812d29Tb>] ? ip_check_defrag+ox3a/0x14a
[BBB55.744422] [<ffffffffB1344459>] ? packet_rcv_fanout+ox5e/oxf9
[BBBS5.7444S0] [<ffffffffB12aaS9b>] ? __netif_receive_skb+ox444/ox4f9
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? netif_receive_skb+ox6d/0x?3
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_deliver_skb+0xbd/0xfa [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_h_data+0x1e0/0x21a [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_handlers+0x3d5/0x480 [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? __wake_up
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? evdev_eventr+0xc0/0xcf [evdev]
Signed-off-by: Eric Leblond <redacted>
So if we look at ieee80211_deliver_skb(), it has code to deal with unaligned
packet headers, wherein it memoves() the data into a better aligned location.
But if these SKBs really are skb_shared(), this packet data
modification is illegal.
I suspect that the assumptions built into this unaligned data handling
code, and AF_PACKET, are correct. Meaning that we should never see
skb_shared() packets here. We just have a missing skb_copy()
somewhere in mac80211, Johannes can you please take a look?
Thanks.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2012-12-07 20:41:46
On Fri, 2012-12-07 at 15:31 -0500, David Miller wrote:
From: Eric Leblond <redacted>
Date: Fri, 7 Dec 2012 19:56:01 +0100
Wireless folks, please take a look. The issue is that,
under the circumstances listed below, we get SKBs in
the AF_PACKET input path that are shared.
Given the logic present in ieee80211_deliver_skb() I think
the mac80211 code doesn't expect this either.
Indeed, it would certainly not like this, I'll take a look.
Eric, what's the driver you're using? I'm wondering whether paged skbs
vs. all data in the header would make a difference, hence the question.
johannes
From: Eric Leblond <hidden> Date: 2012-12-07 20:55:11
Hi,
On Fri, 2012-12-07 at 15:31 -0500, David Miller wrote:
From: Eric Leblond <redacted>
Date: Fri, 7 Dec 2012 19:56:01 +0100
Wireless folks, please take a look. The issue is that,
under the circumstances listed below, we get SKBs in
the AF_PACKET input path that are shared.
Given the logic present in ieee80211_deliver_skb() I think
the mac80211 code doesn't expect this either.
More commentary from me below:
quoted
This patch is adding a check on skb before trying to defrag the
packet for the hash computation in fanout mode. The goal of this
patch is to avoid an kernel crash in pskb_expand_head.
It appears that under some specific condition there is a shared
skb reaching the defrag code and this lead to a crash due to the
following code:
if (skb_shared(skb))
BUG();
I've observed this crash under the following condition:
1. a program is listening to an wifi interface (let say wlan0)
2. it is using fanout capture in flow load balancing mode
3. defrag option is on on the fanout socket
4. the interface disconnect (radio down for example)
5. the interface reconnect (radio switched up)
6. once reconnected a single packet is seen with skb->users=2
7. the kernel crash in pskb_expand_head at skbuff.c:1035
[BBB55:744364] [<ffffffff812a2761>] ? __pskb_pull_tail+0x43x0x26f
[BB8S5.744395] [<ffffffff812d29Tb>] ? ip_check_defrag+ox3a/0x14a
[BBB55.744422] [<ffffffffB1344459>] ? packet_rcv_fanout+ox5e/oxf9
[BBBS5.7444S0] [<ffffffffB12aaS9b>] ? __netif_receive_skb+ox444/ox4f9
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? netif_receive_skb+ox6d/0x?3
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_deliver_skb+0xbd/0xfa [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_h_data+0x1e0/0x21a [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_handlers+0x3d5/0x480 [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? __wake_up
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? evdev_eventr+0xc0/0xcf [evdev]
Signed-off-by: Eric Leblond <redacted>
So if we look at ieee80211_deliver_skb(), it has code to deal with unaligned
packet headers, wherein it memoves() the data into a better aligned location.
But if these SKBs really are skb_shared(), this packet data
modification is illegal.
I suspect that the assumptions built into this unaligned data handling
code, and AF_PACKET, are correct. Meaning that we should never see
skb_shared() packets here. We just have a missing skb_copy()
somewhere in mac80211, Johannes can you please take a look?
Here's some more info that may help people knowing the code. During my
test, I've removed the BUG() and replaced with a printk to have a living
kernel. Only one single shared skb was seen for each up event.
I've also add another oops in the same code:
[BBB55:744364] [<ffffffff812a2761>] ? __pskb_pull_tail+0x43x0x26f
[BB8S5.744395] [<ffffffff812d29Tb>] ? ip_check_defrag+ox3a/0x14a
[BBB55.744422] [<ffffffffB1344459>] ? packet_rcv_fanout+ox5e/oxf9
[BBBS5.7444S0] [<ffffffffB12aaS9b>] ? __netif_receive_skb+ox444/ox4f9
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? netif_receive_skb+ox6d/0x?3
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_deliver_skb+0xbd/0xfa [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_h_data+0x1e0/0x21a [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_rx_handlers+0x3d5/0x480 [mac80211]
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? _raw_spin_lock_irqsave+0x14/0x35
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ieee80211_prepare_and_rx_handle+0x5a3/0x5db [mac80211]
...
[BBB55.T4447B] [<ffffffffB12aa?e1>] ? ttwu_dowakeup+0x2d
Picture of the oops available here:
http://home.regit.org/~regit/wireless-oops.jpg
BR,
--
Eric Leblond [off-list ref]
Blog: https://home.regit.org/
From: Johannes Berg <johannes@sipsolutions.net> Date: 2012-12-07 21:30:04
Wireless folks, please take a look. The issue is that,
under the circumstances listed below, we get SKBs in
the AF_PACKET input path that are shared.
Ok so I took a look, but I can't see where the wireless stack is going
wrong.
Given the logic present in ieee80211_deliver_skb() I think
the mac80211 code doesn't expect this either.
This is correct, but the driver should never give us a shared skb. From
the other mail it seems Eric is using iwlwifi, which is definitely not
creating shared SKBs. Nothing in mac80211 creates them either.
quoted
I've observed this crash under the following condition:
1. a program is listening to an wifi interface (let say wlan0)
2. it is using fanout capture in flow load balancing mode
3. defrag option is on on the fanout socket
How do you set this up, and what does it do? I'd like to try to
reproduce this.
quoted
4. the interface disconnect (radio down for example)
5. the interface reconnect (radio switched up)
6. once reconnected a single packet is seen with skb->users=2
That's interesting. A single one seems odd. I might have expected two,
but not one. Well, since you removed the crash ... I guess I'll have to
believe that there's just one and the second one doesn't show up because
we crashed before :-)
So if we look at ieee80211_deliver_skb(), it has code to deal with unaligned
packet headers, wherein it memoves() the data into a better aligned location.
But if these SKBs really are skb_shared(), this packet data
modification is illegal.
I suspect that the assumptions built into this unaligned data handling
code, and AF_PACKET, are correct. Meaning that we should never see
skb_shared() packets here. We just have a missing skb_copy()
somewhere in mac80211, Johannes can you please take a look?
My first theory was related to multiple virtual interfaces, but Eric
didn't say he was running that, but we use skb_copy() for that in
ieee80211_prepare_and_rx_handle(). That's not necessarily the most
efficient (another reason for drivers to use paged RX here) but clearly
not causing the issue.
The only other theory I can come up with right now is that the skb_get()
happens in deliver_skb via __netif_receive_skb. Keeping in mind that
wpa_supplicant might have another packet socket open for authentication
packets, that seems like a possibility. I'll test it once I figure out
how to do this "defrag" option you speak of :)
johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: 2012-12-07 21:41:21
On Fri, 2012-12-07 at 22:30 +0100, Johannes Berg wrote:
The only other theory I can come up with right now is that the skb_get()
happens in deliver_skb via __netif_receive_skb. Keeping in mind that
wpa_supplicant might have another packet socket open for authentication
packets, that seems like a possibility. I'll test it once I figure out
how to do this "defrag" option you speak of :)
Hmm now I'm venturing into the unknown (for me) and realm of
speculation...
wpa_supplicant opens a packet socket for ETH_P_EAPOL, which indirectly
eventually calls dev_add_pack(). But if you do the same for another
socket, you'll get the same again, and then deliver_skb() will deliver
only a refcounted packet to the prot_hook->func().
This seems like it could very well cause the problem?
johannes
From: Eric Leblond <hidden> Date: 2012-12-07 21:46:49
Hi,
On Fri, 2012-12-07 at 22:30 +0100, Johannes Berg wrote:
quoted
Wireless folks, please take a look. The issue is that,
under the circumstances listed below, we get SKBs in
the AF_PACKET input path that are shared.
Ok so I took a look, but I can't see where the wireless stack is going
wrong.
quoted
Given the logic present in ieee80211_deliver_skb() I think
the mac80211 code doesn't expect this either.
This is correct, but the driver should never give us a shared skb. From
the other mail it seems Eric is using iwlwifi, which is definitely not
creating shared SKBs. Nothing in mac80211 creates them either.
quoted
quoted
I've observed this crash under the following condition:
1. a program is listening to an wifi interface (let say wlan0)
2. it is using fanout capture in flow load balancing mode
3. defrag option is on on the fanout socket
How do you set this up, and what does it do? I'd like to try to
reproduce this.
quoted
quoted
4. the interface disconnect (radio down for example)
5. the interface reconnect (radio switched up)
6. once reconnected a single packet is seen with skb->users=2
That's interesting. A single one seems odd. I might have expected two,
but not one. Well, since you removed the crash ... I guess I'll have to
believe that there's just one and the second one doesn't show up because
we crashed before :-)
It was the case with initial code but I've suppressed the BUG() call and
replaced it with a return ;)
quoted
So if we look at ieee80211_deliver_skb(), it has code to deal with unaligned
packet headers, wherein it memoves() the data into a better aligned location.
But if these SKBs really are skb_shared(), this packet data
modification is illegal.
I suspect that the assumptions built into this unaligned data handling
code, and AF_PACKET, are correct. Meaning that we should never see
skb_shared() packets here. We just have a missing skb_copy()
somewhere in mac80211, Johannes can you please take a look?
My first theory was related to multiple virtual interfaces, but Eric
didn't say he was running that, but we use skb_copy() for that in
ieee80211_prepare_and_rx_handle(). That's not necessarily the most
efficient (another reason for drivers to use paged RX here) but clearly
not causing the issue.
The only other theory I can come up with right now is that the skb_get()
happens in deliver_skb via __netif_receive_skb. Keeping in mind that
wpa_supplicant might have another packet socket open for authentication
packets, that seems like a possibility. I'll test it once I figure out
how to do this "defrag" option you speak of :)
I've no simple code available to test it. I've add the problem when
running suricata. Maybe you could use it. It is packaged in most
distribution now.
To enable packet fanout. Modify default /etc/suricata/suricata.yaml to
have something like:
af-packet:
- interface: wlan0
# Number of receive threads (>1 will enable experimental flow pinned
# runmode)
threads: 3
Start it with: suricata --af-packet=wlan0
Then get wlan0 interface down and up. After a few seconds, the crash
will occur.
It is a bit complicated for a simple test case. I can cook a little
example code if you want.
BR,
--
Eric Leblond [off-list ref]
Blog: https://home.regit.org/
From: Johannes Berg <hidden> Date: 2012-12-07 21:56:28
HI,
quoted
That's interesting. A single one seems odd. I might have expected two,
but not one. Well, since you removed the crash ... I guess I'll have to
believe that there's just one and the second one doesn't show up because
we crashed before :-)
It was the case with initial code but I've suppressed the BUG() call and
replaced it with a return ;)
Right. Well, does it actually still work then? I wonder if then you
don't see a second packet because the first one doesn't make it
through ... I'm thinking that this is just an internal af_packet problem
with multiple listeners.
I've no simple code available to test it. I've add the problem when
running suricata. Maybe you could use it. It is packaged in most
distribution now.
To enable packet fanout. Modify default /etc/suricata/suricata.yaml to
have something like:
af-packet:
- interface: wlan0
# Number of receive threads (>1 will enable experimental flow pinned
# runmode)
threads: 3
That'll should do, thanks.
johannes
--
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