netdev - Re: [PATCH nf-next 3/9] netfilter: conntrack: don't attempt to iterate over empty table

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]

Message-ID: <20160503174144.GA3782@salvia>
Date:	Tue, 3 May 2016 19:41:44 +0200
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Florian Westphal <fw@...len.de>
Cc:	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH nf-next 3/9] netfilter: conntrack: don't attempt to
 iterate over empty table

On Tue, May 03, 2016 at 07:17:44PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@...filter.org> wrote:
> > On Thu, Apr 28, 2016 at 07:13:42PM +0200, Florian Westphal wrote:
> > > Once we place all conntracks into same table iteration becomes more
> > > costly because the table contains conntracks that we are not interested
> > > in (belonging to other netns).
> > > 
> > > So don't bother scanning if the current namespace has no entries.
> > > 
> > > Signed-off-by: Florian Westphal <fw@...len.de>
> > > ---
> > >  net/netfilter/nf_conntrack_core.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > > index 29fa08b..f2e75a5 100644
> > > --- a/net/netfilter/nf_conntrack_core.c
> > > +++ b/net/netfilter/nf_conntrack_core.c
> > > @@ -1428,6 +1428,9 @@ void nf_ct_iterate_cleanup(struct net *net,
> > >  
> > >  	might_sleep();
> > >  
> > > +	if (atomic_read(&net->ct.count) == 0)
> > > +		return;
> > 
> > This optimization gets defeated with just one single conntrack (ie.
> > net->ct.count == 1), so I wonder if this is practical thing.
> 
> I was thinking of the cleanup we do in the netns exit path
> (in nf_conntrack_cleanup_net_list() ).

Right, but in that path we still have entries in the table.

> If you don't like this I can move the check here:
> 
> i_see_dead_people:
>     busy = 0;
>     list_for_each_entry(net, net_exit_list, exit_list) {
>     // here
>     if (atomic_read .. > 0)
>        nf_ct_iterate_cleanup(net, kill_all, ...

I don't mind about placing this or there, as I said, my question is
how often we will hit this optimization in a real scenario.

If you think the answer is often, then this will help.

Otherwise, every time we'll go container destruction path, we'll hit
slow path, ie.  scanning the full table.

> > At the cost of consuming more memory per conntrack, we may consider
> > adding a per-net list so this iteration doesn't become a problem.
> 
> I don't think that will be needed.   We don't have any such iterations
> in the fast path.
>
> For dumps via ctnetlink it shouldn't be a big deal either, if needed
> we can optimize that to use rcu readlocks only and 'upgrade' to locked
> path only when we want to dump the candidate ct.
> for deferred pruning).
> early_drop will go away soon (i'll rework it to do the early_drop from
> work queue).

OK.