On 01/07/13 11:27, Pablo Neira Ayuso wrote: > On Mon, Jan 07, 2013 at 11:05:12AM +0800, Gao feng wrote: >> On 01/07/13 10:59, Gao feng wrote: >>> 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. >> >> I mean ignore the codes of gre part. >> I still think nf_ct_iterate_cleanup doesn't make sure all entries being >> destroyed. > > There is another round of nf_ct_iterate cleanup in > nf_conntrack_cleanup_net. So I don't see any possible leak so far. Let > me know if you find anything. > Ok,I am wrong... Please ignore this patch. ;) -- 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
