| Message ID | 20130105035048.GA20027@1984 |
|---|---|
| State | Not Applicable |
| Headers | show |
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. SO I think we should make proto unavailable first,remove all entries next, and remove proto from nf_ct_protos arrays finally. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Regards. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. I will resend this patch. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From 1c082b3ef4c9bf8bfd0159142ce6ffc49aa7bab2 Mon Sep 17 00:00:00 2001 From: Pablo Neira Ayuso <pablo@netfilter.org> Date: Fri, 4 Jan 2013 22:09:44 +0100 Subject: [PATCH] netfilter: nf_conntrack: fix memory leak during unregistration with GRE entries Protocol trackers are unregistered before conntrack entries of that type are removed. For that reason, l4proto->destroy is never called and that results in leaking the keymap. Fix this by releasing entries before unregistering protocols. Reported-by: Gao feng <gaofeng@cn.fujitsu.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- net/netfilter/nf_conntrack_proto.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c index 51e928d..29cd353 100644 --- a/net/netfilter/nf_conntrack_proto.c +++ b/net/netfilter/nf_conntrack_proto.c @@ -488,6 +488,9 @@ void nf_conntrack_l4proto_unregister(struct net *net, { struct nf_proto_net *pn = NULL; + /* Remove all contrack entries before unregistration */ + nf_ct_iterate_cleanup(net, kill_l4proto, l4proto); + if (net == &init_net) nf_conntrack_l4proto_unregister_net(l4proto); @@ -497,9 +500,6 @@ void nf_conntrack_l4proto_unregister(struct net *net, pn->users--; nf_ct_l4proto_unregister_sysctl(net, pn, l4proto); - - /* Remove all contrack entries for this protocol */ - nf_ct_iterate_cleanup(net, kill_l4proto, l4proto); } EXPORT_SYMBOL_GPL(nf_conntrack_l4proto_unregister); -- 1.7.10.4