* 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).