netdev - Re: [PATCH 01/13] netfilter: fix problem with proto register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 26 Jun 2012 16:36:20 +0200
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Gao feng <gaofeng@...fujitsu.com>
Cc:	netdev@...r.kernel.org, netfilter-devel@...r.kernel.org
Subject: Re: [PATCH 01/13] netfilter: fix problem with proto register

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@...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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html