On Mon, Jan 07, 2013 at 03:15:17AM +0100, Pablo Neira Ayuso wrote:

Hi Gao,

On Mon, Jan 07, 2013 at 09:27:37AM +0800, Gao feng wrote:

quoted

Hi Pablo,

On 01/05/13 11:50, Pablo Neira Ayuso wrote:

quoted

Hi Gao,

On Fri, Dec 28, 2012 at 10:36:46AM +0800, Gao feng wrote:

quoted

Currectly we unregister proto before all conntrack entries of
this proto being destroyed. so in function destroy_conntrack
we can't find proper l4proto to call l4proto->destroy.
this will cause resource leak.

Good catch.

But better to remove the entries before unregistering the protocol
tracker, so l4proto->destroy is always called.

I think the reason we unregister proto before remove all entries
is to avoid new entry for this protocol being created.

If we remove all contrack entries before unregistration, there maybe
some new entries being created between nf_ct_iterate_cleanup and
nf_conntrack_l4proto_unregister_net.this will cause some terrible things.

Leaks are less likely to happen, but may well still happen while
racing with protocol removal, agreed.

quoted

SO I think we should make proto unavailable first,remove all entries next,
and remove proto from nf_ct_protos arrays finally.

Your proposal adds a branch in the packet path to fix an issue that is
specific of the GRE protocol tracker. In the current code, this fixes
the memory leak while removing the nf_conntrack_proto_gre module,
which is a rare operation.

We have to come with a less intrusive solution.

Wait. The existing code is calling nf_ct_gre_keymap_flush in
proto_gre_net_exit path, so those keymap objects are not leaked.