netdev - Re: [PATCH] [RFC] netfilter: nf_conntrack: don't relase a conntrack with non-zero refcnt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 3 Feb 2014 00:30:46 +0100
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Florian Westphal <fw@...len.de>
Cc:	Andrew Vagin <avagin@...allels.com>,
	Andrey Vagin <avagin@...nvz.org>,
	netfilter-devel@...r.kernel.org,
	Eric Dumazet <eric.dumazet@...il.com>,
	netfilter@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, vvs@...nvz.org,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	Vasiliy Averin <vvs@...allels.com>
Subject: Re: [PATCH] [RFC] netfilter: nf_conntrack: don't relase a conntrack
 with non-zero refcnt

On Thu, Jan 16, 2014 at 10:23:01AM +0100, Florian Westphal wrote:
> Andrew Vagin <avagin@...allels.com> wrote:
> > > 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?
> > 
> > I'm not sure, that this is required. Could you elaborate when this can
> > be useful?
> 
> You can dump the lists via ctnetlink.  Its meant as a debugging aid in
> case one suspects refcnt leaks.
> 
> Granted, in this situation there should be no leak since we put the newly
> allocated entry in the error case.
> 
> > Now I see only overhead, because we need to take the nf_conntrack_lock
> > lock to add conntrack in a list.
> 
> True. I don't have any preference, I guess I'd just do the insertion into the
> unconfirmed list when we know we cannot track to keep the "unhashed"
> bug trap in the destroy function.
> 
> Pablo, any preference?

I think we can initially set to zero the refcount and bump it once it
gets into any of the lists, so Eric's golden rule also stands for
conntracks that are released without being inserted in any list via
nf_conntrack_free().

My idea was to use dying list to detect possible runtime leaks (ie.
missing nf_ct_put somewhere), not simple leaks the initialization
path, as you said, it would add too much overhead to catch them with
the dying list, so we can skip those.

Please, let me know if you find any issue with this approach.

View attachment "x.patch" of type "text/x-diff" (4405 bytes)