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