From patchwork Fri Aug 11 13:28:21 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kleber Sacilotto de Souza X-Patchwork-Id: 800557 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=canonical-com.20150623.gappssmtp.com header.i=@canonical-com.20150623.gappssmtp.com header.b="qcLY5Lim"; dkim-atps=neutral Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 3xTQnM6HN4z9t2Z; Fri, 11 Aug 2017 23:28:39 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1dg9zR-0006hG-7n; Fri, 11 Aug 2017 13:28:37 +0000 Received: from mail-wm0-f41.google.com ([74.125.82.41]) by huckleberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1dg9zG-0006dI-GV for kernel-team@lists.ubuntu.com; Fri, 11 Aug 2017 13:28:26 +0000 Received: by mail-wm0-f41.google.com with SMTP id i66so47485339wmg.0 for ; Fri, 11 Aug 2017 06:28:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical-com.20150623.gappssmtp.com; s=20150623; h=from:to:subject:date:message-id:in-reply-to:references; bh=mrB6j3bReEVv0lEdlZwAqLd9E0lUyTV7SaTglCfM42g=; b=qcLY5LimfzG0q+QpFcHrkcPYBbOzPVZmn7k6W9Cv0LiEH+qZTG0gbw4808aHf/uTRC X0Mcj3GWvsbGH4YWbbDTTWl1jue9RdQXBpacTLbmrhx9VCuS0cnNAKSgeCl0UG5DrxrE BITz1vzwwHrswj/anrJVoD3XPFBOEy+8hkljf580lzf7gfOgp4Gs/rrXb9cfHFaeRzOJ fyO+ddaLV/KLnL7pMgFM1Z7H/vZT4o8rB84gR+TwlOLUy6+NmA+slcHHibSU78B+BrMH zr4OwKFPYOsc8PTMF5QL6C0RxYqUd4+buMssfnTcIiF9571OyWOm+62dQ3WcWC4BVQFO 12Ag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=mrB6j3bReEVv0lEdlZwAqLd9E0lUyTV7SaTglCfM42g=; b=bV9WenZzbbHKM8Ao/fQDQdXF3sFakBzd0w3zEAkaBmIDtKOG5MX1440DT7HMU6p4EO NIiMMrNFqlBQYSI07+u0d7ROUxAFQ8Q/MKQjcQBdn5fxZ52dB9jppXN6PsPfS69Z5EVR ZZTldFGBYHtDFCsSyqRPmkpH4WuhEfppdzK7gyqbWTywim7WDZyw0r9WeOZzJ+qjV4vl p0G9lL7XzPtdn0VJF1zeKI8prKXVVpycKzaYO+6zEv/eai0Ghgz4uLubCllcQnwNqWqr 5K6hwhQC71rPJ3ZMTw5aint4dm/++bhD/8eb9gf8WpdJ2Hw0C+/FGKdqXRhRfJ6rfp9p n6Tw== X-Gm-Message-State: AHYfb5gn4Xh/9rtsCZDGCfRQ35GsHKlBa9xC80JcS6LB33RPGDVCP8gW zvBs1WblxjU1VHo4B2jcPw== X-Received: by 10.80.208.217 with SMTP id g25mr2905378edf.229.1502458105836; Fri, 11 Aug 2017 06:28:25 -0700 (PDT) Received: from localhost ([2a02:8109:a540:7e8:b97b:6fe3:d935:b537]) by smtp.gmail.com with ESMTPSA id w31sm686351edb.51.2017.08.11.06.28.25 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 11 Aug 2017 06:28:25 -0700 (PDT) From: Kleber Sacilotto de Souza To: kernel-team@lists.ubuntu.com Subject: [Xenial][PATCH 2/2] netfilter: nf_ct_ext: fix possible panic after nf_ct_extend_unregister Date: Fri, 11 Aug 2017 15:28:21 +0200 Message-Id: <20170811132821.21306-3-kleber.souza@canonical.com> X-Mailer: git-send-email 2.14.0 In-Reply-To: <20170811132821.21306-1-kleber.souza@canonical.com> References: <20170811132821.21306-1-kleber.souza@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: kernel-team-bounces@lists.ubuntu.com From: Liping Zhang BugLink: http://bugs.launchpad.net/bugs/1709032 If one cpu is doing nf_ct_extend_unregister while another cpu is doing __nf_ct_ext_add_length, then we may hit BUG_ON(t == NULL). Moreover, there's no synchronize_rcu invocation after set nf_ct_ext_types[id] to NULL, so it's possible that we may access invalid pointer. But actually, most of the ct extends are built-in, so the problem listed above will not happen. However, there are two exceptions: NF_CT_EXT_NAT and NF_CT_EXT_SYNPROXY. For _EXT_NAT, the panic will not happen, since adding the nat extend and unregistering the nat extend are located in the same file(nf_nat_core.c), this means that after the nat module is removed, we cannot add the nat extend too. For _EXT_SYNPROXY, synproxy extend may be added by init_conntrack, while synproxy extend unregister will be done by synproxy_core_exit. So after nf_synproxy_core.ko is removed, we may still try to add the synproxy extend, then kernel panic may happen. I know it's very hard to reproduce this issue, but I can play a tricky game to make it happen very easily :) Step 1. Enable SYNPROXY for tcp dport 1234 at FORWARD hook: # iptables -I FORWARD -p tcp --dport 1234 -j SYNPROXY Step 2. Queue the syn packet to the userspace at raw table OUTPUT hook. Also note, in the userspace we only add a 20s' delay, then reinject the syn packet to the kernel: # iptables -t raw -I OUTPUT -p tcp --syn -j NFQUEUE --queue-num 1 Step 3. Using "nc 2.2.2.2 1234" to connect the server. Step 4. Now remove the nf_synproxy_core.ko quickly: # iptables -F FORWARD # rmmod ipt_SYNPROXY # rmmod nf_synproxy_core Step 5. After 20s' delay, the syn packet is reinjected to the kernel. Now you will see the panic like this: kernel BUG at net/netfilter/nf_conntrack_extend.c:91! Call Trace: ? __nf_ct_ext_add_length+0x53/0x3c0 [nf_conntrack] init_conntrack+0x12b/0x600 [nf_conntrack] nf_conntrack_in+0x4cc/0x580 [nf_conntrack] ipv4_conntrack_local+0x48/0x50 [nf_conntrack_ipv4] nf_reinject+0x104/0x270 nfqnl_recv_verdict+0x3e1/0x5f9 [nfnetlink_queue] ? nfqnl_recv_verdict+0x5/0x5f9 [nfnetlink_queue] ? nla_parse+0xa0/0x100 nfnetlink_rcv_msg+0x175/0x6a9 [nfnetlink] [...] One possible solution is to make NF_CT_EXT_SYNPROXY extend built-in, i.e. introduce nf_conntrack_synproxy.c and only do ct extend register and unregister in it, similar to nf_conntrack_timeout.c. But having such a obscure restriction of nf_ct_extend_unregister is not a good idea, so we should invoke synchronize_rcu after set nf_ct_ext_types to NULL, and check the NULL pointer when do __nf_ct_ext_add_length. Then it will be easier if we add new ct extend in the future. Last, we use kfree_rcu to free nf_ct_ext, so rcu_barrier() is unnecessary anymore, remove it too. Signed-off-by: Liping Zhang Acked-by: Florian Westphal Signed-off-by: Pablo Neira Ayuso (cherry picked from commit 9c3f3794926a997b1cab6c42480ff300efa2d162) Signed-off-by: Kleber Sacilotto de Souza --- net/netfilter/nf_conntrack_extend.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c index 1a9545965c0d..531ca55f1af6 100644 --- a/net/netfilter/nf_conntrack_extend.c +++ b/net/netfilter/nf_conntrack_extend.c @@ -53,7 +53,11 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, rcu_read_lock(); t = rcu_dereference(nf_ct_ext_types[id]); - BUG_ON(t == NULL); + if (!t) { + rcu_read_unlock(); + return NULL; + } + off = ALIGN(sizeof(struct nf_ct_ext), t->align); len = off + t->len + var_alloc_len; alloc_size = t->alloc_size + var_alloc_len; @@ -88,7 +92,10 @@ void *__nf_ct_ext_add_length(struct nf_conn *ct, enum nf_ct_ext_id id, rcu_read_lock(); t = rcu_dereference(nf_ct_ext_types[id]); - BUG_ON(t == NULL); + if (!t) { + rcu_read_unlock(); + return NULL; + } newoff = ALIGN(old->len, t->align); newlen = newoff + t->len + var_alloc_len; @@ -186,6 +193,6 @@ void nf_ct_extend_unregister(struct nf_ct_ext_type *type) RCU_INIT_POINTER(nf_ct_ext_types[type->id], NULL); update_alloc_size(type); mutex_unlock(&nf_ct_ext_type_mutex); - rcu_barrier(); /* Wait for completion of call_rcu()'s */ + synchronize_rcu(); } EXPORT_SYMBOL_GPL(nf_ct_extend_unregister);