From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Chapman Subject: [RESEND: PATCH] l2tp: Fix UDP socket reference count bugs in the pppol2tp driver Date: Tue, 16 Mar 2010 16:29:20 +0000 Message-ID: <20100316162920.20459.11487.stgit@bert.katalix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: netdev@vger.kernel.org Return-path: Received: from katalix.com ([82.103.140.233]:57912 "EHLO mail.katalix.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933080Ab0CPQVg (ORCPT ); Tue, 16 Mar 2010 12:21:36 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by mail.katalix.com (Postfix) with ESMTP id 4C71DA62098 for ; Tue, 16 Mar 2010 16:29:20 +0000 (GMT) Received: from mail.katalix.com ([127.0.0.1]) by localhost (mail.katalix.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fxmLQvutdERq for ; Tue, 16 Mar 2010 16:29:20 +0000 (GMT) Received: from bert.katalix.com (localhost.localdomain [127.0.0.1]) by mail.katalix.com (Postfix) with ESMTP id 2BBADA62087 for ; Tue, 16 Mar 2010 16:29:20 +0000 (GMT) Sender: netdev-owner@vger.kernel.org List-ID: This patch fixes UDP socket refcnt bugs in the pppol2tp driver. A bug can cause a kernel stack trace when a tunnel socket is closed. 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. 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: [] ? udp_lib_unhash+0x117/0x120 [] ? warn_slowpath_common+0x71/0xd0 [] ? udp_lib_unhash+0x117/0x120 [] ? warn_slowpath_null+0x13/0x20 [] ? udp_lib_unhash+0x117/0x120 [] ? sk_common_release+0x17/0x90 [] ? inet_release+0x33/0x60 [] ? sock_release+0x10/0x60 [] ? sock_close+0xf/0x30 [] ? __fput+0x52/0x150 [] ? filp_close+0x3e/0x70 [] ? put_files_struct+0x62/0xb0 [] ? do_exit+0x5e7/0x650 [] ? mntput_no_expire+0x13/0x70 [] ? filp_close+0x3e/0x70 [] ? do_group_exit+0x2a/0x70 [] ? sys_exit_group+0x11/0x20 [] ? sysenter_do_call+0x12/0x26 Signed-off-by: James Chapman --- This patch was originally submitted 21-jan-2010, http://marc.info/?l=linux-netdev&m=126409025900561&w=2. Its description has since been updated and it has been refreshed against latest netdev git. This patch may be a candidate for -stable. --- drivers/net/pppol2tp.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c index 5861ee9..449a982 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; @@ -1662,6 +1664,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);