Patchwork [19/19] netfilter: gre: fix resource leak when unregister gre proto

login
register
mail settings
Submitter Pablo Neira
Date Jan. 5, 2013, 3:50 a.m.
Message ID <20130105035048.GA20027@1984>
Download mbox | patch
Permalink /patch/209644/
State Not Applicable
Headers show

Comments

Pablo Neira - Jan. 5, 2013, 3:50 a.m.
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.

Patch attached.
Gao feng - Jan. 7, 2013, 1:27 a.m.
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
Pablo Neira - Jan. 7, 2013, 2:15 a.m.
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
Pablo Neira - Jan. 7, 2013, 2:38 a.m.
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
Gao feng - Jan. 7, 2013, 2:59 a.m.
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
Gao feng - Jan. 7, 2013, 3:05 a.m.
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
Pablo Neira - Jan. 7, 2013, 3:27 a.m.
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
Gao feng - Jan. 7, 2013, 3:43 a.m.
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

Patch

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