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

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]

Message-ID: <4FEA6309.5060305@cn.fujitsu.com>
Date:	Wed, 27 Jun 2012 09:34:01 +0800
From:	Gao feng <gaofeng@...fujitsu.com>
To:	Pablo Neira Ayuso <pablo@...filter.org>
CC:	netdev@...r.kernel.org, netfilter-devel@...r.kernel.org
Subject: Re: [PATCH 04/13] netfilter: regard users as refcount for l4proto's
 per-net data

于 2012年06月26日 22:47, Pablo Neira Ayuso 写道:
> 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@...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...

Sorry, I don't know which the same thing I fixed in this patch and 1/13 patch.
the 1/13 patch only change the proto's registration order. and this patch doesn't
change the registration order.

This patch is used to try to make variable "users" clear.

> 
> 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@...r.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html