| Message ID | 1340289410-17642-1-git-send-email-gaofeng@cn.fujitsu.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
于 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
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
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);
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(-)