| Message ID | 20100121161009.5223.34288.stgit@bert.katalix.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: James Chapman <jchapman@katalix.com> Date: Thu, 21 Jan 2010 16:10:09 +0000 > The bug can cause a kernel stack trace when a tunnel socket is closed. > > WARNING: at include/net/sock.h:435 udp_lib_unhash+0x117/0x120() > Pid: 1086, comm: openl2tpd Not tainted 2.6.33-rc1 #8 > Call Trace: This fix doesn't look right at all. You grab one reference in connect() and then drop a reference every single recvmsg() call. recvmsg() calls to connect() would be many to one, so I can't see how this reference counting scheme could possibly work. Why don't you describe the exact sequence of events that lead to the trace, so we can figure out how to correct this properly? Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: James Chapman <jchapman@katalix.com> > Date: Thu, 21 Jan 2010 16:10:09 +0000 > >> The bug can cause a kernel stack trace when a tunnel socket is closed. >> >> WARNING: at include/net/sock.h:435 udp_lib_unhash+0x117/0x120() >> Pid: 1086, comm: openl2tpd Not tainted 2.6.33-rc1 #8 >> Call Trace: > > This fix doesn't look right at all. > > You grab one reference in connect() and then drop a reference > every single recvmsg() call. No, one ref is grabbed when the UDP socket is prepared for L2TP. Another ref is grabbed while processing a skb in the receive path. > recvmsg() calls to connect() would be many to one, so I can't > see how this reference counting scheme could possibly work. Perhaps you missed the sock_hold() in pppol2tp_sock_to_tunnel(), which is called for every received skb in pppol2tp_recv_core()? When userspace closes all session sockets in the tunnel, including the special tunnel pppol2tp socket which has session_id==0, the ref on the UDP tunnel socket is dropped, which allows it to be released. > Why don't you describe the exact sequence of events that lead > to the trace, so we can figure out how to correct this > properly? A way to reproduce the issue is to prepare the UDP socket for L2TP (by opening a tunnel pppol2tp socket) and then close it before any L2TP sessions are added to it. The sequence is Create UDP socket Create tunnel pppol2tp socket to prepare UDP socket for L2TP pppol2tp_connect: session_id=0, peer_session_id=0 L2TP SCCRP control frame received (tunnel_id==0) pppol2tp_recv_core: sock_hold() pppol2tp_recv_core: sock_put L2TP ZLB control frame received (tunnel_id=nnn) pppol2tp_recv_core: sock_hold() pppol2tp_recv_core: sock_put Close tunnel management socket pppol2tp_release: session_id=0, peer_session_id=0 Close UDP socket udp_lib_close: BUG The addition of sock_hold() in pppol2tp_connect() solves the problem. For data frames, two sock_put() calls were added to plug a refcnt leak per received data frame. The ref that is grabbed at the top of pppol2tp_recv_core() must always be released, but this wasn't done for accepted data frames or data frames discarded because of bad UDP checksums. This leak meant that any UDP socket that had passed L2TP data traffic (i.e. L2TP data frames, not just L2TP control frames) using pppol2tp would not be released by the kernel. Does the above help?
From: James Chapman <jchapman@katalix.com> Date: Wed, 27 Jan 2010 13:14:56 +0000 > Does the above help? Thanks for the detailed explanation, I'll take another look at this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: James Chapman <jchapman@katalix.com> > Date: Wed, 27 Jan 2010 13:14:56 +0000 > >> Does the above help? > > Thanks for the detailed explanation, I'll take another look > at this. Did you get a chance to look at this? fyi - I'm getting ready to submit a patch series that adds L2TPv3 support. I've been sitting on these for too long and have finally found time to prep them for review. Previous version was posted as RFC a year ago, archived here: http://marc.info/?l=linux-netdev&m=123532490429538&w=2 Should I wait for my previous fix to be reviewed before posting the new code? Or I could bundle that patch in the series - whichever works for you. Thanks
From: James Chapman <jchapman@katalix.com> Date: Thu, 11 Feb 2010 11:32:12 +0000 > David Miller wrote: >> From: James Chapman <jchapman@katalix.com> >> Date: Wed, 27 Jan 2010 13:14:56 +0000 >> >>> Does the above help? >> >> Thanks for the detailed explanation, I'll take another look >> at this. > > Did you get a chance to look at this? Not yet, sorry. Too many other things are preempting this at the moment. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c index 9fbb2eb..0b5d96e 100644 --- a/drivers/net/pppol2tp.c +++ b/drivers/net/pppol2tp.c @@ -756,6 +756,7 @@ static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb) /* Try to dequeue as many skbs from reorder_q as we can. */ pppol2tp_recv_dequeue(session); + sock_put(sock); return 0; @@ -772,6 +773,7 @@ discard_bad_csum: UDP_INC_STATS_USER(&init_net, UDP_MIB_INERRORS, 0); tunnel->stats.rx_errors++; kfree_skb(skb); + sock_put(sock); return 0; @@ -1661,6 +1663,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr, if (tunnel_sock == NULL) goto end; + sock_hold(tunnel_sock); tunnel = tunnel_sock->sk_user_data; } else { tunnel = pppol2tp_tunnel_find(sock_net(sk), sp->pppol2tp.s_tunnel);
The bug can cause a kernel stack trace when a tunnel socket is closed. WARNING: at include/net/sock.h:435 udp_lib_unhash+0x117/0x120() Pid: 1086, comm: openl2tpd Not tainted 2.6.33-rc1 #8 Call Trace: [<c119e9b7>] ? udp_lib_unhash+0x117/0x120 [<c101b871>] ? warn_slowpath_common+0x71/0xd0 [<c119e9b7>] ? udp_lib_unhash+0x117/0x120 [<c101b8e3>] ? warn_slowpath_null+0x13/0x20 [<c119e9b7>] ? udp_lib_unhash+0x117/0x120 [<c11598a7>] ? sk_common_release+0x17/0x90 [<c11a5e33>] ? inet_release+0x33/0x60 [<c11577b0>] ? sock_release+0x10/0x60 [<c115780f>] ? sock_close+0xf/0x30 [<c106e542>] ? __fput+0x52/0x150 [<c106b68e>] ? filp_close+0x3e/0x70 [<c101d2e2>] ? put_files_struct+0x62/0xb0 [<c101eaf7>] ? do_exit+0x5e7/0x650 [<c1081623>] ? mntput_no_expire+0x13/0x70 [<c106b68e>] ? filp_close+0x3e/0x70 [<c101eb8a>] ? do_group_exit+0x2a/0x70 [<c101ebe1>] ? sys_exit_group+0x11/0x20 [<c10029b0>] ? sysenter_do_call+0x12/0x26 Signed-off-by: James Chapman <jchapman@katalix.com> --- This patch may be a candidate for -stable. --- drivers/net/pppol2tp.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html