* BUG: held lock freed! (udp_queue_rcv_skb)
@ 2013-12-11 0:39 Dave Jones
2013-12-11 0:54 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Dave Jones @ 2013-12-11 0:39 UTC (permalink / raw)
To: netdev
My router just hit this on Linus' current tree.
Dave
[ 5059.434216] =========================
[ 5059.434314] [ BUG: held lock freed! ]
[ 5059.434420] 3.13.0-rc3+ #9 Not tainted
[ 5059.434520] -------------------------
[ 5059.434620] named/863 is freeing memory ffff88005e960000-ffff88005e96061f, with a lock still held there!
[ 5059.434815] (slock-AF_INET){+.-...}, at: [<ffffffff8149bd21>] udp_queue_rcv_skb+0xd1/0x4b0
[ 5059.435012] 3 locks held by named/863:
[ 5059.435086] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8143054d>] __netif_receive_skb_core+0x11d/0x940
[ 5059.435295] #1: (rcu_read_lock){.+.+..}, at: [<ffffffff81467a5e>] ip_local_deliver_finish+0x3e/0x410
[ 5059.435500] #2: (slock-AF_INET){+.-...}, at: [<ffffffff8149bd21>] udp_queue_rcv_skb+0xd1/0x4b0
[ 5059.435734]
stack backtrace:
[ 5059.435858] CPU: 0 PID: 863 Comm: named Not tainted 3.13.0-rc3+ #9 [loadavg: 0.21 0.06 0.06 1/115 1365]
[ 5059.436052] Hardware name: /D510MO, BIOS MOPNV10J.86A.0175.2010.0308.0620 03/08/2010
[ 5059.436223] 0000000000000002 ffff88007e203ad8 ffffffff8153a372 ffff8800677130e0
[ 5059.436390] ffff88007e203b10 ffffffff8108cafa ffff88005e960000 ffff88007b00cfc0
[ 5059.436554] ffffea00017a5800 ffffffff8141c490 0000000000000246 ffff88007e203b48
[ 5059.436718] Call Trace:
[ 5059.436769] <IRQ> [<ffffffff8153a372>] dump_stack+0x4d/0x66
[ 5059.436904] [<ffffffff8108cafa>] debug_check_no_locks_freed+0x15a/0x160
[ 5059.437037] [<ffffffff8141c490>] ? __sk_free+0x110/0x230
[ 5059.437147] [<ffffffff8112da2a>] kmem_cache_free+0x6a/0x150
[ 5059.437260] [<ffffffff8141c490>] __sk_free+0x110/0x230
[ 5059.437364] [<ffffffff8141c5c9>] sk_free+0x19/0x20
[ 5059.437463] [<ffffffff8141cb25>] sock_edemux+0x25/0x40
[ 5059.437567] [<ffffffff8141c181>] sock_queue_rcv_skb+0x81/0x280
[ 5059.437685] [<ffffffff8149bd21>] ? udp_queue_rcv_skb+0xd1/0x4b0
[ 5059.437805] [<ffffffff81499c82>] __udp_queue_rcv_skb+0x42/0x240
[ 5059.437925] [<ffffffff81541d25>] ? _raw_spin_lock+0x65/0x70
[ 5059.438038] [<ffffffff8149bebb>] udp_queue_rcv_skb+0x26b/0x4b0
[ 5059.438155] [<ffffffff8149c712>] __udp4_lib_rcv+0x152/0xb00
[ 5059.438269] [<ffffffff8149d7f5>] udp_rcv+0x15/0x20
[ 5059.438367] [<ffffffff81467b2f>] ip_local_deliver_finish+0x10f/0x410
[ 5059.438492] [<ffffffff81467a5e>] ? ip_local_deliver_finish+0x3e/0x410
[ 5059.438621] [<ffffffff81468653>] ip_local_deliver+0x43/0x80
[ 5059.438733] [<ffffffff81467f70>] ip_rcv_finish+0x140/0x5a0
[ 5059.438843] [<ffffffff81468926>] ip_rcv+0x296/0x3f0
[ 5059.438945] [<ffffffff81430b72>] __netif_receive_skb_core+0x742/0x940
[ 5059.439074] [<ffffffff8143054d>] ? __netif_receive_skb_core+0x11d/0x940
[ 5059.442231] [<ffffffff8108c81d>] ? trace_hardirqs_on+0xd/0x10
[ 5059.442231] [<ffffffff81430d83>] __netif_receive_skb+0x13/0x60
[ 5059.442231] [<ffffffff81431c1e>] netif_receive_skb+0x1e/0x1f0
[ 5059.442231] [<ffffffff814334e0>] napi_gro_receive+0x70/0xa0
[ 5059.442231] [<ffffffffa01de426>] rtl8169_poll+0x166/0x700 [r8169]
[ 5059.442231] [<ffffffff81432bc9>] net_rx_action+0x129/0x1e0
[ 5059.442231] [<ffffffff810478cd>] __do_softirq+0xed/0x240
[ 5059.442231] [<ffffffff81047e25>] irq_exit+0x125/0x140
[ 5059.442231] [<ffffffff81004241>] do_IRQ+0x51/0xc0
[ 5059.442231] [<ffffffff81542bef>] common_interrupt+0x6f/0x6f
[ 5059.442231] <EOI> [<ffffffff814a7280>] ? inet_autobind+0x60/0x60
[ 5059.442231] [<ffffffff8141722a>] ? SYSC_connect+0xea/0x130
[ 5059.442231] [<ffffffff810cf484>] ? __audit_syscall_entry+0x94/0xf0
[ 5059.442231] [<ffffffff810cf484>] ? __audit_syscall_entry+0x94/0xf0
[ 5059.442231] [<ffffffff8100d4be>] ? syscall_trace_enter+0x16e/0x190
[ 5059.442231] [<ffffffff81418549>] SyS_connect+0x9/0x10
[ 5059.442231] [<ffffffff81549c34>] tracesys+0xdd/0xe2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: BUG: held lock freed! (udp_queue_rcv_skb)
2013-12-11 0:39 BUG: held lock freed! (udp_queue_rcv_skb) Dave Jones
@ 2013-12-11 0:54 ` Eric Dumazet
2013-12-11 2:07 ` [PATCH] udp: ipv4: fix an use after free in __udp4_lib_rcv() Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2013-12-11 0:54 UTC (permalink / raw)
To: Dave Jones; +Cc: netdev
On Tue, 2013-12-10 at 19:39 -0500, Dave Jones wrote:
> My router just hit this on Linus' current tree.
>
> Dave
>
> [ 5059.434216] =========================
> [ 5059.434314] [ BUG: held lock freed! ]
> [ 5059.434420] 3.13.0-rc3+ #9 Not tainted
> [ 5059.434520] -------------------------
> [ 5059.434620] named/863 is freeing memory ffff88005e960000-ffff88005e96061f, with a lock still held there!
> [ 5059.434815] (slock-AF_INET){+.-...}, at: [<ffffffff8149bd21>] udp_queue_rcv_skb+0xd1/0x4b0
> [ 5059.435012] 3 locks held by named/863:
> [ 5059.435086] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8143054d>] __netif_receive_skb_core+0x11d/0x940
> [ 5059.435295] #1: (rcu_read_lock){.+.+..}, at: [<ffffffff81467a5e>] ip_local_deliver_finish+0x3e/0x410
> [ 5059.435500] #2: (slock-AF_INET){+.-...}, at: [<ffffffff8149bd21>] udp_queue_rcv_skb+0xd1/0x4b0
> [ 5059.435734]
> stack backtrace:
> [ 5059.435858] CPU: 0 PID: 863 Comm: named Not tainted 3.13.0-rc3+ #9 [loadavg: 0.21 0.06 0.06 1/115 1365]
> [ 5059.436052] Hardware name: /D510MO, BIOS MOPNV10J.86A.0175.2010.0308.0620 03/08/2010
> [ 5059.436223] 0000000000000002 ffff88007e203ad8 ffffffff8153a372 ffff8800677130e0
> [ 5059.436390] ffff88007e203b10 ffffffff8108cafa ffff88005e960000 ffff88007b00cfc0
> [ 5059.436554] ffffea00017a5800 ffffffff8141c490 0000000000000246 ffff88007e203b48
> [ 5059.436718] Call Trace:
> [ 5059.436769] <IRQ> [<ffffffff8153a372>] dump_stack+0x4d/0x66
> [ 5059.436904] [<ffffffff8108cafa>] debug_check_no_locks_freed+0x15a/0x160
> [ 5059.437037] [<ffffffff8141c490>] ? __sk_free+0x110/0x230
> [ 5059.437147] [<ffffffff8112da2a>] kmem_cache_free+0x6a/0x150
> [ 5059.437260] [<ffffffff8141c490>] __sk_free+0x110/0x230
> [ 5059.437364] [<ffffffff8141c5c9>] sk_free+0x19/0x20
> [ 5059.437463] [<ffffffff8141cb25>] sock_edemux+0x25/0x40
> [ 5059.437567] [<ffffffff8141c181>] sock_queue_rcv_skb+0x81/0x280
> [ 5059.437685] [<ffffffff8149bd21>] ? udp_queue_rcv_skb+0xd1/0x4b0
> [ 5059.437805] [<ffffffff81499c82>] __udp_queue_rcv_skb+0x42/0x240
> [ 5059.437925] [<ffffffff81541d25>] ? _raw_spin_lock+0x65/0x70
> [ 5059.438038] [<ffffffff8149bebb>] udp_queue_rcv_skb+0x26b/0x4b0
> [ 5059.438155] [<ffffffff8149c712>] __udp4_lib_rcv+0x152/0xb00
> [ 5059.438269] [<ffffffff8149d7f5>] udp_rcv+0x15/0x20
> [ 5059.438367] [<ffffffff81467b2f>] ip_local_deliver_finish+0x10f/0x410
> [ 5059.438492] [<ffffffff81467a5e>] ? ip_local_deliver_finish+0x3e/0x410
> [ 5059.438621] [<ffffffff81468653>] ip_local_deliver+0x43/0x80
> [ 5059.438733] [<ffffffff81467f70>] ip_rcv_finish+0x140/0x5a0
> [ 5059.438843] [<ffffffff81468926>] ip_rcv+0x296/0x3f0
> [ 5059.438945] [<ffffffff81430b72>] __netif_receive_skb_core+0x742/0x940
> [ 5059.439074] [<ffffffff8143054d>] ? __netif_receive_skb_core+0x11d/0x940
> [ 5059.442231] [<ffffffff8108c81d>] ? trace_hardirqs_on+0xd/0x10
> [ 5059.442231] [<ffffffff81430d83>] __netif_receive_skb+0x13/0x60
> [ 5059.442231] [<ffffffff81431c1e>] netif_receive_skb+0x1e/0x1f0
> [ 5059.442231] [<ffffffff814334e0>] napi_gro_receive+0x70/0xa0
> [ 5059.442231] [<ffffffffa01de426>] rtl8169_poll+0x166/0x700 [r8169]
> [ 5059.442231] [<ffffffff81432bc9>] net_rx_action+0x129/0x1e0
> [ 5059.442231] [<ffffffff810478cd>] __do_softirq+0xed/0x240
> [ 5059.442231] [<ffffffff81047e25>] irq_exit+0x125/0x140
> [ 5059.442231] [<ffffffff81004241>] do_IRQ+0x51/0xc0
> [ 5059.442231] [<ffffffff81542bef>] common_interrupt+0x6f/0x6f
> [ 5059.442231] <EOI> [<ffffffff814a7280>] ? inet_autobind+0x60/0x60
> [ 5059.442231] [<ffffffff8141722a>] ? SYSC_connect+0xea/0x130
> [ 5059.442231] [<ffffffff810cf484>] ? __audit_syscall_entry+0x94/0xf0
> [ 5059.442231] [<ffffffff810cf484>] ? __audit_syscall_entry+0x94/0xf0
> [ 5059.442231] [<ffffffff8100d4be>] ? syscall_trace_enter+0x16e/0x190
> [ 5059.442231] [<ffffffff81418549>] SyS_connect+0x9/0x10
> [ 5059.442231] [<ffffffff81549c34>] tracesys+0xdd/0xe2
>
This is a nice one. I am cooking a fix, thanks !
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] udp: ipv4: fix an use after free in __udp4_lib_rcv()
2013-12-11 0:54 ` Eric Dumazet
@ 2013-12-11 2:07 ` Eric Dumazet
2013-12-11 3:59 ` David Miller
2013-12-11 16:10 ` [PATCH] udp: ipv4: fix potential use after free in udp_v4_early_demux() Eric Dumazet
0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2013-12-11 2:07 UTC (permalink / raw)
To: Dave Jones, Shawn Bohrer, David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
Dave Jones reported a use after free in UDP stack :
[ 5059.434216] =========================
[ 5059.434314] [ BUG: held lock freed! ]
[ 5059.434420] 3.13.0-rc3+ #9 Not tainted
[ 5059.434520] -------------------------
[ 5059.434620] named/863 is freeing memory ffff88005e960000-ffff88005e96061f, with a lock still held there!
[ 5059.434815] (slock-AF_INET){+.-...}, at: [<ffffffff8149bd21>] udp_queue_rcv_skb+0xd1/0x4b0
[ 5059.435012] 3 locks held by named/863:
[ 5059.435086] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8143054d>] __netif_receive_skb_core+0x11d/0x940
[ 5059.435295] #1: (rcu_read_lock){.+.+..}, at: [<ffffffff81467a5e>] ip_local_deliver_finish+0x3e/0x410
[ 5059.435500] #2: (slock-AF_INET){+.-...}, at: [<ffffffff8149bd21>] udp_queue_rcv_skb+0xd1/0x4b0
[ 5059.435734]
stack backtrace:
[ 5059.435858] CPU: 0 PID: 863 Comm: named Not tainted 3.13.0-rc3+ #9 [loadavg: 0.21 0.06 0.06 1/115 1365]
[ 5059.436052] Hardware name: /D510MO, BIOS MOPNV10J.86A.0175.2010.0308.0620 03/08/2010
[ 5059.436223] 0000000000000002 ffff88007e203ad8 ffffffff8153a372 ffff8800677130e0
[ 5059.436390] ffff88007e203b10 ffffffff8108cafa ffff88005e960000 ffff88007b00cfc0
[ 5059.436554] ffffea00017a5800 ffffffff8141c490 0000000000000246 ffff88007e203b48
[ 5059.436718] Call Trace:
[ 5059.436769] <IRQ> [<ffffffff8153a372>] dump_stack+0x4d/0x66
[ 5059.436904] [<ffffffff8108cafa>] debug_check_no_locks_freed+0x15a/0x160
[ 5059.437037] [<ffffffff8141c490>] ? __sk_free+0x110/0x230
[ 5059.437147] [<ffffffff8112da2a>] kmem_cache_free+0x6a/0x150
[ 5059.437260] [<ffffffff8141c490>] __sk_free+0x110/0x230
[ 5059.437364] [<ffffffff8141c5c9>] sk_free+0x19/0x20
[ 5059.437463] [<ffffffff8141cb25>] sock_edemux+0x25/0x40
[ 5059.437567] [<ffffffff8141c181>] sock_queue_rcv_skb+0x81/0x280
[ 5059.437685] [<ffffffff8149bd21>] ? udp_queue_rcv_skb+0xd1/0x4b0
[ 5059.437805] [<ffffffff81499c82>] __udp_queue_rcv_skb+0x42/0x240
[ 5059.437925] [<ffffffff81541d25>] ? _raw_spin_lock+0x65/0x70
[ 5059.438038] [<ffffffff8149bebb>] udp_queue_rcv_skb+0x26b/0x4b0
[ 5059.438155] [<ffffffff8149c712>] __udp4_lib_rcv+0x152/0xb00
[ 5059.438269] [<ffffffff8149d7f5>] udp_rcv+0x15/0x20
[ 5059.438367] [<ffffffff81467b2f>] ip_local_deliver_finish+0x10f/0x410
[ 5059.438492] [<ffffffff81467a5e>] ? ip_local_deliver_finish+0x3e/0x410
[ 5059.438621] [<ffffffff81468653>] ip_local_deliver+0x43/0x80
[ 5059.438733] [<ffffffff81467f70>] ip_rcv_finish+0x140/0x5a0
[ 5059.438843] [<ffffffff81468926>] ip_rcv+0x296/0x3f0
[ 5059.438945] [<ffffffff81430b72>] __netif_receive_skb_core+0x742/0x940
[ 5059.439074] [<ffffffff8143054d>] ? __netif_receive_skb_core+0x11d/0x940
[ 5059.442231] [<ffffffff8108c81d>] ? trace_hardirqs_on+0xd/0x10
[ 5059.442231] [<ffffffff81430d83>] __netif_receive_skb+0x13/0x60
[ 5059.442231] [<ffffffff81431c1e>] netif_receive_skb+0x1e/0x1f0
[ 5059.442231] [<ffffffff814334e0>] napi_gro_receive+0x70/0xa0
[ 5059.442231] [<ffffffffa01de426>] rtl8169_poll+0x166/0x700 [r8169]
[ 5059.442231] [<ffffffff81432bc9>] net_rx_action+0x129/0x1e0
[ 5059.442231] [<ffffffff810478cd>] __do_softirq+0xed/0x240
[ 5059.442231] [<ffffffff81047e25>] irq_exit+0x125/0x140
[ 5059.442231] [<ffffffff81004241>] do_IRQ+0x51/0xc0
[ 5059.442231] [<ffffffff81542bef>] common_interrupt+0x6f/0x6f
We need to keep a reference on the socket, by using skb_steal_sock()
at the right place.
Note that another patch is needed to fix a race in
udp_sk_rx_dst_set(), as we hold no lock protecting the dst.
Fixes: 421b3885bf6d ("udp: ipv4: Add udp early demux")
Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
net/ipv4/udp.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 44f6a20..2e2aecb 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -560,15 +560,11 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
__be16 sport, __be16 dport,
struct udp_table *udptable)
{
- struct sock *sk;
const struct iphdr *iph = ip_hdr(skb);
- if (unlikely(sk = skb_steal_sock(skb)))
- return sk;
- else
- return __udp4_lib_lookup(dev_net(skb_dst(skb)->dev), iph->saddr, sport,
- iph->daddr, dport, inet_iif(skb),
- udptable);
+ return __udp4_lib_lookup(dev_net(skb_dst(skb)->dev), iph->saddr, sport,
+ iph->daddr, dport, inet_iif(skb),
+ udptable);
}
struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
@@ -1739,15 +1735,15 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
if (udp4_csum_init(skb, uh, proto))
goto csum_error;
- if (skb->sk) {
+ sk = skb_steal_sock(skb);
+ if (sk) {
int ret;
- sk = skb->sk;
if (unlikely(sk->sk_rx_dst == NULL))
udp_sk_rx_dst_set(sk, skb);
ret = udp_queue_rcv_skb(sk, skb);
-
+ sock_put(sk);
/* a return value > 0 means to resubmit the input, but
* it wants the return to be -protocol, or 0
*/
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: ipv4: fix an use after free in __udp4_lib_rcv()
2013-12-11 2:07 ` [PATCH] udp: ipv4: fix an use after free in __udp4_lib_rcv() Eric Dumazet
@ 2013-12-11 3:59 ` David Miller
2013-12-11 16:10 ` [PATCH] udp: ipv4: fix potential use after free in udp_v4_early_demux() Eric Dumazet
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2013-12-11 3:59 UTC (permalink / raw)
To: eric.dumazet; +Cc: davej, sbohrer, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 10 Dec 2013 18:07:23 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> Dave Jones reported a use after free in UDP stack :
...
> We need to keep a reference on the socket, by using skb_steal_sock()
> at the right place.
>
> Note that another patch is needed to fix a race in
> udp_sk_rx_dst_set(), as we hold no lock protecting the dst.
>
> Fixes: 421b3885bf6d ("udp: ipv4: Add udp early demux")
> Reported-by: Dave Jones <davej@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Shawn Bohrer <sbohrer@rgmadvisors.com>
Applied, thanks Eric.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] udp: ipv4: fix potential use after free in udp_v4_early_demux()
2013-12-11 2:07 ` [PATCH] udp: ipv4: fix an use after free in __udp4_lib_rcv() Eric Dumazet
2013-12-11 3:59 ` David Miller
@ 2013-12-11 16:10 ` Eric Dumazet
2013-12-11 21:16 ` David Miller
2013-12-11 22:46 ` [PATCH] udp: ipv4: must add synchronization in udp_sk_rx_dst_set() Eric Dumazet
1 sibling, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2013-12-11 16:10 UTC (permalink / raw)
To: David Miller; +Cc: Shawn Bohrer, netdev
From: Eric Dumazet <edumazet@google.com>
pskb_may_pull() can reallocate skb->head, we need to move the
initialization of iph and uh pointers after its call.
Fixes: 421b3885bf6d ("udp: ipv4: Add udp early demux")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
net/ipv4/udp.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 2e2aecbe22c4..16d246a51a02 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1909,17 +1909,20 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
void udp_v4_early_demux(struct sk_buff *skb)
{
- const struct iphdr *iph = ip_hdr(skb);
- const struct udphdr *uh = udp_hdr(skb);
+ struct net *net = dev_net(skb->dev);
+ const struct iphdr *iph;
+ const struct udphdr *uh;
struct sock *sk;
struct dst_entry *dst;
- struct net *net = dev_net(skb->dev);
int dif = skb->dev->ifindex;
/* validate the packet */
if (!pskb_may_pull(skb, skb_transport_offset(skb) + sizeof(struct udphdr)))
return;
+ iph = ip_hdr(skb);
+ uh = udp_hdr(skb);
+
if (skb->pkt_type == PACKET_BROADCAST ||
skb->pkt_type == PACKET_MULTICAST)
sk = __udp4_lib_mcast_demux_lookup(net, uh->dest, iph->daddr,
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: ipv4: fix potential use after free in udp_v4_early_demux()
2013-12-11 16:10 ` [PATCH] udp: ipv4: fix potential use after free in udp_v4_early_demux() Eric Dumazet
@ 2013-12-11 21:16 ` David Miller
2013-12-11 22:46 ` [PATCH] udp: ipv4: must add synchronization in udp_sk_rx_dst_set() Eric Dumazet
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2013-12-11 21:16 UTC (permalink / raw)
To: eric.dumazet; +Cc: sbohrer, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 11 Dec 2013 08:10:05 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> pskb_may_pull() can reallocate skb->head, we need to move the
> initialization of iph and uh pointers after its call.
>
> Fixes: 421b3885bf6d ("udp: ipv4: Add udp early demux")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Shawn Bohrer <sbohrer@rgmadvisors.com>
Applied, thanks Eric.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: ipv4: must add synchronization in udp_sk_rx_dst_set()
2013-12-11 16:10 ` [PATCH] udp: ipv4: fix potential use after free in udp_v4_early_demux() Eric Dumazet
2013-12-11 21:16 ` David Miller
@ 2013-12-11 22:46 ` Eric Dumazet
2013-12-12 1:22 ` David Miller
1 sibling, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2013-12-11 22:46 UTC (permalink / raw)
To: David Miller; +Cc: Shawn Bohrer, netdev
From: Eric Dumazet <edumazet@google.com>
Unlike TCP, UDP input path does not hold the socket lock.
Before messing with sk->sk_rx_dst, we must use a spinlock, otherwise
multiple cpus could leak a refcount.
This patch also takes care of renewing a stale dst entry.
(When the sk->sk_rx_dst would not be used by IP early demux)
Fixes: 421b3885bf6d ("udp: ipv4: Add udp early demux")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
net/ipv4/udp.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 16d246a51a02..62c19fdd102d 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1599,12 +1599,21 @@ static void flush_stack(struct sock **stack, unsigned int count,
kfree_skb(skb1);
}
-static void udp_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
+/* For TCP sockets, sk_rx_dst is protected by socket lock
+ * For UDP, we use sk_dst_lock to guard against concurrent changes.
+ */
+static void udp_sk_rx_dst_set(struct sock *sk, struct dst_entry *dst)
{
- struct dst_entry *dst = skb_dst(skb);
+ struct dst_entry *old;
- dst_hold(dst);
- sk->sk_rx_dst = dst;
+ spin_lock(&sk->sk_dst_lock);
+ old = sk->sk_rx_dst;
+ if (likely(old != dst)) {
+ dst_hold(dst);
+ sk->sk_rx_dst = dst;
+ dst_release(old);
+ }
+ spin_unlock(&sk->sk_dst_lock);
}
/*
@@ -1737,10 +1746,11 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
sk = skb_steal_sock(skb);
if (sk) {
+ struct dst_entry *dst = skb_dst(skb);
int ret;
- if (unlikely(sk->sk_rx_dst == NULL))
- udp_sk_rx_dst_set(sk, skb);
+ if (unlikely(sk->sk_rx_dst != dst))
+ udp_sk_rx_dst_set(sk, dst);
ret = udp_queue_rcv_skb(sk, skb);
sock_put(sk);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: ipv4: must add synchronization in udp_sk_rx_dst_set()
2013-12-11 22:46 ` [PATCH] udp: ipv4: must add synchronization in udp_sk_rx_dst_set() Eric Dumazet
@ 2013-12-12 1:22 ` David Miller
2013-12-15 18:30 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2013-12-12 1:22 UTC (permalink / raw)
To: eric.dumazet; +Cc: sbohrer, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 11 Dec 2013 14:46:51 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> Unlike TCP, UDP input path does not hold the socket lock.
>
> Before messing with sk->sk_rx_dst, we must use a spinlock, otherwise
> multiple cpus could leak a refcount.
>
> This patch also takes care of renewing a stale dst entry.
> (When the sk->sk_rx_dst would not be used by IP early demux)
>
> Fixes: 421b3885bf6d ("udp: ipv4: Add udp early demux")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied.
Longterm, perhaps a candidate for using xchg() instead of this
spinlock?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: ipv4: must add synchronization in udp_sk_rx_dst_set()
2013-12-12 1:22 ` David Miller
@ 2013-12-15 18:30 ` Eric Dumazet
0 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2013-12-15 18:30 UTC (permalink / raw)
To: David Miller; +Cc: sbohrer, netdev
On Wed, 2013-12-11 at 20:22 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 11 Dec 2013 14:46:51 -0800
>
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Unlike TCP, UDP input path does not hold the socket lock.
> >
> > Before messing with sk->sk_rx_dst, we must use a spinlock, otherwise
> > multiple cpus could leak a refcount.
> >
> > This patch also takes care of renewing a stale dst entry.
> > (When the sk->sk_rx_dst would not be used by IP early demux)
> >
> > Fixes: 421b3885bf6d ("udp: ipv4: Add udp early demux")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Applied.
>
> Longterm, perhaps a candidate for using xchg() instead of this
> spinlock?
Indeed... It appears sk_dst_lock should not be used from BH context
anyway...
I am sending a fix, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-12-15 18:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-11 0:39 BUG: held lock freed! (udp_queue_rcv_skb) Dave Jones
2013-12-11 0:54 ` Eric Dumazet
2013-12-11 2:07 ` [PATCH] udp: ipv4: fix an use after free in __udp4_lib_rcv() Eric Dumazet
2013-12-11 3:59 ` David Miller
2013-12-11 16:10 ` [PATCH] udp: ipv4: fix potential use after free in udp_v4_early_demux() Eric Dumazet
2013-12-11 21:16 ` David Miller
2013-12-11 22:46 ` [PATCH] udp: ipv4: must add synchronization in udp_sk_rx_dst_set() Eric Dumazet
2013-12-12 1:22 ` David Miller
2013-12-15 18:30 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).