Re: [PATCH 04/13] netfilter: regard users as refcount for l4proto's per-net data

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Jun 26, 2012 at 11:58:45AM +0800, Gao feng wrote:
> Hi Pablo:
> 于 2012年06月25日 19:20, Pablo Neira Ayuso 写道:
> > On Thu, Jun 21, 2012 at 10:36:41PM +0800, Gao feng wrote:
> >> Now, nf_proto_net's users is confusing.
> >> we should regard it as the refcount for l4proto's per-net data,
> >> because maybe there are two l4protos use the same per-net data.
> >>
> >> so increment pn->users when nf_conntrack_l4proto_register
> >> success, and decrement it for nf_conntrack_l4_unregister case.
> >>
> >> because nf_conntrack_l3proto_ipv[4|6] don't use the same per-net
> >> data,so we don't need to add a refcnt for their per-net data.
> >>
> >> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> >> ---
> >>  net/netfilter/nf_conntrack_proto.c |   76 ++++++++++++++++++++++--------------
> >>  1 files changed, 46 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
> >> index 9d6b6ab..63612e6 100644
> >> --- a/net/netfilter/nf_conntrack_proto.c
> >> +++ b/net/netfilter/nf_conntrack_proto.c
> > [...]
> >> @@ -458,23 +446,32 @@ int nf_conntrack_l4proto_register(struct net *net,
> >>  				  struct nf_conntrack_l4proto *l4proto)
> >>  {
> >>  	int ret = 0;
> >> +	struct nf_proto_net *pn = NULL;
> >>  
> >>  	if (l4proto->init_net) {
> >>  		ret = l4proto->init_net(net, l4proto->l3proto);
> >>  		if (ret < 0)
> >> -			return ret;
> >> +			goto out;
> >>  	}
> >>  
> >> -	ret = nf_ct_l4proto_register_sysctl(net, l4proto);
> >> +	pn = nf_ct_l4proto_net(net, l4proto);
> >> +	if (pn == NULL)
> >> +		goto out;
> > 
> > Same thing here, we're leaking memory allocated by l4proto->init_net.
> 
> if pn is NULL,init_net can't allocate memory for pn->ctl_table.
> So I think it's not memory leak here.

Sorry, I meant to say the line below. But we've already clarified
this in patch 1/1.

> >> +	ret = nf_ct_l4proto_register_sysctl(net, pn, l4proto);
> >>  	if (ret < 0)
> >> -		return ret;
> >> +		goto out;
> >>  
> >>  	if (net == &init_net) {
> >>  		ret = nf_conntrack_l4proto_register_net(l4proto);
> >> -		if (ret < 0)
> >> -			nf_ct_l4proto_unregister_sysctl(net, l4proto);
> >> +		if (ret < 0) {
> >> +			nf_ct_l4proto_unregister_sysctl(net, pn, l4proto);
> >> +			goto out;
> > 
> > Better replace the two lines above by:
> > 
> > goto out_register_net;
> > 
> > and then...
> > 
> >> +		}
> >>  	}
> >>  
> >> +	pn->users++;
> > 
> > out_register_net:
> >         nf_ct_l4proto_unregister_sysctl(net, pn, l4proto);
> > 
> >> +out:
> >>  	return ret;
> > 
> > I think that this change is similar to patch 1/1, I think you should
> > send it as a separated patch.
> > 
> 
> Yes, It looks better.
> should I change this and rebase whole patchset or
> maybe you just apply this patchset and then I send a cleanup patch to do this?

This patch includes changes that are not included in the description,
so you have two choices:

1) You resend me this patch with appropriate description (including
the fact that you're fixing the same thing that patch 1/1 does). This
option still I don't like too much, since making two different things
in one single patch is nasty, but well if you push me...

2) you split the patch in two, with the appropriate descriptions each
and you'll make me happy.
--
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