Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751959AbaANSxe (ORCPT ); Tue, 14 Jan 2014 13:53:34 -0500 Received: from Chamillionaire.breakpoint.cc ([80.244.247.6]:54075 "EHLO Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751472AbaANSxc (ORCPT ); Tue, 14 Jan 2014 13:53:32 -0500 Date: Tue, 14 Jan 2014 19:53:29 +0100 From: Florian Westphal To: Andrey Vagin Cc: netfilter-devel@vger.kernel.org, Eric Dumazet , netfilter@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, vvs@openvz.org, Florian Westphal , Cyrill Gorcunov , Vasiliy Averin Subject: Re: [PATCH] [RFC] netfilter: nf_conntrack: don't relase a conntrack with non-zero refcnt Message-ID: <20140114185329.GB28205@breakpoint.cc> References: <1389710201.31367.255.camel@edumazet-glaptop2.roam.corp.google.com> <1389720948-7883-1-git-send-email-avagin@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1389720948-7883-1-git-send-email-avagin@openvz.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Andrey Vagin wrote: > ---- > Eric and Florian, could you look at this patch. When you say, > that it looks good, I will ask the user to validate it. > I can't reorder these actions, because it's reproduced on a real host > with real users. Thanks. > ---- > > nf_conntrack_free can't be called for a conntract with non-zero ref-counter, > because it can race with nf_conntrack_find_get(). Indeed. > A conntrack slab is created with SLAB_DESTROY_BY_RCU. Non-zero > ref-conunter says that this conntrack is used now. So when we release a > conntrack with non-zero counter, we break this assumption. > > CPU1 CPU2 > ____nf_conntrack_find() > nf_ct_put() > destroy_conntrack() > ... > init_conntrack > __nf_conntrack_alloc (set use = 1) > atomic_inc_not_zero(&ct->use) (use = 2) > if (!l4proto->new(ct, skb, dataoff, timeouts)) > nf_conntrack_free(ct); (use = 2 !!!) > ... Yes, I think this sequence is possible; we must not use nf_conntrack_free here. > - /* We overload first tuple to link into unconfirmed or dying list.*/ > - BUG_ON(hlist_nulls_unhashed(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode)); > - hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode); > + if (!hlist_nulls_unhashed(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode)) > + hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode); This is the only thing that I don't like about this patch. Currently all the conntracks in the system are always put on a list before they're supposed to be visible/handled via refcnt system (unconfirmed, hash, or dying list). I think it would be nice if we could keep it that way. If everything fails we could proably intoduce a 'larval' dummy list similar to the one used by template conntracks? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/