Re: [PATCH] netfilter: accounting rework: ct_extend + 64bit counters (v4)

[Patrick McHardy <kaber@trash.net>]

Krzysztof Oledzki wrote:
> 
> 
> On Wed, 9 Jul 2008, Patrick McHardy wrote:
> 
>> Krzysztof Piotr Oledzki wrote:
>>>> From 187703b8bfc30048c3a973fea6d3b3ce9bcbb49a Mon Sep 17 00:00:00 2001
>>> From: Krzysztof Piotr Oledzki <ole@ans.pl>
>>> Date: Mon, 7 Jul 2008 21:49:57 +0200
>>> Subject: netfilter: accounting rework: ct_extend + 64bit counters (v4)
>>>
>>> Initially netfilter has had 64bit counters for conntrack-based 
>>> accounting, but
>>> it was changed in 2.6.14 to save memory. Unfortunately in-kernel 
>>> 64bit counters are
>>> still required, for example for "connbytes" extension. However, 64bit 
>>> counters
>>> waste a lot of memory and it was not possible to enable/disable it 
>>> runtime.
>>>
>>> This patch:
>>>  - reimplements accounting with respect to the extension infrastructure,
>>>  - makes one global version of seq_print_acct() instead of two 
>>> seq_print_counters(),
>>>  - makes it possible to enable it at boot time (for 
>>> CONFIG_SYSCTL/CONFIG_SYSFS=n),
>>>  - makes it possible to enable/disable it at runtime by sysctl or sysfs,
>>>  - extends counters from 32bit to 64bit,
>>>  - renames ip_conntrack_counter -> nf_conn_counter,
>>>  - enables accounting code unconditionally (no longer depends on 
>>> CONFIG_NF_CT_ACCT),
>>>  - set initial accounting enable state based on CONFIG_NF_CT_ACCT
>>>  - removes buggy IPCT_COUNTER_FILLING event handling.
>>>
>>> If accounting is enabled newly created connections get additional 
>>> acct extend.
>>> Old connections are not changed as it is not possible to add a 
>>> ct_extend area
>>> to confirmed conntrack. Accounting is performed for all connections with
>>> acct extend regardless of a current state of 
>>> "net.netfilter.nf_conntrack_acct".
>>
>> Applied with the attached minor cleanups. Thanks Krzysztof!
>>
>> There is one things I think needs fixing however:
>>
>>>              if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) ==
>>> -                        IPCTNL_MSG_CT_GET_CTRZERO)
>>> -                memset(&ct->counters, 0, sizeof(ct->counters));
>>> -#endif
>>> +                        IPCTNL_MSG_CT_GET_CTRZERO) {
>>> +                struct nf_conn_counter *acct;
>>> +
>>> +                acct = nf_conn_acct_find(ct);
>>> +                if (acct)
>>> +                    memset(acct, 0, sizeof(struct 
>>> nf_conn_counter[IP_CT_DIR_MAX]));
>>
>> This needs locking.
> 
> Thanks, I'll look at it. Is it enough to place it between 
> spin_lock_bh/spin_unlock_bh, like this?
> 
> spin_lock_bh(&nf_conntrack_lock);
> 
> memset()
> 
> spin_unlock_bh(&nf_conntrack_lock);

Probably better to grab it outside the loop to avoid repeatedly
taking and releasing it again.