On 01/07/13 10:38, Pablo Neira Ayuso wrote: > 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: >>> Hi Pablo, >>> >>> On 01/05/13 11:50, Pablo Neira Ayuso wrote: >>>> Hi Gao, >>>> >>>> On Fri, Dec 28, 2012 at 10:36:46AM +0800, Gao feng wrote: >>>>> 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. >> >>> 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. Agree, it looks like nf_ct_gre_keymap_flush is added to solve this problem. Please ignore this patch. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
