Skip to contentSkip to conversation

PROBLEM: pppol2tp over pppoe NULL pointer dereference

PATCH In review Misha Labjuk 4 participants 16 messages Nov 1, 2011 – Nov 8, 2011

Subsystems (3)
Networking Network drivers UDP
lore
Share & tools
lore

Misha Labjuk

author
#0 lore raw

pppol2tp over pppoe NULL pointer dereference

Kernel panic after establishing pppol2tp tunnel over pppoe connection.

Get panic in 5-15 min with 10 mbit/s data transfer speed. pppoe and pppol2tp connections stable separately.

Linux version 3.1.0 (user@host) (gcc version 4.6.1 (Gentoo 4.6.1-r1 p1.0, pie-0.4.5) ) #1 SMP Mon Oct 31 18:48:18 MSK 2011

[  151.913193] L2TP core driver, V2.0
[  151.974584] L2TP netlink interface
[  151.993803] PPPoL2TP kernel driver, V2.0
[  437.496670] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000008
[  437.496683] IP: [<ffffffffa03679dc>] l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.496691] PGD d7840067 PUD cd4e7067 PMD 0
[  437.496697] Oops: 0002 [#1] SMP
[  437.496702] CPU 0
[  437.496704] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core

firewire_sbp2 sit tunnel4 netconsole it87 hwmon_vid coretemp pppoe pppox ppp_generic slhc ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 xt_TCPMSS iptable_mangle ip_tables snd_seq_midi snd_emu10k1_synth snd_emux_synth snd_seq_virmidi snd_seq_midi_emul snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_pcm_oss snd_mixer_oss nfsd lockd nfs_acl auth_rpcgss sunrpc usb_storage usb_libusual uas usbhid ipv6 snd_emu10k1 8250_pnp snd_rawmidi snd_hda_codec_realtek snd_ac97_codec snd_hda_intel snd_hda_codec uhci_hcd ac97_bus snd_pcm ehci_hcd usbcore snd_seq_device snd_timer 8250 snd_util_mem snd_hwdep psmouse snd firewire_ohci firewire_core serial_core intel_agp intel_gtt pcspkr soundcore r8169 crc_itu_t mii snd_page_alloc processor button

[  437.497005]
[  437.497005] Pid: 3274, comm: qbittorrent Not tainted 3.1.0 #1
Gigabyte Technology Co., Ltd. EP45-EXTREME/EP45-EXTREME
[  437.497005] RIP: 0010:[<ffffffffa03679dc>]  [<ffffffffa03679dc>]
l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.497005] RSP: 0000:ffff88011fc03b90  EFLAGS: 00010296
[  437.497005] RAX: 0000000000000000 RBX: ffff8800d79e8200 RCX: ffff88011fc10bd0
[  437.497005] RDX: 0000000000000000 RSI: 0000000000004002 RDI: ffff8800d79e8254
[  437.497005] RBP: ffff88011fc03be0 R08: 0000000000004002 R09: 0000000000004002
[  437.497005] R10: ffff8801091ec87a R11: ffff88011b300000 R12: ffff880118922300
[  437.497005] R13: 0000000000000000 R14: ffff8800d79e8254 R15: ffff8800d79e826c
[  437.497005] FS:  00007f0ced3e8700(0000) GS:ffff88011fc00000(0000)
knlGS:0000000000000000
[  437.497005] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  437.497005] CR2: 0000000000000008 CR3: 00000000c8811000 CR4: 00000000000406f0
[  437.497005] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  437.497005] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  437.497005] Process qbittorrent (pid: 3274, threadinfo
ffff8800c9906000, task ffff8800db999200)
[  437.497005] Stack:
[  437.497005]  ffff88011fc03bb0 ffff8801091ec872 ffff8800d79e8240
0000052000000520
[  437.497005]  ffff88011fc03be0 ffff8800d7844e00 ffff880119099200
ffff8801091ec872
[  437.497005]  ffff8800db8b6400 00000000000050cd ffff88011fc03c60
ffffffffa0367e65
[  437.497005] Call Trace:
[  437.497005]  <IRQ>
[  437.497005]  [<ffffffffa0367e65>] l2tp_udp_encap_recv+0x33b/0x3e6 [l2tp_core]
[  437.497005]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.497005]  [<ffffffffa030640c>] ? ipv4_confirm+0x17e/0x198
[nf_conntrack_ipv4]
[  437.497005]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.497005]  [<ffffffff812eb32b>] udp_queue_rcv_skb+0xee/0x2ce
[  437.497005]  [<ffffffff812ebba3>] __udp4_lib_rcv+0x2d2/0x536
[  437.497005]  [<ffffffff812cb046>] ? ip_rcv_finish+0x29a/0x29a
[  437.497005]  [<ffffffff812ebe1c>] udp_rcv+0x15/0x17
[  437.497005]  [<ffffffff812cb165>] ip_local_deliver_finish+0x11f/0x1c7
[  437.497005]  [<ffffffff812cb361>] ip_local_deliver+0x75/0x7c
[  437.497005]  [<ffffffff812cb023>] ip_rcv_finish+0x277/0x29a
[  437.497005]  [<ffffffff812cb5a1>] ip_rcv+0x239/0x260
[  437.497005]  [<ffffffff812a46cd>] ? napi_skb_finish+0x21/0x38
[  437.497005]  [<ffffffff812a3adb>] __netif_receive_skb+0x430/0x462
[  437.497005]  [<ffffffff8102342f>] ? update_curr+0x53/0x89
[  437.497005]  [<ffffffff812a3b9d>] process_backlog+0x90/0x151
[  437.497005]  [<ffffffff812a3e6a>] net_rx_action+0x9e/0x171
[  437.497005]  [<ffffffff810344b0>] __do_softirq+0x93/0x129
[  437.497005]  [<ffffffff8133034c>] call_softirq+0x1c/0x30
[  437.497005]  [<ffffffff8100351c>] do_softirq+0x33/0x6b
[  437.497005]  [<ffffffff8103470b>] irq_exit+0x52/0xac
[  437.497005]  [<ffffffff81003251>] do_IRQ+0x98/0xaf
[  437.497005]  [<ffffffff8132eb6b>] common_interrupt+0x6b/0x6b
[  437.497005]  <EOI>
[  437.497005]  [<ffffffff8132f17b>] ? system_call_fastpath+0x16/0x1b
[  437.497005] Code: 6c e8 57 03 fc e0 e9 22 01 00 00 ff 4b 50 4c 89
f7 49 8b 14 24 49 c7 04 24 00 00 00 00 49 8b 44 24 08 49 c7 44 24 08
00 00 00 00
[  437.497005]  89 42 08 48 89 10 e8 1d 6f fc e0 41 0f b7 54 24 3e 48 8b 43
[  437.497005] RIP  [<ffffffffa03679dc>] l2tp_recv_common+0x4d3/0x621
[l2tp_core]
[  437.497005]  RSP <ffff88011fc03b90>
[  437.497005] CR2: 0000000000000008
[  437.498126] ---[ end trace 053df4c7c6743d26 ]---
[  437.498184] Kernel panic - not syncing: Fatal exception in interrupt
[  437.498187] Pid: 3274, comm: qbittorrent Tainted: G      D     3.1.0 #1
[  437.498189] Call Trace:
[  437.498190]  <IRQ>  [<ffffffff81327c11>] panic+0x8c/0x189
[  437.498197]  [<ffffffff8100471d>] oops_end+0x81/0x8e
[  437.498200]  [<ffffffff8132765b>] no_context+0x1fe/0x20d
[  437.498203]  [<ffffffff81327829>] __bad_area_nosemaphore+0x1bf/0x1e0
[  437.498206]  [<ffffffff812a5308>] ? dev_hard_start_xmit+0x412/0x51b
[  437.498210]  [<ffffffff81327858>] bad_area_nosemaphore+0xe/0x10
[  437.498213]  [<ffffffff8101c858>] do_page_fault+0x175/0x371
[  437.498217]  [<ffffffff812a1eba>] ? netif_rx+0xc5/0xd0
[  437.498281]  [<ffffffffa031ee7d>] ?
ppp_receive_nonmp_frame+0x58f/0x5cf [ppp_generic]
[  437.498286]  [<ffffffffa0320625>] ? ppp_receive_frame+0x5c1/0x5e2
[ppp_generic]
[  437.498290]  [<ffffffff8132ed6f>] page_fault+0x1f/0x30
[  437.498293]  [<ffffffffa03679dc>] ? l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.498298]  [<ffffffffa0367e65>] l2tp_udp_encap_recv+0x33b/0x3e6 [l2tp_core]
[  437.498302]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.498306]  [<ffffffffa030640c>] ? ipv4_confirm+0x17e/0x198
[nf_conntrack_ipv4]
[  437.498310]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.498314]  [<ffffffff812eb32b>] udp_queue_rcv_skb+0xee/0x2ce
[  437.498317]  [<ffffffff812ebba3>] __udp4_lib_rcv+0x2d2/0x536
[  437.498321]  [<ffffffff812cb046>] ? ip_rcv_finish+0x29a/0x29a
[  437.498324]  [<ffffffff812ebe1c>] udp_rcv+0x15/0x17
[  437.498328]  [<ffffffff812cb165>] ip_local_deliver_finish+0x11f/0x1c7
[  437.498332]  [<ffffffff812cb361>] ip_local_deliver+0x75/0x7c
[  437.498391]  [<ffffffff812cb023>] ip_rcv_finish+0x277/0x29a
[  437.498394]  [<ffffffff812cb5a1>] ip_rcv+0x239/0x260
[  437.498398]  [<ffffffff812a46cd>] ? napi_skb_finish+0x21/0x38
[  437.498401]  [<ffffffff812a3adb>] __netif_receive_skb+0x430/0x462
[  437.498404]  [<ffffffff8102342f>] ? update_curr+0x53/0x89
[  437.498408]  [<ffffffff812a3b9d>] process_backlog+0x90/0x151

Software:
Gnu C 4.6.1
Gnu make 3.82
binutils 2.21.1
openl2tp 1.8-r3

l2tp_recv_common+0x4d3/0x621 is match to net/l2tp/l2tp_core.c:429:__skb_unlink(skb, &session->reorder_q); skb->next is NULL.

#1 lore raw
Quoted 140 lines · Misha Labjuk

pppol2tp over pppoe NULL pointer dereference

Kernel panic after establishing pppol2tp tunnel over pppoe connection.

Get panic in 5-15 min with 10 mbit/s data transfer speed. pppoe and pppol2tp connections stable separately.

Linux version 3.1.0 (user@host) (gcc version 4.6.1 (Gentoo 4.6.1-r1 p1.0, pie-0.4.5) ) #1 SMP Mon Oct 31 18:48:18 MSK 2011

[  151.913193] L2TP core driver, V2.0
[  151.974584] L2TP netlink interface
[  151.993803] PPPoL2TP kernel driver, V2.0
[  437.496670] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000008
[  437.496683] IP: [<ffffffffa03679dc>] l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.496691] PGD d7840067 PUD cd4e7067 PMD 0
[  437.496697] Oops: 0002 [#1] SMP
[  437.496702] CPU 0
[  437.496704] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core

firewire_sbp2 sit tunnel4 netconsole it87 hwmon_vid coretemp pppoe pppox ppp_generic slhc ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 xt_TCPMSS iptable_mangle ip_tables snd_seq_midi snd_emu10k1_synth snd_emux_synth snd_seq_virmidi snd_seq_midi_emul snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_pcm_oss snd_mixer_oss nfsd lockd nfs_acl auth_rpcgss sunrpc usb_storage usb_libusual uas usbhid ipv6 snd_emu10k1 8250_pnp snd_rawmidi snd_hda_codec_realtek snd_ac97_codec snd_hda_intel snd_hda_codec uhci_hcd ac97_bus snd_pcm ehci_hcd usbcore snd_seq_device snd_timer 8250 snd_util_mem snd_hwdep psmouse snd firewire_ohci firewire_core serial_core intel_agp intel_gtt pcspkr soundcore r8169 crc_itu_t mii snd_page_alloc processor button

[  437.497005]
[  437.497005] Pid: 3274, comm: qbittorrent Not tainted 3.1.0 #1
Gigabyte Technology Co., Ltd. EP45-EXTREME/EP45-EXTREME
[  437.497005] RIP: 0010:[<ffffffffa03679dc>]  [<ffffffffa03679dc>]
l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.497005] RSP: 0000:ffff88011fc03b90  EFLAGS: 00010296
[  437.497005] RAX: 0000000000000000 RBX: ffff8800d79e8200 RCX: ffff88011fc10bd0
[  437.497005] RDX: 0000000000000000 RSI: 0000000000004002 RDI: ffff8800d79e8254
[  437.497005] RBP: ffff88011fc03be0 R08: 0000000000004002 R09: 0000000000004002
[  437.497005] R10: ffff8801091ec87a R11: ffff88011b300000 R12: ffff880118922300
[  437.497005] R13: 0000000000000000 R14: ffff8800d79e8254 R15: ffff8800d79e826c
[  437.497005] FS:  00007f0ced3e8700(0000) GS:ffff88011fc00000(0000)
knlGS:0000000000000000
[  437.497005] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  437.497005] CR2: 0000000000000008 CR3: 00000000c8811000 CR4: 00000000000406f0
[  437.497005] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  437.497005] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  437.497005] Process qbittorrent (pid: 3274, threadinfo
ffff8800c9906000, task ffff8800db999200)
[  437.497005] Stack:
[  437.497005]  ffff88011fc03bb0 ffff8801091ec872 ffff8800d79e8240
0000052000000520
[  437.497005]  ffff88011fc03be0 ffff8800d7844e00 ffff880119099200
ffff8801091ec872
[  437.497005]  ffff8800db8b6400 00000000000050cd ffff88011fc03c60
ffffffffa0367e65
[  437.497005] Call Trace:
[  437.497005]  <IRQ>
[  437.497005]  [<ffffffffa0367e65>] l2tp_udp_encap_recv+0x33b/0x3e6 [l2tp_core]
[  437.497005]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.497005]  [<ffffffffa030640c>] ? ipv4_confirm+0x17e/0x198
[nf_conntrack_ipv4]
[  437.497005]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.497005]  [<ffffffff812eb32b>] udp_queue_rcv_skb+0xee/0x2ce
[  437.497005]  [<ffffffff812ebba3>] __udp4_lib_rcv+0x2d2/0x536
[  437.497005]  [<ffffffff812cb046>] ? ip_rcv_finish+0x29a/0x29a
[  437.497005]  [<ffffffff812ebe1c>] udp_rcv+0x15/0x17
[  437.497005]  [<ffffffff812cb165>] ip_local_deliver_finish+0x11f/0x1c7
[  437.497005]  [<ffffffff812cb361>] ip_local_deliver+0x75/0x7c
[  437.497005]  [<ffffffff812cb023>] ip_rcv_finish+0x277/0x29a
[  437.497005]  [<ffffffff812cb5a1>] ip_rcv+0x239/0x260
[  437.497005]  [<ffffffff812a46cd>] ? napi_skb_finish+0x21/0x38
[  437.497005]  [<ffffffff812a3adb>] __netif_receive_skb+0x430/0x462
[  437.497005]  [<ffffffff8102342f>] ? update_curr+0x53/0x89
[  437.497005]  [<ffffffff812a3b9d>] process_backlog+0x90/0x151
[  437.497005]  [<ffffffff812a3e6a>] net_rx_action+0x9e/0x171
[  437.497005]  [<ffffffff810344b0>] __do_softirq+0x93/0x129
[  437.497005]  [<ffffffff8133034c>] call_softirq+0x1c/0x30
[  437.497005]  [<ffffffff8100351c>] do_softirq+0x33/0x6b
[  437.497005]  [<ffffffff8103470b>] irq_exit+0x52/0xac
[  437.497005]  [<ffffffff81003251>] do_IRQ+0x98/0xaf
[  437.497005]  [<ffffffff8132eb6b>] common_interrupt+0x6b/0x6b
[  437.497005]  <EOI>
[  437.497005]  [<ffffffff8132f17b>] ? system_call_fastpath+0x16/0x1b
[  437.497005] Code: 6c e8 57 03 fc e0 e9 22 01 00 00 ff 4b 50 4c 89
f7 49 8b 14 24 49 c7 04 24 00 00 00 00 49 8b 44 24 08 49 c7 44 24 08
00 00 00 00
[  437.497005]  89 42 08 48 89 10 e8 1d 6f fc e0 41 0f b7 54 24 3e 48 8b 43
[  437.497005] RIP  [<ffffffffa03679dc>] l2tp_recv_common+0x4d3/0x621
[l2tp_core]
[  437.497005]  RSP <ffff88011fc03b90>
[  437.497005] CR2: 0000000000000008
[  437.498126] ---[ end trace 053df4c7c6743d26 ]---
[  437.498184] Kernel panic - not syncing: Fatal exception in interrupt
[  437.498187] Pid: 3274, comm: qbittorrent Tainted: G      D     3.1.0 #1
[  437.498189] Call Trace:
[  437.498190]  <IRQ>  [<ffffffff81327c11>] panic+0x8c/0x189
[  437.498197]  [<ffffffff8100471d>] oops_end+0x81/0x8e
[  437.498200]  [<ffffffff8132765b>] no_context+0x1fe/0x20d
[  437.498203]  [<ffffffff81327829>] __bad_area_nosemaphore+0x1bf/0x1e0
[  437.498206]  [<ffffffff812a5308>] ? dev_hard_start_xmit+0x412/0x51b
[  437.498210]  [<ffffffff81327858>] bad_area_nosemaphore+0xe/0x10
[  437.498213]  [<ffffffff8101c858>] do_page_fault+0x175/0x371
[  437.498217]  [<ffffffff812a1eba>] ? netif_rx+0xc5/0xd0
[  437.498281]  [<ffffffffa031ee7d>] ?
ppp_receive_nonmp_frame+0x58f/0x5cf [ppp_generic]
[  437.498286]  [<ffffffffa0320625>] ? ppp_receive_frame+0x5c1/0x5e2
[ppp_generic]
[  437.498290]  [<ffffffff8132ed6f>] page_fault+0x1f/0x30
[  437.498293]  [<ffffffffa03679dc>] ? l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.498298]  [<ffffffffa0367e65>] l2tp_udp_encap_recv+0x33b/0x3e6 [l2tp_core]
[  437.498302]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.498306]  [<ffffffffa030640c>] ? ipv4_confirm+0x17e/0x198
[nf_conntrack_ipv4]
[  437.498310]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.498314]  [<ffffffff812eb32b>] udp_queue_rcv_skb+0xee/0x2ce
[  437.498317]  [<ffffffff812ebba3>] __udp4_lib_rcv+0x2d2/0x536
[  437.498321]  [<ffffffff812cb046>] ? ip_rcv_finish+0x29a/0x29a
[  437.498324]  [<ffffffff812ebe1c>] udp_rcv+0x15/0x17
[  437.498328]  [<ffffffff812cb165>] ip_local_deliver_finish+0x11f/0x1c7
[  437.498332]  [<ffffffff812cb361>] ip_local_deliver+0x75/0x7c
[  437.498391]  [<ffffffff812cb023>] ip_rcv_finish+0x277/0x29a
[  437.498394]  [<ffffffff812cb5a1>] ip_rcv+0x239/0x260
[  437.498398]  [<ffffffff812a46cd>] ? napi_skb_finish+0x21/0x38
[  437.498401]  [<ffffffff812a3adb>] __netif_receive_skb+0x430/0x462
[  437.498404]  [<ffffffff8102342f>] ? update_curr+0x53/0x89
[  437.498408]  [<ffffffff812a3b9d>] process_backlog+0x90/0x151

Software:
Gnu C 4.6.1
Gnu make 3.82
binutils 2.21.1
openl2tp 1.8-r3

Quoted 3 lines

l2tp_recv_common+0x4d3/0x621 is match to net/l2tp/l2tp_core.c:429:__skb_unlink(skb, &session->reorder_q); skb->next is NULL.

Hi Misha

On what kind of NIC this is happening ?

author
#2 lore raw

2011/11/2 Eric Dumazet <[email protected]>:

Quoted 3 lines

On what kind of NIC this is happening ?

Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 02)
Kernel driver in use: r8169

#3 lore raw
Quoted 8 lines · Misha Labjuk

2011/11/2 Eric Dumazet <[email protected]>:

Quoted 3 lines

On what kind of NIC this is happening ?

Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 02)
Kernel driver in use: r8169

OK thanks, could you try the following patch as well ?

If we release reorder_q.lock, we must not keep a dangling pointer (tmp) and restart the whole loop.

net/l2tp/l2tp_core.c +2 -1
index 34b2dde..bf8d50c 100644
@@ -397,6 +397,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
 	 * expect to send up next, dequeue it and any other
 	 * in-sequence packets behind it.
 	 */
+start:
 	spin_lock_bh(&session->reorder_q.lock);
 	skb_queue_walk_safe(&session->reorder_q, skb, tmp) {
 		if (time_after(jiffies, L2TP_SKB_CB(skb)->expires)) {

@@ -433,7 +434,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
 		 */
 		spin_unlock_bh(&session->reorder_q.lock);
 		l2tp_recv_dequeue_skb(session, skb);
-		spin_lock_bh(&session->reorder_q.lock);
+		goto start;
 	}
 
 out:
#4 lore raw
Quoted 39 lines · Eric Dumazet
Quoted 8 lines · Misha Labjuk

2011/11/2 Eric Dumazet <[email protected]>:

Quoted 3 lines

On what kind of NIC this is happening ?

Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 02)
Kernel driver in use: r8169

OK thanks, could you try the following patch as well ?

If we release reorder_q.lock, we must not keep a dangling pointer (tmp) and restart the whole loop.

net/l2tp/l2tp_core.c +2 -2
index 34b2dde..bf8d50c 100644
@@ -397,6 +397,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
 	 * expect to send up next, dequeue it and any other
 	 * in-sequence packets behind it.
 	 */
+start:
 	spin_lock_bh(&session->reorder_q.lock);
 	skb_queue_walk_safe(&session->reorder_q, skb, tmp) {
 		if (time_after(jiffies, L2TP_SKB_CB(skb)->expires)) {

@@ -433,7 +434,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
 		 */
 		spin_unlock_bh(&session->reorder_q.lock);
 		l2tp_recv_dequeue_skb(session, skb);
-		spin_lock_bh(&session->reorder_q.lock);
+		goto start;
 	}
 
 out:
 
 
--
Quoted 5 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I've been using this same exact patch on an old kernel since a while ago. I had
one system that crashed here, now decommissioned. After some testing I was unable to reproduce the bug in another systems but on the one that exhibited the problem it fixed the crashes.

Regards,
Jorge

Signature · 10 lines
==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- There is only so much duct tape you can put on something
  before it just becomes a giant ball of duct tape.
==============================================================
author
#5 lore raw
In reply to Eric Dumazet

Thanks!! Panic disappeared.

2011/11/2 Eric Dumazet <[email protected]>:

Quoted 27 lines

OK thanks, could you try the following patch as well ?

If we release reorder_q.lock, we must not keep a dangling pointer (tmp) and restart the whole loop.

net/l2tp/l2tp_core.c +2 -1
index 34b2dde..bf8d50c 100644
@@ -397,6 +397,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
         * expect to send up next, dequeue it and any other
         * in-sequence packets behind it.
         */
+start:
        spin_lock_bh(&session->reorder_q.lock);
        skb_queue_walk_safe(&session->reorder_q, skb, tmp) {
                if (time_after(jiffies, L2TP_SKB_CB(skb)->expires)) {

@@ -433,7 +434,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
                 */
                spin_unlock_bh(&session->reorder_q.lock);
                l2tp_recv_dequeue_skb(session, skb);
-               spin_lock_bh(&session->reorder_q.lock);
+               goto start;
        }

 out:

#6 lore raw

PATCH 1/1 l2tp: fix l2tp_recv_dequeue()

Quoted 3 lines · Misha Labjuk

Thanks!! Panic disappeared.

Thanks !

Here is the official submission then :)

[PATCH] l2tp: fix l2tp_recv_dequeue()

Misha Labjuk reported panics occurring in l2tp_recv_dequeue()

If we release reorder_q.lock, we must not keep a dangling pointer (tmp) and restart the whole loop.

Reported-by Misha Labjuk <[email protected]>
Tested-by Misha Labjuk <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>
---
 net/l2tp/l2tp_core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/l2tp/l2tp_core.c +0 -0
index 34b2dde..bf8d50c 100644
@@ -397,6 +397,7 @@ static void l2tp_recv_dequeue(struct l2tp_session

*session)
* expect to send up next, dequeue it and any other
* in-sequence packets behind it.
*/
+start:
spin_lock_bh(&session->reorder_q.lock); skb_queue_walk_safe(&session->reorder_q, skb, tmp) {
if (time_after(jiffies, L2TP_SKB_CB(skb)->expires)) {
@@ -433,7 +434,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
*/
spin_unlock_bh(&session->reorder_q.lock); l2tp_recv_dequeue_skb(session, skb);
- spin_lock_bh(&session->reorder_q.lock);
+ goto start;
}

out:

#7 lore raw

Re: [PATCH] l2tp: fix l2tp_recv_dequeue()

net/l2tp/l2tp_core.c static void l2tp_recv_dequeue(struct l2tp_session · line 398

Quoted 7 lines · Eric Dumazet
net/l2tp/l2tp_core.c +0 -0
index 34b2dde..bf8d50c 100644
@@ -397,6 +397,7 @@ static void l2tp_recv_dequeue(struct l2tp_session
 *session)
 	 * expect to send up next, dequeue it and any other

Oh well, thunderbird mangled this patch, I'll repost it when I get back my laptop (evolution instead of thundercrap) in ~one hour, if its still on my desk at office :(

Sorry David.

Tested-by Misha Labjuk
#8 lore raw
In reply to Misha Labjuk
Quoted 2 lines · Misha Labjuk

Thanks!! Panic disappeared.

Here is the official patch submission again (not mangled this time), thanks !

[PATCH] l2tp: fix race in l2tp_recv_dequeue()

Misha Labjuk reported panics occurring in l2tp_recv_dequeue()

If we release reorder_q.lock, we must not keep a dangling pointer (tmp), since another thread could manipulate reorder_q.

Instead we must restart the scan at beginning of list.

Reported-by Misha Labjuk <[email protected]>
Tested-by Misha Labjuk <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>
---
 net/l2tp/l2tp_core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/l2tp/l2tp_core.c +2 -1
index 34b2dde..bf8d50c 100644
@@ -397,6 +397,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
 	 * expect to send up next, dequeue it and any other
 	 * in-sequence packets behind it.
 	 */
+start:
 	spin_lock_bh(&session->reorder_q.lock);
 	skb_queue_walk_safe(&session->reorder_q, skb, tmp) {
 		if (time_after(jiffies, L2TP_SKB_CB(skb)->expires)) {

@@ -433,7 +434,7 @@ static void l2tp_recv_dequeue(struct l2tp_session *session)
 		 */
 		spin_unlock_bh(&session->reorder_q.lock);
 		l2tp_recv_dequeue_skb(session, skb);
-		spin_lock_bh(&session->reorder_q.lock);
+		goto start;
 	}
 
 out:
maintainer
#9 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Nov 2011 09:47:44 +0100

Quoted 8 lines · Misha Labjuk
Quoted 2 lines

Thanks!! Panic disappeared.

Here is the official patch submission again (not mangled this time), thanks !

[PATCH] l2tp: fix race in l2tp_recv_dequeue()

Applied and queued up for -stable, thanks!

#10 lore raw

PATCH 1/1 udp: fix a race in encap_rcv handling

In reply to Misha Labjuk

udp_queue_rcv_skb() has a possible race in encap_rcv handling, since this pointer can be changed anytime.

We should use ACCESS_ONCE() to close the race.

Signed-off-by Eric Dumazet <[email protected]>
---
 net/ipv4/udp.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
net/ipv4/udp.c +5 -3
index ebaa96b..15b50df 100644
@@ -1397,6 +1397,8 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	nf_reset(skb);
 
 	if (up->encap_type) {
+		int (*encap_rcv)(struct sock *sk, struct sk_buff *skb);
+
 		/*
 		 * This is an encapsulation socket so pass the skb to
 		 * the socket's udp_encap_rcv() hook. Otherwise, just

@@ -1409,11 +1411,11 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		 */
 
 		/* if we're overly short, let UDP handle it */
-		if (skb->len > sizeof(struct udphdr) &&
-		    up->encap_rcv != NULL) {
+		encap_rcv = ACCESS_ONCE(up->encap_rcv);
+		if (skb->len > sizeof(struct udphdr) && encap_rcv != NULL) {
 			int ret;
 
-			ret = (*up->encap_rcv)(sk, skb);
+			ret = encap_rcv(sk, skb);
 			if (ret <= 0) {
 				UDP_INC_STATS_BH(sock_net(sk),
 						 UDP_MIB_INDATAGRAMS,
maintainer
#11 lore raw

Re: [PATCH] udp: fix a race in encap_rcv handling

From: Eric Dumazet <[email protected]>
Date: Tue, 01 Nov 2011 23:56:59 +0100

Quoted 6 lines

udp_queue_rcv_skb() has a possible race in encap_rcv handling, since this pointer can be changed anytime.

We should use ACCESS_ONCE() to close the race.

Signed-off-by Eric Dumazet <[email protected]>

Applied and queued up for -stable, thanks!

#12 lore raw
In reply to Misha Labjuk
Quoted 140 lines · Misha Labjuk

pppol2tp over pppoe NULL pointer dereference

Kernel panic after establishing pppol2tp tunnel over pppoe connection.

Get panic in 5-15 min with 10 mbit/s data transfer speed. pppoe and pppol2tp connections stable separately.

Linux version 3.1.0 (user@host) (gcc version 4.6.1 (Gentoo 4.6.1-r1 p1.0, pie-0.4.5) ) #1 SMP Mon Oct 31 18:48:18 MSK 2011

[  151.913193] L2TP core driver, V2.0
[  151.974584] L2TP netlink interface
[  151.993803] PPPoL2TP kernel driver, V2.0
[  437.496670] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000008
[  437.496683] IP: [<ffffffffa03679dc>] l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.496691] PGD d7840067 PUD cd4e7067 PMD 0
[  437.496697] Oops: 0002 [#1] SMP
[  437.496702] CPU 0
[  437.496704] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core

firewire_sbp2 sit tunnel4 netconsole it87 hwmon_vid coretemp pppoe pppox ppp_generic slhc ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 xt_TCPMSS iptable_mangle ip_tables snd_seq_midi snd_emu10k1_synth snd_emux_synth snd_seq_virmidi snd_seq_midi_emul snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_pcm_oss snd_mixer_oss nfsd lockd nfs_acl auth_rpcgss sunrpc usb_storage usb_libusual uas usbhid ipv6 snd_emu10k1 8250_pnp snd_rawmidi snd_hda_codec_realtek snd_ac97_codec snd_hda_intel snd_hda_codec uhci_hcd ac97_bus snd_pcm ehci_hcd usbcore snd_seq_device snd_timer 8250 snd_util_mem snd_hwdep psmouse snd firewire_ohci firewire_core serial_core intel_agp intel_gtt pcspkr soundcore r8169 crc_itu_t mii snd_page_alloc processor button

[  437.497005]
[  437.497005] Pid: 3274, comm: qbittorrent Not tainted 3.1.0 #1
Gigabyte Technology Co., Ltd. EP45-EXTREME/EP45-EXTREME
[  437.497005] RIP: 0010:[<ffffffffa03679dc>]  [<ffffffffa03679dc>]
l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.497005] RSP: 0000:ffff88011fc03b90  EFLAGS: 00010296
[  437.497005] RAX: 0000000000000000 RBX: ffff8800d79e8200 RCX: ffff88011fc10bd0
[  437.497005] RDX: 0000000000000000 RSI: 0000000000004002 RDI: ffff8800d79e8254
[  437.497005] RBP: ffff88011fc03be0 R08: 0000000000004002 R09: 0000000000004002
[  437.497005] R10: ffff8801091ec87a R11: ffff88011b300000 R12: ffff880118922300
[  437.497005] R13: 0000000000000000 R14: ffff8800d79e8254 R15: ffff8800d79e826c
[  437.497005] FS:  00007f0ced3e8700(0000) GS:ffff88011fc00000(0000)
knlGS:0000000000000000
[  437.497005] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  437.497005] CR2: 0000000000000008 CR3: 00000000c8811000 CR4: 00000000000406f0
[  437.497005] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  437.497005] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  437.497005] Process qbittorrent (pid: 3274, threadinfo
ffff8800c9906000, task ffff8800db999200)
[  437.497005] Stack:
[  437.497005]  ffff88011fc03bb0 ffff8801091ec872 ffff8800d79e8240
0000052000000520
[  437.497005]  ffff88011fc03be0 ffff8800d7844e00 ffff880119099200
ffff8801091ec872
[  437.497005]  ffff8800db8b6400 00000000000050cd ffff88011fc03c60
ffffffffa0367e65
[  437.497005] Call Trace:
[  437.497005]  <IRQ>
[  437.497005]  [<ffffffffa0367e65>] l2tp_udp_encap_recv+0x33b/0x3e6 [l2tp_core]
[  437.497005]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.497005]  [<ffffffffa030640c>] ? ipv4_confirm+0x17e/0x198
[nf_conntrack_ipv4]
[  437.497005]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.497005]  [<ffffffff812eb32b>] udp_queue_rcv_skb+0xee/0x2ce
[  437.497005]  [<ffffffff812ebba3>] __udp4_lib_rcv+0x2d2/0x536
[  437.497005]  [<ffffffff812cb046>] ? ip_rcv_finish+0x29a/0x29a
[  437.497005]  [<ffffffff812ebe1c>] udp_rcv+0x15/0x17
[  437.497005]  [<ffffffff812cb165>] ip_local_deliver_finish+0x11f/0x1c7
[  437.497005]  [<ffffffff812cb361>] ip_local_deliver+0x75/0x7c
[  437.497005]  [<ffffffff812cb023>] ip_rcv_finish+0x277/0x29a
[  437.497005]  [<ffffffff812cb5a1>] ip_rcv+0x239/0x260
[  437.497005]  [<ffffffff812a46cd>] ? napi_skb_finish+0x21/0x38
[  437.497005]  [<ffffffff812a3adb>] __netif_receive_skb+0x430/0x462
[  437.497005]  [<ffffffff8102342f>] ? update_curr+0x53/0x89
[  437.497005]  [<ffffffff812a3b9d>] process_backlog+0x90/0x151
[  437.497005]  [<ffffffff812a3e6a>] net_rx_action+0x9e/0x171
[  437.497005]  [<ffffffff810344b0>] __do_softirq+0x93/0x129
[  437.497005]  [<ffffffff8133034c>] call_softirq+0x1c/0x30
[  437.497005]  [<ffffffff8100351c>] do_softirq+0x33/0x6b
[  437.497005]  [<ffffffff8103470b>] irq_exit+0x52/0xac
[  437.497005]  [<ffffffff81003251>] do_IRQ+0x98/0xaf
[  437.497005]  [<ffffffff8132eb6b>] common_interrupt+0x6b/0x6b
[  437.497005]  <EOI>
[  437.497005]  [<ffffffff8132f17b>] ? system_call_fastpath+0x16/0x1b
[  437.497005] Code: 6c e8 57 03 fc e0 e9 22 01 00 00 ff 4b 50 4c 89
f7 49 8b 14 24 49 c7 04 24 00 00 00 00 49 8b 44 24 08 49 c7 44 24 08
00 00 00 00
[  437.497005]  89 42 08 48 89 10 e8 1d 6f fc e0 41 0f b7 54 24 3e 48 8b 43
[  437.497005] RIP  [<ffffffffa03679dc>] l2tp_recv_common+0x4d3/0x621
[l2tp_core]
[  437.497005]  RSP <ffff88011fc03b90>
[  437.497005] CR2: 0000000000000008
[  437.498126] ---[ end trace 053df4c7c6743d26 ]---
[  437.498184] Kernel panic - not syncing: Fatal exception in interrupt
[  437.498187] Pid: 3274, comm: qbittorrent Tainted: G      D     3.1.0 #1
[  437.498189] Call Trace:
[  437.498190]  <IRQ>  [<ffffffff81327c11>] panic+0x8c/0x189
[  437.498197]  [<ffffffff8100471d>] oops_end+0x81/0x8e
[  437.498200]  [<ffffffff8132765b>] no_context+0x1fe/0x20d
[  437.498203]  [<ffffffff81327829>] __bad_area_nosemaphore+0x1bf/0x1e0
[  437.498206]  [<ffffffff812a5308>] ? dev_hard_start_xmit+0x412/0x51b
[  437.498210]  [<ffffffff81327858>] bad_area_nosemaphore+0xe/0x10
[  437.498213]  [<ffffffff8101c858>] do_page_fault+0x175/0x371
[  437.498217]  [<ffffffff812a1eba>] ? netif_rx+0xc5/0xd0
[  437.498281]  [<ffffffffa031ee7d>] ?
ppp_receive_nonmp_frame+0x58f/0x5cf [ppp_generic]
[  437.498286]  [<ffffffffa0320625>] ? ppp_receive_frame+0x5c1/0x5e2
[ppp_generic]
[  437.498290]  [<ffffffff8132ed6f>] page_fault+0x1f/0x30
[  437.498293]  [<ffffffffa03679dc>] ? l2tp_recv_common+0x4d3/0x621 [l2tp_core]
[  437.498298]  [<ffffffffa0367e65>] l2tp_udp_encap_recv+0x33b/0x3e6 [l2tp_core]
[  437.498302]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.498306]  [<ffffffffa030640c>] ? ipv4_confirm+0x17e/0x198
[nf_conntrack_ipv4]
[  437.498310]  [<ffffffffa0377d96>] ? pppol2tp_setsockopt+0x2e0/0x2e0
[l2tp_ppp]
[  437.498314]  [<ffffffff812eb32b>] udp_queue_rcv_skb+0xee/0x2ce
[  437.498317]  [<ffffffff812ebba3>] __udp4_lib_rcv+0x2d2/0x536
[  437.498321]  [<ffffffff812cb046>] ? ip_rcv_finish+0x29a/0x29a
[  437.498324]  [<ffffffff812ebe1c>] udp_rcv+0x15/0x17
[  437.498328]  [<ffffffff812cb165>] ip_local_deliver_finish+0x11f/0x1c7
[  437.498332]  [<ffffffff812cb361>] ip_local_deliver+0x75/0x7c
[  437.498391]  [<ffffffff812cb023>] ip_rcv_finish+0x277/0x29a
[  437.498394]  [<ffffffff812cb5a1>] ip_rcv+0x239/0x260
[  437.498398]  [<ffffffff812a46cd>] ? napi_skb_finish+0x21/0x38
[  437.498401]  [<ffffffff812a3adb>] __netif_receive_skb+0x430/0x462
[  437.498404]  [<ffffffff8102342f>] ? update_curr+0x53/0x89
[  437.498408]  [<ffffffff812a3b9d>] process_backlog+0x90/0x151

Software:
Gnu C 4.6.1
Gnu make 3.82
binutils 2.21.1
openl2tp 1.8-r3

Quoted 3 lines

l2tp_recv_common+0x4d3/0x621 is match to net/l2tp/l2tp_core.c:429:__skb_unlink(skb, &session->reorder_q); skb->next is NULL.

Please try following patch, thanks !

[PATCH] l2tp: handle fragmented skbs in receive path

Modern drivers provide skb with fragments, and L2TP doesnt properly handles them.

Some bad frames can also trigger panics because of insufficent checks.

Reported-by Misha Labjuk <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>
---
 net/l2tp/l2tp_core.c |   71 +++++++++++++++++++++++------------------
 net/l2tp/l2tp_core.h |    2 -
 net/l2tp/l2tp_ip.c   |   22 +++++-------
 3 files changed, 50 insertions(+), 45 deletions(-)
net/l2tp/l2tp_core.c +40 -31
index 34b2dde..e04d3a3 100644
@@ -525,11 +525,10 @@ static inline int l2tp_verify_udp_checksum(struct sock *sk,
  * after the session-id.
  */
 void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
-		      unsigned char *ptr, unsigned char *optr, u16 hdrflags,
+		      int offset, u16 hdrflags,
 		      int length, int (*payload_hook)(struct sk_buff *skb))
 {
 	struct l2tp_tunnel *tunnel = session->tunnel;
-	int offset;
 	u32 ns, nr;
 
 	/* The ref count is increased since we now hold a pointer to

@@ -542,14 +541,17 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 
 	/* Parse and check optional cookie */
 	if (session->peer_cookie_len > 0) {
-		if (memcmp(ptr, &session->peer_cookie[0], session->peer_cookie_len)) {
+		if (!pskb_may_pull(skb, offset + session->peer_cookie_len) ||
+		    memcmp(skb->data + offset,
+			   &session->peer_cookie[0],
+			   session->peer_cookie_len)) {
 			PRINTK(tunnel->debug, L2TP_MSG_DATA, KERN_INFO,
 			       "%s: cookie mismatch (%u/%u). Discarding.\n",
 			       tunnel->name, tunnel->tunnel_id, session->session_id);
 			session->stats.rx_cookie_discards++;
 			goto discard;
 		}
-		ptr += session->peer_cookie_len;
+		offset += session->peer_cookie_len;
 	}
 
 	/* Handle the optional sequence numbers. Sequence numbers are

@@ -563,10 +565,12 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	L2TP_SKB_CB(skb)->has_seq = 0;
 	if (tunnel->version == L2TP_HDR_VER_2) {
 		if (hdrflags & L2TP_HDRFLAG_S) {
-			ns = ntohs(*(__be16 *) ptr);
-			ptr += 2;
-			nr = ntohs(*(__be16 *) ptr);
-			ptr += 2;
+			if (!pskb_may_pull(skb, offset + 4))
+				goto discard;
+			ns = ntohs(*(__be16 *) (skb->data + offset));
+			offset += 2;
+			nr = ntohs(*(__be16 *) (skb->data + offset));
+			offset += 2;
 
 			/* Store L2TP info in the skb */
 			L2TP_SKB_CB(skb)->ns = ns;

@@ -577,8 +581,11 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 			       session->name, ns, nr, session->nr);
 		}
 	} else if (session->l2specific_type == L2TP_L2SPECTYPE_DEFAULT) {
-		u32 l2h = ntohl(*(__be32 *) ptr);
+		u32 l2h;
 
+		if (!pskb_may_pull(skb, offset + 4))
+			goto discard;
+		l2h = ntohl(*(__be32 *) (skb->data + offset));
 		if (l2h & 0x40000000) {
 			ns = l2h & 0x00ffffff;
 

@@ -593,7 +600,7 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	}
 
 	/* Advance past L2-specific header, if present */
-	ptr += session->l2specific_len;
+	offset += session->l2specific_len;
 
 	if (L2TP_SKB_CB(skb)->has_seq) {
 		/* Received a packet with sequence numbers. If we're the LNS,

@@ -647,13 +654,13 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	if (tunnel->version == L2TP_HDR_VER_2) {
 		/* If offset bit set, skip it. */
 		if (hdrflags & L2TP_HDRFLAG_O) {
-			offset = ntohs(*(__be16 *)ptr);
-			ptr += 2 + offset;
+			if (!pskb_may_pull(skb, offset + 2))
+				goto discard;
+			offset += 2 + ntohs(*(__be16 *)(skb->data + offset));
 		}
 	} else
-		ptr += session->offset;
+		offset += session->offset;
 
-	offset = ptr - optr;
 	if (!pskb_may_pull(skb, offset))
 		goto discard;
 

@@ -735,10 +742,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb,
 			      int (*payload_hook)(struct sk_buff *skb))
 {
 	struct l2tp_session *session = NULL;
-	unsigned char *ptr, *optr;
+	unsigned char *ptr;
 	u16 hdrflags;
 	u32 tunnel_id, session_id;
-	int offset;
+	int hlen;
 	u16 version;
 	int length;
 

@@ -756,20 +763,22 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb,
 	}
 
 	/* Point to L2TP header */
-	optr = ptr = skb->data;
+	ptr = skb->data;
 
 	/* Trace packet contents, if enabled */
 	if (tunnel->debug & L2TP_MSG_DATA) {
+		int i;
+
 		length = min(32u, skb->len);
 		if (!pskb_may_pull(skb, length))
 			goto error;
-
+		ptr = skb->data;
 		printk(KERN_DEBUG "%s: recv: ", tunnel->name);
 
-		offset = 0;
+		i = 0;
 		do {
-			printk(" %02X", ptr[offset]);
-		} while (++offset < length);
+			printk(" %02X", ptr[i]);
+		} while (++i < length);
 
 		printk("\n");
 	}

@@ -797,23 +806,23 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb,
 	}
 
 	/* Skip flags */
-	ptr += 2;
+	hlen = 2;
 
 	if (tunnel->version == L2TP_HDR_VER_2) {
 		/* If length is present, skip it */
 		if (hdrflags & L2TP_HDRFLAG_L)
-			ptr += 2;
+			hlen += 2;
 
 		/* Extract tunnel and session ID */
-		tunnel_id = ntohs(*(__be16 *) ptr);
-		ptr += 2;
-		session_id = ntohs(*(__be16 *) ptr);
-		ptr += 2;
+		tunnel_id = ntohs(*(__be16 *) (ptr + hlen));
+		hlen += 2;
+		session_id = ntohs(*(__be16 *) (ptr + hlen));
+		hlen += 2;
 	} else {
-		ptr += 2;	/* skip reserved bits */
+		hlen += 2;	/* skip reserved bits */
 		tunnel_id = tunnel->tunnel_id;
-		session_id = ntohl(*(__be32 *) ptr);
-		ptr += 4;
+		session_id = ntohl(*(__be32 *) (ptr + hlen));
+		hlen += 4;
 	}
 
 	/* Find the session context */

@@ -826,7 +835,7 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb,
 		goto error;
 	}
 
-	l2tp_recv_common(session, skb, ptr, optr, hdrflags, length, payload_hook);
+	l2tp_recv_common(session, skb, hlen, hdrflags, length, payload_hook);
 
 	return 0;
net/l2tp/l2tp_core.h +1 -1
index a16a48e..5341228 100644
@@ -232,7 +232,7 @@ extern int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
 extern struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunnel, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg);
 extern int l2tp_session_delete(struct l2tp_session *session);
 extern void l2tp_session_free(struct l2tp_session *session);
-extern void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb, unsigned char *ptr, unsigned char *optr, u16 hdrflags, int length, int (*payload_hook)(struct sk_buff *skb));
+extern void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb, int offset, u16 hdrflags, int length, int (*payload_hook)(struct sk_buff *skb));
 extern int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb);
 
 extern int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len);
net/l2tp/l2tp_ip.c +9 -13
index d21e7eb..9046865 100644
@@ -123,20 +123,15 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 	struct sock *sk;
 	u32 session_id;
 	u32 tunnel_id;
-	unsigned char *ptr, *optr;
 	struct l2tp_session *session;
 	struct l2tp_tunnel *tunnel = NULL;
-	int length;
-	int offset;
-
-	/* Point to L2TP header */
-	optr = ptr = skb->data;
+	int hlen;
 
 	if (!pskb_may_pull(skb, 4))
 		goto discard;
 
-	session_id = ntohl(*((__be32 *) ptr));
-	ptr += 4;
+	session_id = ntohl(*(__be32 *) skb->data);
+	hlen = 4;
 
 	/* RFC3931: L2TP/IP packets have the first 4 bytes containing
 	 * the session_id. If it is 0, the packet is a L2TP control

@@ -158,21 +153,22 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 
 	/* Trace packet contents, if enabled */
 	if (tunnel->debug & L2TP_MSG_DATA) {
-		length = min(32u, skb->len);
+		int i, length = min(32u, skb->len);
+
 		if (!pskb_may_pull(skb, length))
 			goto discard;
 
 		printk(KERN_DEBUG "%s: ip recv: ", tunnel->name);
 
-		offset = 0;
+		i = 0;
 		do {
-			printk(" %02X", ptr[offset]);
-		} while (++offset < length);
+			printk(" %02X", skb->data[i]);
+		} while (++i < length);
 
 		printk("\n");
 	}
 
-	l2tp_recv_common(session, skb, ptr, optr, 0, skb->len, tunnel->recv_payload_hook);
+	l2tp_recv_common(session, skb, hlen, 0, skb->len, tunnel->recv_payload_hook);
 
 	return 0;
maintainer
#13 lore raw

From: Eric Dumazet <[email protected]>
Date: Wed, 02 Nov 2011 00:58:13 +0100

Quoted 11 lines

Please try following patch, thanks !

[PATCH] l2tp: handle fragmented skbs in receive path

Modern drivers provide skb with fragments, and L2TP doesnt properly handles them.

Some bad frames can also trigger panics because of insufficent checks.

Reported-by Misha Labjuk <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>

I'm still waiting for testing results of this patch.

#14 lore raw
Quoted 10 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Wed, 02 Nov 2011 00:58:13 +0100

Quoted 7 lines

Please try following patch, thanks !

[PATCH] l2tp: handle fragmented skbs in receive path

Modern drivers provide skb with fragments, and L2TP doesnt properly handles them.

Quoted 6 lines
Quoted 4 lines

Some bad frames can also trigger panics because of insufficent checks.

Reported-by Misha Labjuk <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>

I'm still waiting for testing results of this patch.

Of course.

If you prefer, I can submit a smaller patch for the obvious bug first, and I can respin the thing when net-next reopens.

[PATCH] l2tp: fix l2tp_udp_recv_core()

pskb_may_pull() can change skb->data, so we have to load ptr/optr at the right place.

Signed-off-by Eric Dumazet <[email protected]>
---
 net/l2tp/l2tp_core.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
net/l2tp/l2tp_core.c +4 -4
index bf8d50c..cf0f308 100644
@@ -756,9 +756,6 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb,
 		goto error;
 	}
 
-	/* Point to L2TP header */
-	optr = ptr = skb->data;
-
 	/* Trace packet contents, if enabled */
 	if (tunnel->debug & L2TP_MSG_DATA) {
 		length = min(32u, skb->len);

@@ -769,12 +766,15 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb,
 
 		offset = 0;
 		do {
-			printk(" %02X", ptr[offset]);
+			printk(" %02X", skb->data[offset]);
 		} while (++offset < length);
 
 		printk("\n");
 	}
 
+	/* Point to L2TP header */
+	optr = ptr = skb->data;
+
 	/* Get L2TP header flags */
 	hdrflags = ntohs(*(__be16 *) ptr);
maintainer Applied
#15 lore raw

From: Eric Dumazet <[email protected]>
Date: Sat, 05 Nov 2011 08:40:29 +0100

Quoted 23 lines · David Miller
Quoted 10 lines

From: Eric Dumazet <[email protected]>
Date: Wed, 02 Nov 2011 00:58:13 +0100

Quoted 7 lines

Please try following patch, thanks !

[PATCH] l2tp: handle fragmented skbs in receive path

Modern drivers provide skb with fragments, and L2TP doesnt properly handles them.

Quoted 6 lines
Quoted 4 lines

Some bad frames can also trigger panics because of insufficent checks.

Reported-by Misha Labjuk <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>

I'm still waiting for testing results of this patch.

Of course.

If you prefer, I can submit a smaller patch for the obvious bug first, and I can respin the thing when net-next reopens.

Quoted 6 lines

[PATCH] l2tp: fix l2tp_udp_recv_core()

pskb_may_pull() can change skb->data, so we have to load ptr/optr at the right place.

Signed-off-by Eric Dumazet <[email protected]>

Yes, this is easier to digest right now.

Applied.