netdev - Re: [PATCH] [RFC] netfilter: ct: flush net_gre->keymap_list only in nf_conntrack_proto

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: <20140408065600.GA15309@paralelels.com>
Date:	Tue, 8 Apr 2014 10:56:00 +0400
From:	Andrew Vagin <avagin@...allels.com>
To:	Pablo Neira Ayuso <pablo@...filter.org>
CC:	Andrey Vagin <avagin@...nvz.org>, <linux-kernel@...r.kernel.org>,
	<netfilter-devel@...r.kernel.org>, <netfilter@...r.kernel.org>,
	<coreteam@...filter.org>, <netdev@...r.kernel.org>,
	<vvs@...allels.com>, Patrick McHardy <kaber@...sh.net>,
	Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] [RFC] netfilter: ct: flush net_gre->keymap_list only in
 nf_conntrack_proto_gre

On Mon, Apr 07, 2014 at 04:51:58PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Mar 31, 2014 at 06:14:18PM +0400, Andrey Vagin wrote:
> > nf_ct_gre_keymap_flush() removes a nf_ct_gre_keymap object from
> > net_gre->keymap_list and frees the object. But it doesn't clean
> > a reference on this object from ct_pptp_info->keymap[dir].
> > Then nf_ct_gre_keymap_destroy() may release the same object again.
> > 
> > So nf_ct_gre_keymap_flush() can be called only when we are sure that
> > when nf_ct_gre_keymap_destroy will not be called.
> > 
> > nf_ct_gre_keymap is created by nf_ct_gre_keymap_add() and the right way
> > to destroy it is to call nf_ct_gre_keymap_destroy().
> 
> I think you use call nf_ct_iterate_cleanup() to get rid of all GRE
> conntrack entries that have a ct->master, which means that they were
> created by PPTP. That implicitly calls gre_destroy, which releases the
> keymap object.

I'm not sure that I understand your comment. Currently I change nothing
about nf_ct_iterate_cleanup(). In this sentence I point to the right
procedure of destroying a gre conntrack.

nf_ct_iterate_cleanup() is called from nf_ct_l4proto_pernet_unregister().

[ 1858.693250]   <idle>-0       1..s. 118795582us : nf_ct_gre_keymap_add <-exp_gre
[ 1858.693250]   <idle>-0       1..s. 118795598us : nf_ct_gre_keymap_add <-exp_gre
[ 1858.693250]   <idle>-0       1..s. 118795652us : nf_ct_gre_keymap_add <-exp_gre
[ 1858.693250]   <idle>-0       1..s. 118795683us : nf_ct_gre_keymap_add <-exp_gre
[ 1858.693250] kworker/-44      1.... 133620272us : nf_conntrack_pptp_net_exit <-ops_exit_list.isra.1
[ 1858.693250] kworker/-44      1.... 133620274us : nf_ct_gre_keymap_flush <-nf_conntrack_pptp_net_exit
[ 1858.693250] kworker/-44      1.... 133621145us : proto_gre_net_exit <-ops_exit_list.isra.1
[ 1858.693250] kworker/-44      1.... 133621145us : nf_ct_l4proto_pernet_unregister <-proto_gre_net_exit
[ 1858.693250] kworker/-44      1.... 133621146us : nf_ct_iterate_cleanup <-nf_ct_l4proto_pernet_unregister
[ 1858.693250] kworker/-44      1.... 133621187us : nf_ct_gre_keymap_destroy <-gre_destroy

> 
> > This patch marks nf_ct_gre_keymap_flush() as static, so this patch can
> > break compilation of third party modules, which use
> > nf_ct_gre_keymap_flush. I'm not sure this is the right way to deprecate
> > this function.
> 
> No problem with that, we don't mind about out of tree modules. With
> the approach I'm proposing I think you will also need to make this
> function static as it won't be used by PPTP anymore.

I have made this function static. If you don't have other comment, could
you take this patch?

Thanks,
Andrey
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html