Patchwork [01/13] netfilter: fix problem with proto register

login
register
mail settings
Submitter Gao feng
Date June 21, 2012, 2:36 p.m.
Message ID <1340289410-17642-1-git-send-email-gaofeng@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/166313/
State Accepted
Headers show

Comments

Gao feng - June 21, 2012, 2:36 p.m.
before commit 2c352f444ccfa966a1aa4fd8e9ee29381c467448
(netfilter: nf_conntrack: prepare namespace support for
l4 protocol trackers), we register sysctl before register
protos, so if sysctl is registered faild, the protos will
not be registered.

but now, we register protos first, and when register
sysctl failed, we can use protos too, it's different
from before.

so change to register sysctl before register protos.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/netfilter/nf_conntrack_proto.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)
Pablo Neira - June 25, 2012, 11:12 a.m.
On Thu, Jun 21, 2012 at 10:36:38PM +0800, Gao feng wrote:
> before commit 2c352f444ccfa966a1aa4fd8e9ee29381c467448
> (netfilter: nf_conntrack: prepare namespace support for
> l4 protocol trackers), we register sysctl before register
> protos, so if sysctl is registered faild, the protos will
> not be registered.
> 
> but now, we register protos first, and when register
> sysctl failed, we can use protos too, it's different
> from before.

No, this has to be an all-or-nothing game. If one fails, everything
else that you've registered has to be unregistered.

> so change to register sysctl before register protos.
> 
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
>  net/netfilter/nf_conntrack_proto.c |   36 +++++++++++++++++++++++-------------
>  1 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
> index 1ea9194..9bd88aa 100644
> --- a/net/netfilter/nf_conntrack_proto.c
> +++ b/net/netfilter/nf_conntrack_proto.c
> @@ -253,18 +253,23 @@ int nf_conntrack_l3proto_register(struct net *net,
>  {
>  	int ret = 0;
>  
> -	if (net == &init_net)
> -		ret = nf_conntrack_l3proto_register_net(proto);
> +	if (proto->init_net) {
> +		ret = proto->init_net(net);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
> +	ret = nf_ct_l3proto_register_sysctl(net, proto);
>  	if (ret < 0)
>  		return ret;

This is still wrong.

If nf_ct_l3proto_register_sysctl fails, we'll leak the memory that has
been reserved by proto->init_net.

> -	if (proto->init_net) {
> -		ret = proto->init_net(net);
> +	if (net == &init_net) {
> +		ret = nf_conntrack_l3proto_register_net(proto);
>  		if (ret < 0)
> -			return ret;
> +			nf_ct_l3proto_unregister_sysctl(net, proto);
>  	}
> -	return nf_ct_l3proto_register_sysctl(net, proto);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(nf_conntrack_l3proto_register);
>  
> @@ -454,19 +459,24 @@ int nf_conntrack_l4proto_register(struct net *net,
>  				  struct nf_conntrack_l4proto *l4proto)
>  {
>  	int ret = 0;
> -	if (net == &init_net)
> -		ret = nf_conntrack_l4proto_register_net(l4proto);
>  
> -	if (ret < 0)
> -		return ret;
> -
> -	if (l4proto->init_net)
> +	if (l4proto->init_net) {
>  		ret = l4proto->init_net(net);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
> +	ret = nf_ct_l4proto_register_sysctl(net, l4proto);
>  	if (ret < 0)
>  		return ret;
>  
> -	return nf_ct_l4proto_register_sysctl(net, l4proto);
> +	if (net == &init_net) {
> +		ret = nf_conntrack_l4proto_register_net(l4proto);
> +		if (ret < 0)
> +			nf_ct_l4proto_unregister_sysctl(net, l4proto);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(nf_conntrack_l4proto_register);
>  
> -- 
> 1.7.7.6
> 
> --
> 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
--
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 - June 26, 2012, 3:40 a.m.
Hi Pablo:

于 2012年06月25日 19:12, Pablo Neira Ayuso 写道:
> On Thu, Jun 21, 2012 at 10:36:38PM +0800, Gao feng wrote:
>> before commit 2c352f444ccfa966a1aa4fd8e9ee29381c467448
>> (netfilter: nf_conntrack: prepare namespace support for
>> l4 protocol trackers), we register sysctl before register
>> protos, so if sysctl is registered faild, the protos will
>> not be registered.
>>
>> but now, we register protos first, and when register
>> sysctl failed, we can use protos too, it's different
>> from before.
> 
> No, this has to be an all-or-nothing game. If one fails, everything
> else that you've registered has to be unregistered.

indeed,this is an all-or-nothing game right now,please look at the ipv4_net_init,
when we register nf_conntrack_l3proto_ipv4 failed,we will unregister the already
registered l4protoes, and in nf_conntrack_l4proto_unregister,we will call
nf_ct_l4proto_unregister_sysctl to free the sysctl table.

> 
>> so change to register sysctl before register protos.
>>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>> ---
>>  net/netfilter/nf_conntrack_proto.c |   36 +++++++++++++++++++++++-------------
>>  1 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
>> index 1ea9194..9bd88aa 100644
>> --- a/net/netfilter/nf_conntrack_proto.c
>> +++ b/net/netfilter/nf_conntrack_proto.c
>> @@ -253,18 +253,23 @@ int nf_conntrack_l3proto_register(struct net *net,
>>  {
>>  	int ret = 0;
>>  
>> -	if (net == &init_net)
>> -		ret = nf_conntrack_l3proto_register_net(proto);
>> +	if (proto->init_net) {
>> +		ret = proto->init_net(net);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>>  
>> +	ret = nf_ct_l3proto_register_sysctl(net, proto);
>>  	if (ret < 0)
>>  		return ret;
> 
> This is still wrong.
> 
> If nf_ct_l3proto_register_sysctl fails, we'll leak the memory that has
> been reserved by proto->init_net.
> 

we have freed the memory in nf_ct_l[3,4]proto_register_sysctl when we
call nf_ct_register_sysctl failed, so there is no need to free this memory
in nf_conntrack_l[3,4]proto_register.

>> -	if (proto->init_net) {
>> -		ret = proto->init_net(net);
>> +	if (net == &init_net) {
>> +		ret = nf_conntrack_l3proto_register_net(proto);
>>  		if (ret < 0)
>> -			return ret;
>> +			nf_ct_l3proto_unregister_sysctl(net, proto);
>>  	}
>> -	return nf_ct_l3proto_register_sysctl(net, proto);
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(nf_conntrack_l3proto_register);
>>  
>> @@ -454,19 +459,24 @@ int nf_conntrack_l4proto_register(struct net *net,
>>  				  struct nf_conntrack_l4proto *l4proto)
>>  {
>>  	int ret = 0;
>> -	if (net == &init_net)
>> -		ret = nf_conntrack_l4proto_register_net(l4proto);
>>  
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	if (l4proto->init_net)
>> +	if (l4proto->init_net) {
>>  		ret = l4proto->init_net(net);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>>  
>> +	ret = nf_ct_l4proto_register_sysctl(net, l4proto);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	return nf_ct_l4proto_register_sysctl(net, l4proto);
>> +	if (net == &init_net) {
>> +		ret = nf_conntrack_l4proto_register_net(l4proto);
>> +		if (ret < 0)
>> +			nf_ct_l4proto_unregister_sysctl(net, l4proto);
>> +	}
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(nf_conntrack_l4proto_register);
>>  
>> -- 
>> 1.7.7.6
>>
>> --
>> 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
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
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 - June 26, 2012, 2:36 p.m.
On Tue, Jun 26, 2012 at 11:40:14AM +0800, Gao feng wrote:
> Hi Pablo:
> 
> 于 2012年06月25日 19:12, Pablo Neira Ayuso 写道:
> > On Thu, Jun 21, 2012 at 10:36:38PM +0800, Gao feng wrote:
> >> before commit 2c352f444ccfa966a1aa4fd8e9ee29381c467448
> >> (netfilter: nf_conntrack: prepare namespace support for
> >> l4 protocol trackers), we register sysctl before register
> >> protos, so if sysctl is registered faild, the protos will
> >> not be registered.
> >>
> >> but now, we register protos first, and when register
> >> sysctl failed, we can use protos too, it's different
> >> from before.
> > 
> > No, this has to be an all-or-nothing game. If one fails, everything
> > else that you've registered has to be unregistered.
> 
> indeed,this is an all-or-nothing game right now,please look at the ipv4_net_init,
> when we register nf_conntrack_l3proto_ipv4 failed,we will unregister the already
> registered l4protoes, and in nf_conntrack_l4proto_unregister,we will call
> nf_ct_l4proto_unregister_sysctl to free the sysctl table.

I see proto->init_net allocates in->ctl_table, then
nf_ct_l3proto_register_sysctl release it if it fails. I got confused
because I did not see where that memory was being freed. Then, it's
good.

Still one more thing:

> >> so change to register sysctl before register protos.
> >>
> >> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> >> ---
> >>  net/netfilter/nf_conntrack_proto.c |   36 +++++++++++++++++++++++-------------
> >>  1 files changed, 23 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
> >> index 1ea9194..9bd88aa 100644
> >> --- a/net/netfilter/nf_conntrack_proto.c
> >> +++ b/net/netfilter/nf_conntrack_proto.c
> >> @@ -253,18 +253,23 @@ int nf_conntrack_l3proto_register(struct net *net,
> >>  {
> >>  	int ret = 0;
> >>  
> >> -	if (net == &init_net)
> >> -		ret = nf_conntrack_l3proto_register_net(proto);
> >> +	if (proto->init_net) {

I think proto->init_net has to be mandatory since all protocol support
pernet already. We can add BUG_ON at the beginning of the function if
proto->init_net is not defined.

I can manually add that to the patch if you see no inconvenience with
it.
--
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 - June 27, 2012, 1:38 a.m.
于 2012年06月26日 22:36, Pablo Neira Ayuso 写道:
> On Tue, Jun 26, 2012 at 11:40:14AM +0800, Gao feng wrote:
>> Hi Pablo:
>>
>> 于 2012年06月25日 19:12, Pablo Neira Ayuso 写道:
>>> On Thu, Jun 21, 2012 at 10:36:38PM +0800, Gao feng wrote:
>>>> before commit 2c352f444ccfa966a1aa4fd8e9ee29381c467448
>>>> (netfilter: nf_conntrack: prepare namespace support for
>>>> l4 protocol trackers), we register sysctl before register
>>>> protos, so if sysctl is registered faild, the protos will
>>>> not be registered.
>>>>
>>>> but now, we register protos first, and when register
>>>> sysctl failed, we can use protos too, it's different
>>>> from before.
>>>
>>> No, this has to be an all-or-nothing game. If one fails, everything
>>> else that you've registered has to be unregistered.
>>
>> indeed,this is an all-or-nothing game right now,please look at the ipv4_net_init,
>> when we register nf_conntrack_l3proto_ipv4 failed,we will unregister the already
>> registered l4protoes, and in nf_conntrack_l4proto_unregister,we will call
>> nf_ct_l4proto_unregister_sysctl to free the sysctl table.
> 
> I see proto->init_net allocates in->ctl_table, then
> nf_ct_l3proto_register_sysctl release it if it fails. I got confused
> because I did not see where that memory was being freed. Then, it's
> good.
> 
> Still one more thing:
> 
>>>> so change to register sysctl before register protos.
>>>>
>>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>>>> ---
>>>>  net/netfilter/nf_conntrack_proto.c |   36 +++++++++++++++++++++++-------------
>>>>  1 files changed, 23 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
>>>> index 1ea9194..9bd88aa 100644
>>>> --- a/net/netfilter/nf_conntrack_proto.c
>>>> +++ b/net/netfilter/nf_conntrack_proto.c
>>>> @@ -253,18 +253,23 @@ int nf_conntrack_l3proto_register(struct net *net,
>>>>  {
>>>>  	int ret = 0;
>>>>  
>>>> -	if (net == &init_net)
>>>> -		ret = nf_conntrack_l3proto_register_net(proto);
>>>> +	if (proto->init_net) {
> 
> I think proto->init_net has to be mandatory since all protocol support
> pernet already. We can add BUG_ON at the beginning of the function if
> proto->init_net is not defined.
> 

we can add BUG_ON at nf_conntrack_l4proto_register,because all of the l4protoes
have the init_net function.

BUT nf_conntrack_l3proto_ipv6 doesn't have init_net function,because this proto
doesn't have pernet data, and nf_conntrack_l3proto_ipv4 has pernet data only when
CONFIG_NF_CONNTRACK_PROC_COMPAT is configured.

> I can manually add that to the patch if you see no inconvenience with
> it.
> --
> 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
> 

--
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 - June 27, 2012, 8:53 a.m.
On Wed, Jun 27, 2012 at 09:38:44AM +0800, Gao feng wrote:
> 于 2012年06月26日 22:36, Pablo Neira Ayuso 写道:
> > On Tue, Jun 26, 2012 at 11:40:14AM +0800, Gao feng wrote:
> >> Hi Pablo:
> >>
> >> 于 2012年06月25日 19:12, Pablo Neira Ayuso 写道:
> >>> On Thu, Jun 21, 2012 at 10:36:38PM +0800, Gao feng wrote:
> >>>> before commit 2c352f444ccfa966a1aa4fd8e9ee29381c467448
> >>>> (netfilter: nf_conntrack: prepare namespace support for
> >>>> l4 protocol trackers), we register sysctl before register
> >>>> protos, so if sysctl is registered faild, the protos will
> >>>> not be registered.
> >>>>
> >>>> but now, we register protos first, and when register
> >>>> sysctl failed, we can use protos too, it's different
> >>>> from before.
> >>>
> >>> No, this has to be an all-or-nothing game. If one fails, everything
> >>> else that you've registered has to be unregistered.
> >>
> >> indeed,this is an all-or-nothing game right now,please look at the ipv4_net_init,
> >> when we register nf_conntrack_l3proto_ipv4 failed,we will unregister the already
> >> registered l4protoes, and in nf_conntrack_l4proto_unregister,we will call
> >> nf_ct_l4proto_unregister_sysctl to free the sysctl table.
> > 
> > I see proto->init_net allocates in->ctl_table, then
> > nf_ct_l3proto_register_sysctl release it if it fails. I got confused
> > because I did not see where that memory was being freed. Then, it's
> > good.
> > 
> > Still one more thing:
> > 
> >>>> so change to register sysctl before register protos.
> >>>>
> >>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> >>>> ---
> >>>>  net/netfilter/nf_conntrack_proto.c |   36 +++++++++++++++++++++++-------------
> >>>>  1 files changed, 23 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
> >>>> index 1ea9194..9bd88aa 100644
> >>>> --- a/net/netfilter/nf_conntrack_proto.c
> >>>> +++ b/net/netfilter/nf_conntrack_proto.c
> >>>> @@ -253,18 +253,23 @@ int nf_conntrack_l3proto_register(struct net *net,
> >>>>  {
> >>>>  	int ret = 0;
> >>>>  
> >>>> -	if (net == &init_net)
> >>>> -		ret = nf_conntrack_l3proto_register_net(proto);
> >>>> +	if (proto->init_net) {
> > 
> > I think proto->init_net has to be mandatory since all protocol support
> > pernet already. We can add BUG_ON at the beginning of the function if
> > proto->init_net is not defined.
> > 
> 
> we can add BUG_ON at nf_conntrack_l4proto_register,because all of the l4protoes
> have the init_net function.
> 
> BUT nf_conntrack_l3proto_ipv6 doesn't have init_net function,because this proto
> doesn't have pernet data, and nf_conntrack_l3proto_ipv4 has pernet data only when
> CONFIG_NF_CONNTRACK_PROC_COMPAT is configured.

OK, thanks for the clarification.
--
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

diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index 1ea9194..9bd88aa 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -253,18 +253,23 @@  int nf_conntrack_l3proto_register(struct net *net,
 {
 	int ret = 0;
 
-	if (net == &init_net)
-		ret = nf_conntrack_l3proto_register_net(proto);
+	if (proto->init_net) {
+		ret = proto->init_net(net);
+		if (ret < 0)
+			return ret;
+	}
 
+	ret = nf_ct_l3proto_register_sysctl(net, proto);
 	if (ret < 0)
 		return ret;
 
-	if (proto->init_net) {
-		ret = proto->init_net(net);
+	if (net == &init_net) {
+		ret = nf_conntrack_l3proto_register_net(proto);
 		if (ret < 0)
-			return ret;
+			nf_ct_l3proto_unregister_sysctl(net, proto);
 	}
-	return nf_ct_l3proto_register_sysctl(net, proto);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_l3proto_register);
 
@@ -454,19 +459,24 @@  int nf_conntrack_l4proto_register(struct net *net,
 				  struct nf_conntrack_l4proto *l4proto)
 {
 	int ret = 0;
-	if (net == &init_net)
-		ret = nf_conntrack_l4proto_register_net(l4proto);
 
-	if (ret < 0)
-		return ret;
-
-	if (l4proto->init_net)
+	if (l4proto->init_net) {
 		ret = l4proto->init_net(net);
+		if (ret < 0)
+			return ret;
+	}
 
+	ret = nf_ct_l4proto_register_sysctl(net, l4proto);
 	if (ret < 0)
 		return ret;
 
-	return nf_ct_l4proto_register_sysctl(net, l4proto);
+	if (net == &init_net) {
+		ret = nf_conntrack_l4proto_register_net(l4proto);
+		if (ret < 0)
+			nf_ct_l4proto_unregister_sysctl(net, l4proto);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_l4proto_register);