reset nfacct counters

reset nfacct counters

[Alexey Perevalov]

Hello Pablo and Mathieu.
I would like to thank you for quota with notification implementation in 
nfacct.

But also I want to discuss about resetting counters value. Right now 
nfacct has 2 way to get counter
NFNL_MSG_ACCT_GET and NFNL_MSG_ACCT_GET_CTRZERO, last one is intended to 
nullify accumulated counter.
It resets counters with and without populated quota value. After commit 
683399eddb nfacct really operates
with 2 different entities: pure counter and quota based counter. If so, 
why not to operate with it separately,
maybe by some filter (flag).

Also it was strange for me, why reset is not a command of command line 
tool nfacct, like get? Ok, if it's an argument of get, why not it's a 
flag (attribute) in netlink serialization?

Why I'm asking such questions. My use case requires periodic reset of 
the counters, also I have quota based counters and I don't want to reset 
them.
I could work around it from user space, for example, I could get quota 
based counter before I'm going to reset counters, delete it and key it 
in after counters reset. As you could see too much operations. Or I 
could could avoid reseting, but in this case I need to operates with 
deltas in user space and it's not robust in situation when my daemon is 
restarting. Every variant in user space leads to more run time complexity.

And my final question, will you accept a patch, which will move CTRZERO 
to netlink attribute and CTRZERO will be expanded to CTRZERO_OVERAL, 
CTRZERO_COUNTER and CTRZERO_QUOTA? For both kernel side and user space 
part (nfacct tool with libraries).


-- 
Best regards,
Alexey Perevalov

Re: reset nfacct counters

[Pablo Neira Ayuso]

Hi Alexey,

On Fri, Jul 25, 2014 at 12:05:12PM +0400, Alexey Perevalov wrote:
> Hello Pablo and Mathieu.
> I would like to thank you for quota with notification implementation
> in nfacct.
> 
> But also I want to discuss about resetting counters value. Right now
> nfacct has 2 way to get counter
> NFNL_MSG_ACCT_GET and NFNL_MSG_ACCT_GET_CTRZERO, last one is
> intended to nullify accumulated counter.
> It resets counters with and without populated quota value. After
> commit 683399eddb nfacct really operates
> with 2 different entities: pure counter and quota based counter. If
> so, why not to operate with it separately,
> maybe by some filter (flag).
> 
> Also it was strange for me, why reset is not a command of command
> line tool nfacct, like get? Ok, if it's an argument of get, why not
> it's a flag (attribute) in netlink serialization?
> 
> Why I'm asking such questions. My use case requires periodic reset
> of the counters, also I have quota based counters and I don't want
> to reset them.
>
> I could work around it from user space, for example, I could get
> quota based counter before I'm going to reset counters, delete it
> and key it in after counters reset. As you could see too much
> operations. Or I could could avoid reseting, but in this case I need
> to operates with deltas in user space and it's not robust in
> situation when my daemon is restarting. Every variant in user space
> leads to more run time complexity.
> 
> And my final question, will you accept a patch, which will move
> CTRZERO to netlink attribute and CTRZERO will be expanded to
> CTRZERO_OVERAL, CTRZERO_COUNTER and CTRZERO_QUOTA? For both kernel
> side and user space part (nfacct tool with libraries).

You can make a patch that uses NFACCT_FLAGS in the nfnl_acct_get()
path to specify the filtering criteria. In the nfnl_acct_get() path,
you can use c->data in struct netlink_dump_control to attach the
filter, which needs to be allocated before the dump_start() call and
released through the dump_done() callback. If no NFACCT_FLAGS are
specified, no filter is specified and all counters should be cleared
as it happens now.

Re: reset nfacct counters

[Alexey Perevalov]

On 07/25/2014 08:01 PM, Pablo Neira Ayuso wrote:
> Hi Alexey,
>
> On Fri, Jul 25, 2014 at 12:05:12PM +0400, Alexey Perevalov wrote:
>> Hello Pablo and Mathieu.
>> I would like to thank you for quota with notification implementation
>> in nfacct.
>>
>> But also I want to discuss about resetting counters value. Right now
>> nfacct has 2 way to get counter
>> NFNL_MSG_ACCT_GET and NFNL_MSG_ACCT_GET_CTRZERO, last one is
>> intended to nullify accumulated counter.
>> It resets counters with and without populated quota value. After
>> commit 683399eddb nfacct really operates
>> with 2 different entities: pure counter and quota based counter. If
>> so, why not to operate with it separately,
>> maybe by some filter (flag).
>>
>> Also it was strange for me, why reset is not a command of command
>> line tool nfacct, like get? Ok, if it's an argument of get, why not
>> it's a flag (attribute) in netlink serialization?
>>
>> Why I'm asking such questions. My use case requires periodic reset
>> of the counters, also I have quota based counters and I don't want
>> to reset them.
>>
>> I could work around it from user space, for example, I could get
>> quota based counter before I'm going to reset counters, delete it
>> and key it in after counters reset. As you could see too much
>> operations. Or I could could avoid reseting, but in this case I need
>> to operates with deltas in user space and it's not robust in
>> situation when my daemon is restarting. Every variant in user space
>> leads to more run time complexity.
>>
>> And my final question, will you accept a patch, which will move
>> CTRZERO to netlink attribute and CTRZERO will be expanded to
>> CTRZERO_OVERAL, CTRZERO_COUNTER and CTRZERO_QUOTA? For both kernel
>> side and user space part (nfacct tool with libraries).
> You can make a patch that uses NFACCT_FLAGS in the nfnl_acct_get()
> path to specify the filtering criteria. In the nfnl_acct_get() path,
> you can use c->data in struct netlink_dump_control to attach the
> filter, which needs to be allocated before the dump_start() call and
> released through the dump_done() callback. If no NFACCT_FLAGS are
> specified, no filter is specified and all counters should be cleared
> as it happens now.
>
Thank you Pablo for such detailed advice, I hope I'll send patches next 
week.

[PATCH] reset nfacct counters

[Alexey Perevalov]

Hello Pablo,
I used you guidance on flags usage for separate counters reset.
If no flags were specified we'll have old behaviour. I wanted to remove
NFNL_MSG_ACCT_GET_CTRZERO and use combination of (NFACCT_F_RESET_COUNTERS |
NFACCT_F_RESET_QUOTAS). But in this case I didn't find how to solve backward
compatibility issue.

You proposed to allocate memory for filter entity to use it with
netlink_dump_control, but netlink_dump_start is synchronous all the way,
and stack variable could be used there. But I could be wrong here and
there is a use case where netlink_dump_control's callback is calling from
another thread.

Due nfnetlink_acct header was modified, I think I need to send additional patches
for nfacct and libnetfilter_acct git reposotories. I saw such practice,
nfnetlink_acct.h duplicates in these repositories.

This patch was made for git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
repository, on top of 633594bb2d3890711a887897f2003f41735f0dfa commit.

Do you need a patch for nfacct command line tool to support following behaviour:
nfacct list reset [counter|quota] ?

Alexey Perevalov (1):
  netfilter: nfnetlink_acct: use flag to reset counters

 include/uapi/linux/netfilter/nfnetlink_acct.h |    2 ++
 net/netfilter/nfnetlink_acct.c                |   30 ++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
1.7.9.5

Re: [PATCH] reset nfacct counters

[Pablo Neira Ayuso]

On Mon, Jul 28, 2014 at 09:57:50PM +0400, Alexey Perevalov wrote:
> Hello Pablo,
> I used you guidance on flags usage for separate counters reset.
> If no flags were specified we'll have old behaviour. I wanted to remove
> NFNL_MSG_ACCT_GET_CTRZERO and use combination of (NFACCT_F_RESET_COUNTERS |
> NFACCT_F_RESET_QUOTAS). But in this case I didn't find how to solve backward
> compatibility issue.

Just made a proposal to avoid adding these new flags.

> You proposed to allocate memory for filter entity to use it with
> netlink_dump_control, but netlink_dump_start is synchronous all the way,
> and stack variable could be used there. But I could be wrong here and
> there is a use case where netlink_dump_control's callback is calling from
> another thread.

You can't do that. The dumping happens per recvmsg() call in netlink,
if the number of nfacct objects is larger than one page
(NLMSG_GOODSIZE), the next call to recvmsg() will reference to an
invalid memory area. You really need to allocate this, which is a bit
overkill for just one u32 flags filter, but I guess we'll have more
selectors to filter out nfacct objects in the future.

> Due nfnetlink_acct header was modified, I think I need to send additional patches
> for nfacct and libnetfilter_acct git reposotories. I saw such practice,
> nfnetlink_acct.h duplicates in these repositories.
> 
> This patch was made for git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> repository, on top of 633594bb2d3890711a887897f2003f41735f0dfa commit.

That tree is fine in this case, there is no changes in nf-next that
can clash with this.

> Do you need a patch for nfacct command line tool to support following behaviour:
> nfacct list reset [counter|quota] ?

Yes please :-), it would be great so others in the community can
benefit from this new feature, it's just little extra work. Thanks.

[PATCH] netfilter: nfnetlink_acct: use flag to reset counters

[Alexey Perevalov]

Two additional NFACCT_F* was introduced for ability to reset
counters with and without quota separately.

It could be useful when client has to reset counters and wants to keep
quotas untouched or vice versa without flushing and renewing.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 include/uapi/linux/netfilter/nfnetlink_acct.h |    2 ++
 net/netfilter/nfnetlink_acct.c                |   30 ++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
index 51404ec..1181c8e 100644
--- a/include/uapi/linux/netfilter/nfnetlink_acct.h
+++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
@@ -18,6 +18,8 @@ enum nfnl_acct_flags {
 	NFACCT_F_QUOTA_PKTS	= (1 << 0),
 	NFACCT_F_QUOTA_BYTES	= (1 << 1),
 	NFACCT_F_OVERQUOTA	= (1 << 2), /* can't be set from userspace */
+	NFACCT_F_RESET_COUNTERS = (1 << 3),
+	NFACCT_F_RESET_QUOTAS   = (1 << 4),
 };
 
 enum nfnl_acct_type {
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 2baa125..1f47503 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -121,9 +121,23 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
 	return 0;
 }
 
+static inline bool
+is_counters_reset(u32 nfacct_flags, unsigned long counter_flags)
+{
+	return nfacct_flags & NFACCT_F_RESET_COUNTERS &&
+		!(counter_flags & NFACCT_F_QUOTA);
+}
+
+static inline bool
+is_quotas_reset(u32 nfacct_flags, unsigned long counter_flags)
+{
+	return nfacct_flags & NFACCT_F_RESET_QUOTAS &&
+		counter_flags & NFACCT_F_QUOTA;
+}
+
 static int
 nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
-		   int event, struct nf_acct *acct)
+		   int event, struct nf_acct *acct, u32 nfacct_flags)
 {
 	struct nlmsghdr *nlh;
 	struct nfgenmsg *nfmsg;
@@ -143,7 +157,9 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	if (nla_put_string(skb, NFACCT_NAME, acct->name))
 		goto nla_put_failure;
 
-	if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
+	if (type == NFNL_MSG_ACCT_GET_CTRZERO &&
+		(!nfacct_flags || is_counters_reset(nfacct_flags, acct->flags) ||
+		is_quotas_reset(nfacct_flags, acct->flags))) {
 		pkts = atomic64_xchg(&acct->pkts, 0);
 		bytes = atomic64_xchg(&acct->bytes, 0);
 		smp_mb__before_atomic();
@@ -177,6 +193,7 @@ static int
 nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct nf_acct *cur, *last;
+	u32 flags = cb->data ? *(u32 *)cb->data : 0;
 
 	if (cb->args[2])
 		return 0;
@@ -196,7 +213,7 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
 				       cb->nlh->nlmsg_seq,
 				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
-				       NFNL_MSG_ACCT_NEW, cur) < 0) {
+				       NFNL_MSG_ACCT_NEW, cur, flags) < 0) {
 			cb->args[1] = (unsigned long)cur;
 			break;
 		}
@@ -214,10 +231,13 @@ nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	int ret = -ENOENT;
 	struct nf_acct *cur;
 	char *acct_name;
+	u32 flags = tb[NFACCT_FLAGS] ?
+		ntohl(nla_get_u32(tb[NFACCT_FLAGS])) : 0;
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = nfnl_acct_dump,
+			.data = &flags,
 		};
 		return netlink_dump_start(nfnl, skb, nlh, &c);
 	}
@@ -241,7 +261,7 @@ nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 		ret = nfnl_acct_fill_info(skb2, NETLINK_CB(skb).portid,
 					 nlh->nlmsg_seq,
 					 NFNL_MSG_TYPE(nlh->nlmsg_type),
-					 NFNL_MSG_ACCT_NEW, cur);
+					 NFNL_MSG_ACCT_NEW, cur, flags);
 		if (ret <= 0) {
 			kfree_skb(skb2);
 			break;
@@ -386,7 +406,7 @@ static void nfnl_overquota_report(struct nf_acct *nfacct)
 		return;
 
 	ret = nfnl_acct_fill_info(skb, 0, 0, NFNL_MSG_ACCT_OVERQUOTA, 0,
-				  nfacct);
+				  nfacct, 0);
 	if (ret <= 0) {
 		kfree_skb(skb);
 		return;
-- 
1.7.9.5

Re: [PATCH] netfilter: nfnetlink_acct: use flag to reset counters

[Pablo Neira Ayuso]

On Mon, Jul 28, 2014 at 09:57:51PM +0400, Alexey Perevalov wrote:
> Two additional NFACCT_F* was introduced for ability to reset
> counters with and without quota separately.
> 
> It could be useful when client has to reset counters and wants to keep
> quotas untouched or vice versa without flushing and renewing.
> 
> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> ---
>  include/uapi/linux/netfilter/nfnetlink_acct.h |    2 ++
>  net/netfilter/nfnetlink_acct.c                |   30 ++++++++++++++++++++-----
>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
> index 51404ec..1181c8e 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
> @@ -18,6 +18,8 @@ enum nfnl_acct_flags {
>  	NFACCT_F_QUOTA_PKTS	= (1 << 0),
>  	NFACCT_F_QUOTA_BYTES	= (1 << 1),
>  	NFACCT_F_OVERQUOTA	= (1 << 2), /* can't be set from userspace */
> +	NFACCT_F_RESET_COUNTERS = (1 << 3),
> +	NFACCT_F_RESET_QUOTAS   = (1 << 4),
>  };
>  
>  enum nfnl_acct_type {
> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
> index 2baa125..1f47503 100644
> --- a/net/netfilter/nfnetlink_acct.c
> +++ b/net/netfilter/nfnetlink_acct.c
> @@ -121,9 +121,23 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static inline bool
> +is_counters_reset(u32 nfacct_flags, unsigned long counter_flags)
> +{
> +	return nfacct_flags & NFACCT_F_RESET_COUNTERS &&
> +		!(counter_flags & NFACCT_F_QUOTA);
> +}
> +
> +static inline bool
> +is_quotas_reset(u32 nfacct_flags, unsigned long counter_flags)
> +{
> +	return nfacct_flags & NFACCT_F_RESET_QUOTAS &&
> +		counter_flags & NFACCT_F_QUOTA;
> +}

I think you can use the existing flags, ie.

1) If no flag is set, it means that userspace wants to dump/reset
everything.

2) If NFACCT_F_QUOTA_PKTS is set, it means that userspace wants to
dump/reset only packet-based quotas.

3) If NFACCT_F_QUOTA_BYTES is set, it means that userspace wants to
dump/reset only byte-based quotas.

4) If NFACCT_F_QUOTA_PKTS|NFACCT_F_QUOTA_BYTES are set, any accounting
object with quota is dump/reset.

5) If NFACCT_F_OVERQUOTA is set, only objects overquota are reset.

... Basically, you could even make any possible combination. I think
that should be flexible enough for all cases.

Therefore:

>  static int
>  nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
> -		   int event, struct nf_acct *acct)
> +		   int event, struct nf_acct *acct, u32 nfacct_flags)
>  {
>  	struct nlmsghdr *nlh;
>  	struct nfgenmsg *nfmsg;
> @@ -143,7 +157,9 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>  	if (nla_put_string(skb, NFACCT_NAME, acct->name))
>  		goto nla_put_failure;
>  
> -	if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
> +	if (type == NFNL_MSG_ACCT_GET_CTRZERO &&
> +		(!nfacct_flags || is_counters_reset(nfacct_flags, acct->flags) ||
> +		is_quotas_reset(nfacct_flags, acct->flags))) {

Replacing this:

                acct->flags & nfacct_flags == nfacct_flags

I think it should be enough.

Re: [PATCH] netfilter: nfnetlink_acct: use flag to reset counters

[Alexey Perevalov]

On 07/29/2014 01:53 AM, Pablo Neira Ayuso wrote:
> On Mon, Jul 28, 2014 at 09:57:51PM +0400, Alexey Perevalov wrote:
>> Two additional NFACCT_F* was introduced for ability to reset
>> counters with and without quota separately.
>>
>> It could be useful when client has to reset counters and wants to keep
>> quotas untouched or vice versa without flushing and renewing.
>>
>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>> ---
>>   include/uapi/linux/netfilter/nfnetlink_acct.h |    2 ++
>>   net/netfilter/nfnetlink_acct.c                |   30 ++++++++++++++++++++-----
>>   2 files changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> index 51404ec..1181c8e 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> @@ -18,6 +18,8 @@ enum nfnl_acct_flags {
>>   	NFACCT_F_QUOTA_PKTS	= (1 << 0),
>>   	NFACCT_F_QUOTA_BYTES	= (1 << 1),
>>   	NFACCT_F_OVERQUOTA	= (1 << 2), /* can't be set from userspace */
>> +	NFACCT_F_RESET_COUNTERS = (1 << 3),
>> +	NFACCT_F_RESET_QUOTAS   = (1 << 4),
>>   };
>>   
>>   enum nfnl_acct_type {
>> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
>> index 2baa125..1f47503 100644
>> --- a/net/netfilter/nfnetlink_acct.c
>> +++ b/net/netfilter/nfnetlink_acct.c
>> @@ -121,9 +121,23 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>>   	return 0;
>>   }
>>   
>> +static inline bool
>> +is_counters_reset(u32 nfacct_flags, unsigned long counter_flags)
>> +{
>> +	return nfacct_flags & NFACCT_F_RESET_COUNTERS &&
>> +		!(counter_flags & NFACCT_F_QUOTA);
>> +}
>> +
>> +static inline bool
>> +is_quotas_reset(u32 nfacct_flags, unsigned long counter_flags)
>> +{
>> +	return nfacct_flags & NFACCT_F_RESET_QUOTAS &&
>> +		counter_flags & NFACCT_F_QUOTA;
>> +}
> I think you can use the existing flags, ie.
>
> 1) If no flag is set, it means that userspace wants to dump/reset
> everything.
>
> 2) If NFACCT_F_QUOTA_PKTS is set, it means that userspace wants to
> dump/reset only packet-based quotas.
>
> 3) If NFACCT_F_QUOTA_BYTES is set, it means that userspace wants to
> dump/reset only byte-based quotas.
>
> 4) If NFACCT_F_QUOTA_PKTS|NFACCT_F_QUOTA_BYTES are set, any accounting
> object with quota is dump/reset.
>
> 5) If NFACCT_F_OVERQUOTA is set, only objects overquota are reset.
>
> ... Basically, you could even make any possible combination. I think
> that should be flexible enough for all cases.

>
> Therefore:
>
>>   static int
>>   nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>> -		   int event, struct nf_acct *acct)
>> +		   int event, struct nf_acct *acct, u32 nfacct_flags)
>>   {
>>   	struct nlmsghdr *nlh;
>>   	struct nfgenmsg *nfmsg;
>> @@ -143,7 +157,9 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>   	if (nla_put_string(skb, NFACCT_NAME, acct->name))
>>   		goto nla_put_failure;
>>   
>> -	if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
>> +	if (type == NFNL_MSG_ACCT_GET_CTRZERO &&
>> +		(!nfacct_flags || is_counters_reset(nfacct_flags, acct->flags) ||
>> +		is_quotas_reset(nfacct_flags, acct->flags))) {
> Replacing this:
>
>                  acct->flags & nfacct_flags == nfacct_flags
>
> I think it should be enough.
>
Yes, agree, but how to be with just counters without a quota,
at first look it should be
#define NFACCT_F_COUNTER ~(NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES | 
NFACCT_F_OVERQUOTA)
if you don't want additional flags, kernel will check it manually, 
because _no_ flag reserved for everything, as before.

Also I decided to put such condition not into nfnl_acct_fill_info, but 
directly into nfnl_acct_dump, it will make this feature more general,
client will get ability to filter it not only for reset command, but 
also for list command, also nfnl_acct_fill_info is using in many places, 
e.g.
just get command or nfnl_overquota_report.

And finally, I found strange approach for working with 
NFACCT_F_OVERQUOTA (value 1 << 2, 4), in nfnl_acct_overquota the 
test_and_set_bit function is used, which wants
Nth bit, and that Nth bit is 4, but not 2. Clearing is fine clear_bit 
accept Nth bit as well,
but nfnl_acct_new gets from netlink attribute NFACCT_F_OVERQUOTA not as 
offset value.

I took a look at it because of:
1. It's not convenient to keep combined bit set for NFACCT_F_COUNTER, 
because NFACCT_F_OVERQUOTA not what we have in acct->flags. Of course if 
you not against such bit set.
2. nlnf_overquata_report sends to user space _forth_ activated bit, but 
not _second_ for NFACCT_F_OVERQUOTA, but NFACCT_F_QUOTA_[BYTES|PKTS] was 
being sent as is. Output of the nfacct, after overquota event occurred, 
not so clear, in case of bytes quota it's just changing to package. It's 
not a big deal to fix it to print "overquoted", but I feel it's better 
to use some unified method to set bits.


-- 
Best regards,
Alexey Perevalov

Re: [PATCH] netfilter: nfnetlink_acct: use flag to reset counters

[Pablo Neira Ayuso]

On Tue, Jul 29, 2014 at 03:46:00PM +0400, Alexey Perevalov wrote:
[...]
> >>@@ -143,7 +157,9 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
> >>  	if (nla_put_string(skb, NFACCT_NAME, acct->name))
> >>  		goto nla_put_failure;
> >>-	if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
> >>+	if (type == NFNL_MSG_ACCT_GET_CTRZERO &&
> >>+		(!nfacct_flags || is_counters_reset(nfacct_flags, acct->flags) ||
> >>+		is_quotas_reset(nfacct_flags, acct->flags))) {
> >Replacing this:
> >
> >                 acct->flags & nfacct_flags == nfacct_flags
> >
> >I think it should be enough.
> >
> Yes, agree, but how to be with just counters without a quota,
> at first look it should be
> #define NFACCT_F_COUNTER ~(NFACCT_F_QUOTA_PKTS |
> NFACCT_F_QUOTA_BYTES | NFACCT_F_OVERQUOTA)

Right. I get the problem, what I proposed is not enough.

We can extend this in a more flexible way to allow better filtering
from userspace, eg.

NFACCT_FLAGS_FILTER (nest)
 NFACCT_VALUE (u32)
 NFACCT_MASK (u32)

The use the value and the mask to perform:

value & filter->mask == filter->data

Where filter comes from netlink cb->data.

Please, see this thread:

http://www.spinics.net/lists/netfilter-devel/msg32173.html

The idea is very similar. We should only have one single key after
your patch to filter by flags.

Is this OK for your needs?

> Also I decided to put such condition not into nfnl_acct_fill_info,
> but directly into nfnl_acct_dump, it will make this feature more
> general,
> client will get ability to filter it not only for reset command, but
> also for list command, also nfnl_acct_fill_info is using in many
> places, e.g.
> just get command or nfnl_overquota_report.

Right, the filtering should be generic for both list and reset
commands.

> And finally, I found strange approach for working with
> NFACCT_F_OVERQUOTA (value 1 << 2, 4), in nfnl_acct_overquota the
> test_and_set_bit function is used, which wants
> Nth bit, and that Nth bit is 4, but not 2. Clearing is fine
> clear_bit accept Nth bit as well,
> but nfnl_acct_new gets from netlink attribute NFACCT_F_OVERQUOTA not
> as offset value.

That's a bug. Would you also send a separated patch for that, please?

> I took a look at it because of:
> 1. It's not convenient to keep combined bit set for
> NFACCT_F_COUNTER, because NFACCT_F_OVERQUOTA not what we have in
> acct->flags. Of course if you not against such bit set.
> 2. nlnf_overquata_report sends to user space _forth_ activated bit,
> but not _second_ for NFACCT_F_OVERQUOTA, but
> NFACCT_F_QUOTA_[BYTES|PKTS] was being sent as is. Output of the
> nfacct, after overquota event occurred, not so clear, in case of
> bytes quota it's just changing to package. It's not a big deal to
> fix it to print "overquoted", but I feel it's better to use some
> unified method to set bits.

That's inconsistent and this of course needs a fix. Patch? Thanks.

RE: [PATCH] netfilter: nfnetlink_acct: use flag to reset counters

[Alexey Perevalov]

----------------------------------------
> Date: Tue, 29 Jul 2014 18:32:09 +0200
> From: pablo@netfilter.org
> To: a.perevalov@samsung.com
> CC: kyungmin.park@samsung.com; hs81.go@samsung.com; netfilter-devel@vger.kernel.org; alexey.perevalov@hotmail.com; mathieu.poirier@linaro.org
> Subject: Re: [PATCH] netfilter: nfnetlink_acct: use flag to reset counters
>
> On Tue, Jul 29, 2014 at 03:46:00PM +0400, Alexey Perevalov wrote:
> [...]
>>>>@@ -143,7 +157,9 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>>> if (nla_put_string(skb, NFACCT_NAME, acct->name))
>>>> goto nla_put_failure;
>>>>- if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
>>>>+ if (type == NFNL_MSG_ACCT_GET_CTRZERO &&
>>>>+ (!nfacct_flags || is_counters_reset(nfacct_flags, acct->flags) ||
>>>>+ is_quotas_reset(nfacct_flags, acct->flags))) {
>>>Replacing this:
>>>
>>> acct->flags & nfacct_flags == nfacct_flags
>>>
>>>I think it should be enough.
>>>
>> Yes, agree, but how to be with just counters without a quota,
>> at first look it should be
>> #define NFACCT_F_COUNTER ~(NFACCT_F_QUOTA_PKTS |
>> NFACCT_F_QUOTA_BYTES | NFACCT_F_OVERQUOTA)
>
> Right. I get the problem, what I proposed is not enough.
>
> We can extend this in a more flexible way to allow better filtering
> from userspace, eg.
>
> NFACCT_FLAGS_FILTER (nest)
> NFACCT_VALUE (u32)
> NFACCT_MASK (u32)
>
> The use the value and the mask to perform:
>
> value & filter->mask == filter->data
>
> Where filter comes from netlink cb->data.
>
> Please, see this thread:
>
> http://www.spinics.net/lists/netfilter-devel/msg32173.html
>
> The idea is very similar. We should only have one single key after
> your patch to filter by flags.
>
> Is this OK for your needs?
I think yes, due filter mechanism should be extendible. As an example,
also I don't want to  receive not yet touched counters (which has 0 bytes).

>
>> Also I decided to put such condition not into nfnl_acct_fill_info,
>> but directly into nfnl_acct_dump, it will make this feature more
>> general,
>> client will get ability to filter it not only for reset command, but
>> also for list command, also nfnl_acct_fill_info is using in many
>> places, e.g.
>> just get command or nfnl_overquota_report.
>
> Right, the filtering should be generic for both list and reset
> commands.
>
>> And finally, I found strange approach for working with
>> NFACCT_F_OVERQUOTA (value 1 << 2, 4), in nfnl_acct_overquota the
>> test_and_set_bit function is used, which wants
>> Nth bit, and that Nth bit is 4, but not 2. Clearing is fine
>> clear_bit accept Nth bit as well,
>> but nfnl_acct_new gets from netlink attribute NFACCT_F_OVERQUOTA not
>> as offset value.
>
> That's a bug. Would you also send a separated patch for that, please?
Ok, but I need to know you opinion. My opinion is following:
if you have 1st, 2nd, 3rd bit for flags, you should use 1st,  2nd, 3rd, and using power of 2
not robust, I think bug was hidden due it was the latest bit. But on other hand, usage of set_bit and clear_bit looks
really perfect. So right now user/kernel space serialization is using NFACCT_F_QUOTA_PKTS|BYTES their values are 1 and 2, I think it's not too late to change the value of enum nfnl_acct_flags to sequential number, but not power of 2 and then use bit helper functions.
But you could stay on old used approach, power of 2 in enum and no bit helpers.

>
>> I took a look at it because of:
>> 1. It's not convenient to keep combined bit set for
>> NFACCT_F_COUNTER, because NFACCT_F_OVERQUOTA not what we have in
>> acct->flags. Of course if you not against such bit set.
>> 2. nlnf_overquata_report sends to user space _forth_ activated bit,
>> but not _second_ for NFACCT_F_OVERQUOTA, but
>> NFACCT_F_QUOTA_[BYTES|PKTS] was being sent as is. Output of the
>> nfacct, after overquota event occurred, not so clear, in case of
>> bytes quota it's just changing to package. It's not a big deal to
>> fix it to print "overquoted", but I feel it's better to use some
>> unified method to set bits.
>
> That's inconsistent and this of course needs a fix. Patch? Thanks
No problem. 
 		 	   		  --
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

[PATCH] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Alexey Perevalov]

I followed the same way as in
net/netfilter/nf_conntrack_netlink.c, I put filter code under ifdef.
Seems in case of not NFACCT_FILTER attribute support at kernel side,
client could not detect it.

Due there is no way to identify version number of the serialized
message, solution for counters is not so robust, e.g. kernel side could
be extended by new NFACCT_F_QUOTA_* value, but client side not. In this
case old version of the client side will get incorrect response for
counters request. I think OS vendors should keep it in sync.

I didn't find a way to support listening/reseting quota of any available type
(NFACCT_F_QUOTA) per one request by only one condition.

I saw the thread "[RFC PATCH libnetfilter_conntrack] add userspace dump filter".
For my purpose, where I want to receive only non zero counters, the proposed
way should be extended by list. NFACCT_FILTER should have NESTED type as you
proposed, also it should contain array of nfacct_filter. And condition should
traverse on list as well. Due receiving counters for me is a primary requirement
and non zero counter is a minory optimization requirement.
I decided to send a patch without key field in nfacct_filter structure at first
stage. But if you wish, I could. I mean, if you want the key field and fetching
by that key, it could be in this patch as well.

If you ok with protocol, I'll send client side patch as well.

Alexey Perevalov (1):
  netfilter: nfnetlink_acct: add filter support to nfacct counter
    list/reset

 include/uapi/linux/netfilter/nfnetlink_acct.h |   12 +++++
 net/netfilter/Kconfig                         |    9 ++++
 net/netfilter/nfnetlink_acct.c                |   61 +++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

-- 
1.7.9.5

[PATCH] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Alexey Perevalov]

Filtering covers following cases:
	1. no filter specified. In this case client will get old
behaviour
	2. filter is specified for getting just counters:
		in this case mask should be NFACCT_F_QUOTAS and value 0
	3. filter is specified for getting quotas:
		for packet based quota mask should be
NFACCT_F_QUOTA_PKTS and value - the same as mask,
		for byte based quota mask should be
NFACCT_F_QUOTA_BYTES and value - the same as mask.
	There is no support for listening/reseting quota of any available type
per one request. In case of NFACCT_F_QUOTA two list/reset request is necessary.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 include/uapi/linux/netfilter/nfnetlink_acct.h |   12 +++++
 net/netfilter/Kconfig                         |    9 ++++
 net/netfilter/nfnetlink_acct.c                |   61 +++++++++++++++++++++++++
 3 files changed, 82 insertions(+)

diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
index 51404ec..1683edb 100644
--- a/include/uapi/linux/netfilter/nfnetlink_acct.h
+++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
@@ -28,9 +28,21 @@ enum nfnl_acct_type {
 	NFACCT_USE,
 	NFACCT_FLAGS,
 	NFACCT_QUOTA,
+#ifdef CONFIG_NF_ACCT_FILTER
+	NFACCT_FILTER,
+#endif
 	__NFACCT_MAX
 };
 #define NFACCT_MAX (__NFACCT_MAX - 1)
 
+#ifdef CONFIG_NF_ACCT_FILTER
+enum nfnl_attr_filter_type {
+	NFACCT_FILTER_ATTR_UNSPEC,
+	NFACCT_FILTER_ATTR_MASK,
+	NFACCT_FILTER_ATTR_VALUE,
+	__NFACCT_FILTER_ATTR_MAX
+};
+#define NFACCT_FILTER_ATTR_MAX (__NFACCT_FILTER_ATTR_MAX - 1)
+#endif /* CONFIG_NF_ACCT_FILTER */
 
 #endif /* _UAPI_NFNL_ACCT_H_ */
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index ad751fe..8e3f882 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -12,6 +12,15 @@ tristate "Netfilter NFACCT over NFNETLINK interface"
 	  If this option is enabled, the kernel will include support
 	  for extended accounting via NFNETLINK.
 
+if NETFILTER_NETLINK_ACCT
+config NF_ACCT_FILTER
+	bool  'Filter support for nfacct netlink protocol'
+	depends on NETFILTER_ADVANCED
+	help
+	 If this option is enabled, nfacct framework will be able to list
+	 counters by filter.
+endif
+
 config NETFILTER_NETLINK_QUEUE
 	tristate "Netfilter NFQUEUE over NFNETLINK interface"
 	depends on NETFILTER_ADVANCED
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 3ea0eac..53293a4 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -40,6 +40,13 @@ struct nf_acct {
 	char			data[0];
 };
 
+#ifdef CONFIG_NF_ACCT_FILTER
+struct nfacct_filter {
+	u32 value;
+	u32 mask;
+};
+#endif
+
 #define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
 #define NFACCT_OVERQUOTA_BIT	2	/* NFACCT_F_OVERQUOTA */
 
@@ -181,6 +188,9 @@ static int
 nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct nf_acct *cur, *last;
+#ifdef CONFIG_NF_ACCT_FILTER
+	const struct nfacct_filter *filter = cb->data;
+#endif
 
 	if (cb->args[2])
 		return 0;
@@ -197,6 +207,12 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 			last = NULL;
 		}
+
+#ifdef CONFIG_NF_ACCT_FILTER
+		if (filter && (cur->flags & filter->mask) != filter->value)
+			continue;
+#endif
+
 		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
 				       cb->nlh->nlmsg_seq,
 				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
@@ -211,6 +227,44 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
+#ifdef CONFIG_NF_ACCT_FILTER
+static int
+nfnl_acct_done(struct netlink_callback *cb)
+{
+	if (cb->data)
+		kfree(cb->data);
+	return 0;
+}
+
+static const struct nla_policy filter_policy[NFACCT_FILTER_ATTR_MAX + 1] = {
+	[NFACCT_FILTER_ATTR_MASK]	= { .type = NLA_U32 },
+	[NFACCT_FILTER_ATTR_VALUE]	= { .type = NLA_U32 },
+};
+
+static struct nfacct_filter *
+init_filter(const struct nlattr * const nla)
+{
+	struct nfacct_filter *filter = NULL;
+	struct nlattr *attrs[NFACCT_FILTER_ATTR_MAX + 1];
+
+	if (!nla)
+		return NULL;
+
+	if (nla_parse_nested(attrs, NFACCT_FILTER_ATTR_MAX,
+			nla, filter_policy) != 0)
+		return NULL;
+
+	filter = kzalloc(sizeof(struct nfacct_filter), GFP_KERNEL);
+	if (!filter)
+		return NULL;
+
+	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_ATTR_MASK]);
+	filter->value = nla_get_be32(attrs[NFACCT_FILTER_ATTR_VALUE]);
+
+	return filter;
+}
+#endif /* CONFIG_NF_ACCT_FILTER */
+
 static int
 nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	     const struct nlmsghdr *nlh, const struct nlattr * const tb[])
@@ -220,9 +274,15 @@ nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	char *acct_name;
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
+		/* using filters only for dump/list operation */
 		struct netlink_dump_control c = {
 			.dump = nfnl_acct_dump,
 		};
+#ifdef CONFIG_NF_ACCT_FILTER
+		c.data = init_filter(tb[NFACCT_FILTER]);
+		c.done = nfnl_acct_done;
+#endif /* CONFIG_NF_ACCT_FILTER */
+
 		return netlink_dump_start(nfnl, skb, nlh, &c);
 	}
 
@@ -314,6 +374,7 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
 	[NFACCT_PKTS] = { .type = NLA_U64 },
 	[NFACCT_FLAGS] = { .type = NLA_U32 },
 	[NFACCT_QUOTA] = { .type = NLA_U64 },
+	[NFACCT_FILTER] = {.type = NLA_NESTED },
 };
 
 static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
-- 
1.7.9.5

Re: [PATCH] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Pablo Neira Ayuso]

On Mon, Aug 04, 2014 at 07:52:28PM +0400, Alexey Perevalov wrote:
> Filtering covers following cases:
> 	1. no filter specified. In this case client will get old
> behaviour
> 	2. filter is specified for getting just counters:
> 		in this case mask should be NFACCT_F_QUOTAS and value 0
> 	3. filter is specified for getting quotas:
> 		for packet based quota mask should be
> NFACCT_F_QUOTA_PKTS and value - the same as mask,
> 		for byte based quota mask should be
> NFACCT_F_QUOTA_BYTES and value - the same as mask.
> 	There is no support for listening/reseting quota of any available type
> per one request. In case of NFACCT_F_QUOTA two list/reset request is necessary.

If no filter is specified, any of the available types is reset. So no
need for the two requests.

> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> ---
>  include/uapi/linux/netfilter/nfnetlink_acct.h |   12 +++++
>  net/netfilter/Kconfig                         |    9 ++++
>  net/netfilter/nfnetlink_acct.c                |   61 +++++++++++++++++++++++++
>  3 files changed, 82 insertions(+)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
> index 51404ec..1683edb 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
> @@ -28,9 +28,21 @@ enum nfnl_acct_type {
>  	NFACCT_USE,
>  	NFACCT_FLAGS,
>  	NFACCT_QUOTA,
> +#ifdef CONFIG_NF_ACCT_FILTER

Please, remove the ifdef and the Kconfig switch.

> +	NFACCT_FILTER,
> +#endif
>  	__NFACCT_MAX
>  };
>  #define NFACCT_MAX (__NFACCT_MAX - 1)
>  
> +#ifdef CONFIG_NF_ACCT_FILTER
> +enum nfnl_attr_filter_type {
> +	NFACCT_FILTER_ATTR_UNSPEC,
> +	NFACCT_FILTER_ATTR_MASK,
> +	NFACCT_FILTER_ATTR_VALUE,
> +	__NFACCT_FILTER_ATTR_MAX
> +};
> +#define NFACCT_FILTER_ATTR_MAX (__NFACCT_FILTER_ATTR_MAX - 1)
> +#endif /* CONFIG_NF_ACCT_FILTER */
>  
>  #endif /* _UAPI_NFNL_ACCT_H_ */
> diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
> index ad751fe..8e3f882 100644
> --- a/net/netfilter/Kconfig
> +++ b/net/netfilter/Kconfig
> @@ -12,6 +12,15 @@ tristate "Netfilter NFACCT over NFNETLINK interface"
>  	  If this option is enabled, the kernel will include support
>  	  for extended accounting via NFNETLINK.
>  
> +if NETFILTER_NETLINK_ACCT
> +config NF_ACCT_FILTER
> +	bool  'Filter support for nfacct netlink protocol'
> +	depends on NETFILTER_ADVANCED
> +	help
> +	 If this option is enabled, nfacct framework will be able to list
> +	 counters by filter.
> +endif
> +
>  config NETFILTER_NETLINK_QUEUE
>  	tristate "Netfilter NFQUEUE over NFNETLINK interface"
>  	depends on NETFILTER_ADVANCED
> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
> index 3ea0eac..53293a4 100644
> --- a/net/netfilter/nfnetlink_acct.c
> +++ b/net/netfilter/nfnetlink_acct.c
> @@ -40,6 +40,13 @@ struct nf_acct {
>  	char			data[0];
>  };
>  
> +#ifdef CONFIG_NF_ACCT_FILTER
> +struct nfacct_filter {
> +	u32 value;
> +	u32 mask;
> +};
> +#endif
> +
>  #define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
>  #define NFACCT_OVERQUOTA_BIT	2	/* NFACCT_F_OVERQUOTA */
>  
> @@ -181,6 +188,9 @@ static int
>  nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  {
>  	struct nf_acct *cur, *last;
> +#ifdef CONFIG_NF_ACCT_FILTER
> +	const struct nfacct_filter *filter = cb->data;
> +#endif
>  
>  	if (cb->args[2])
>  		return 0;
> @@ -197,6 +207,12 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  
>  			last = NULL;
>  		}
> +
> +#ifdef CONFIG_NF_ACCT_FILTER
> +		if (filter && (cur->flags & filter->mask) != filter->value)
> +			continue;
> +#endif
> +
>  		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
>  				       cb->nlh->nlmsg_seq,
>  				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
> @@ -211,6 +227,44 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> +#ifdef CONFIG_NF_ACCT_FILTER
> +static int
> +nfnl_acct_done(struct netlink_callback *cb)
> +{
> +	if (cb->data)

You can remove this branch above, kfree already handles NULL pointers
for us.

Apart from those, this looks good to me. Thanks.

[PATCH v2] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Alexey Perevalov]

Filtering covers following cases:
	1. no filter specified. In this case client will get old
behaviour
	2. filter specified for getting only counters:
		in this case marks should be NFACCT_F_QUOTAS and value 0
	3. filter specified for getting quotas:
		for packet based quota mask should be
NFACCT_F_QUOTA_PKTS and value - the same,
		for byte based quota mask should be
NFACCT_F_QUOTA_BYTES and value - the same.
	Bit set for quota is't supported. NFACCT_F_QUOTA didn't expose
to user space program in public header as well. In case of NFACCT_F_QUOTA two
request is necessary, one for NFACCT_F_QUOTA_PKTS and another one for
NFACCT_F_QUOTA_BYTES.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 include/uapi/linux/netfilter/nfnetlink_acct.h |    8 ++++
 net/netfilter/nfnetlink_acct.c                |   50 +++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
index 51404ec..3ebc813 100644
--- a/include/uapi/linux/netfilter/nfnetlink_acct.h
+++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
@@ -28,9 +28,17 @@ enum nfnl_acct_type {
 	NFACCT_USE,
 	NFACCT_FLAGS,
 	NFACCT_QUOTA,
+	NFACCT_FILTER,
 	__NFACCT_MAX
 };
 #define NFACCT_MAX (__NFACCT_MAX - 1)
 
+enum nfnl_attr_filter_type {
+	NFACCT_FILTER_ATTR_UNSPEC,
+	NFACCT_FILTER_ATTR_MASK,
+	NFACCT_FILTER_ATTR_VALUE,
+	__NFACCT_FILTER_ATTR_MAX
+};
+#define NFACCT_FILTER_ATTR_MAX (__NFACCT_FILTER_ATTR_MAX - 1)
 
 #endif /* _UAPI_NFNL_ACCT_H_ */
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 3ea0eac..94a47c4 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -40,6 +40,11 @@ struct nf_acct {
 	char			data[0];
 };
 
+struct nfacct_filter {
+	u32 value;
+	u32 mask;
+};
+
 #define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
 #define NFACCT_OVERQUOTA_BIT	2	/* NFACCT_F_OVERQUOTA */
 
@@ -181,6 +186,7 @@ static int
 nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct nf_acct *cur, *last;
+	const struct nfacct_filter *filter = cb->data;
 
 	if (cb->args[2])
 		return 0;
@@ -197,6 +203,10 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 			last = NULL;
 		}
+
+		if (filter && (cur->flags & filter->mask) != filter->value)
+			continue;
+
 		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
 				       cb->nlh->nlmsg_seq,
 				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
@@ -212,6 +222,41 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 }
 
 static int
+nfnl_acct_done(struct netlink_callback *cb)
+{
+	kfree(cb->data);
+	return 0;
+}
+
+static const struct nla_policy filter_policy[NFACCT_FILTER_ATTR_MAX + 1] = {
+	[NFACCT_FILTER_ATTR_MASK]	= { .type = NLA_U32 },
+	[NFACCT_FILTER_ATTR_VALUE]	= { .type = NLA_U32 },
+};
+
+static struct nfacct_filter *
+init_filter(const struct nlattr * const nla)
+{
+	struct nfacct_filter *filter = NULL;
+	struct nlattr *attrs[NFACCT_FILTER_ATTR_MAX + 1];
+
+	if (!nla)
+		return NULL;
+
+	if (nla_parse_nested(attrs, NFACCT_FILTER_ATTR_MAX,
+			nla, filter_policy) != 0)
+		return NULL;
+
+	filter = kzalloc(sizeof(struct nfacct_filter), GFP_KERNEL);
+	if (!filter)
+		return NULL;
+
+	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_ATTR_MASK]);
+	filter->value = nla_get_be32(attrs[NFACCT_FILTER_ATTR_VALUE]);
+
+	return filter;
+}
+
+static int
 nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	     const struct nlmsghdr *nlh, const struct nlattr * const tb[])
 {
@@ -220,9 +265,13 @@ nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	char *acct_name;
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
+		/* using filters only for dump/list operation */
 		struct netlink_dump_control c = {
 			.dump = nfnl_acct_dump,
+			.done = nfnl_acct_done,
 		};
+		c.data = init_filter(tb[NFACCT_FILTER]);
+
 		return netlink_dump_start(nfnl, skb, nlh, &c);
 	}
 
@@ -314,6 +363,7 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
 	[NFACCT_PKTS] = { .type = NLA_U64 },
 	[NFACCT_FLAGS] = { .type = NLA_U32 },
 	[NFACCT_QUOTA] = { .type = NLA_U64 },
+	[NFACCT_FILTER] = {.type = NLA_NESTED },
 };
 
 static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
-- 
1.7.9.5

Re: [PATCH v2] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Pablo Neira Ayuso]

On Wed, Aug 06, 2014 at 02:41:02PM +0400, Alexey Perevalov wrote:
> +enum nfnl_attr_filter_type {
> +	NFACCT_FILTER_ATTR_UNSPEC,
> +	NFACCT_FILTER_ATTR_MASK,
> +	NFACCT_FILTER_ATTR_VALUE,
> +	__NFACCT_FILTER_ATTR_MAX
> +};

Minor nitpick: Could you remove the _ATTR so we get smaller name?

> +#define NFACCT_FILTER_ATTR_MAX (__NFACCT_FILTER_ATTR_MAX - 1)
>  
>  #endif /* _UAPI_NFNL_ACCT_H_ */
> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
> index 3ea0eac..94a47c4 100644
[...]
> +static struct nfacct_filter *
> +init_filter(const struct nlattr * const nla)

Rename this to nfacct_filter_alloc(), this fits in one line of
80-chars, no need to split it..

> +	struct nfacct_filter *filter = NULL;
> +	struct nlattr *attrs[NFACCT_FILTER_ATTR_MAX + 1];
> +
> +	if (!nla)
> +		return NULL;
> +
> +	if (nla_parse_nested(attrs, NFACCT_FILTER_ATTR_MAX,
> +			nla, filter_policy) != 0)
> +		return NULL;

        err = nla_parse_nested(...);
        if (err < 0)
                return ERR_PTR(err),

> +
> +	filter = kzalloc(sizeof(struct nfacct_filter), GFP_KERNEL);
> +	if (!filter)
> +		return NULL;

                return ERR_PTR(-ENOMEM);
> +
> +	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_ATTR_MASK]);
> +	filter->value = nla_get_be32(attrs[NFACCT_FILTER_ATTR_VALUE]);
> +
> +	return filter;
> +}
> +
> +static int
>  nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
>  	     const struct nlmsghdr *nlh, const struct nlattr * const tb[])
>  {
> @@ -220,9 +265,13 @@ nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
>  	char *acct_name;
>  
>  	if (nlh->nlmsg_flags & NLM_F_DUMP) {
> +		/* using filters only for dump/list operation */
>  		struct netlink_dump_control c = {
>  			.dump = nfnl_acct_dump,
> +			.done = nfnl_acct_done,
>  		};
> +		c.data = init_filter(tb[NFACCT_FILTER]);

I need better a bit error handling here, I suggest:

                if (tb[NFACCT_FILTER]) {
                        filter = nfacct_filter_alloc(tb[NFACCT_FILTER]);
                        if (IS_ERR(filter))
                                return PTR_ERR(filter);
                }

Currently, if we fail to allocate the filter, it silently ignores the
user request and it is not exactly doing what he requested.

Please, address this and resubmit. Thanks!

[[PATCH v3]] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Alexey Perevalov]

Filtering covers following cases:
	1. no filter specified. In this case client will get old
behaviour
	2. filter specified for getting only counters:
		in this case marks should be NFACCT_F_QUOTAS and value 0
	3. filter specified for getting quotas:
		for packet based quota mask should be
NFACCT_F_QUOTA_PKTS and value - the same,
		for byte based quota mask should be
NFACCT_F_QUOTA_BYTES and value - the same.
	Bit set for quota is't supported in this patch. NFACCT_F_QUOTA didn't
expose for user space program in public header as well. In case of
NFACCT_F_QUOTA two request is necessary, one for NFACCT_F_QUOTA_PKTS and another
one for NFACCT_F_QUOTA_BYTES.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 include/uapi/linux/netfilter/nfnetlink_acct.h |    8 ++++
 net/netfilter/nfnetlink_acct.c                |   51 +++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
index 51404ec..f3e34db 100644
--- a/include/uapi/linux/netfilter/nfnetlink_acct.h
+++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
@@ -28,9 +28,17 @@ enum nfnl_acct_type {
 	NFACCT_USE,
 	NFACCT_FLAGS,
 	NFACCT_QUOTA,
+	NFACCT_FILTER,
 	__NFACCT_MAX
 };
 #define NFACCT_MAX (__NFACCT_MAX - 1)
 
+enum nfnl_attr_filter_type {
+	NFACCT_FILTER_UNSPEC,
+	NFACCT_FILTER_MASK,
+	NFACCT_FILTER_VALUE,
+	__NFACCT_FILTER_MAX
+};
+#define NFACCT_FILTER_MAX (__NFACCT_FILTER_MAX - 1)
 
 #endif /* _UAPI_NFNL_ACCT_H_ */
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 3ea0eac..0f7159f 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -40,6 +40,11 @@ struct nf_acct {
 	char			data[0];
 };
 
+struct nfacct_filter {
+	u32 value;
+	u32 mask;
+};
+
 #define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
 #define NFACCT_OVERQUOTA_BIT	2	/* NFACCT_F_OVERQUOTA */
 
@@ -181,6 +186,7 @@ static int
 nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct nf_acct *cur, *last;
+	const struct nfacct_filter *filter = cb->data;
 
 	if (cb->args[2])
 		return 0;
@@ -197,6 +203,10 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 			last = NULL;
 		}
+
+		if (filter && (cur->flags & filter->mask) != filter->value)
+			continue;
+
 		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
 				       cb->nlh->nlmsg_seq,
 				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
@@ -212,6 +222,32 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 }
 
 static int
+nfnl_acct_done(struct netlink_callback *cb)
+{
+	kfree(cb->data);
+	return 0;
+}
+
+static const struct nla_policy filter_policy[NFACCT_FILTER_MAX + 1] = {
+	[NFACCT_FILTER_MASK]	= { .type = NLA_U32 },
+	[NFACCT_FILTER_VALUE]	= { .type = NLA_U32 },
+};
+
+static struct nfacct_filter *
+nfacct_filter_alloc(struct nlattr *attrs[NFACCT_FILTER_MAX + 1])
+{
+	struct nfacct_filter *filter = kzalloc(sizeof(struct nfacct_filter),
+					       GFP_KERNEL);
+	if (!filter)
+		return ERR_PTR(-ENOMEM);
+
+	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_MASK]);
+	filter->value = nla_get_be32(attrs[NFACCT_FILTER_VALUE]);
+
+	return filter;
+}
+
+static int
 nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	     const struct nlmsghdr *nlh, const struct nlattr * const tb[])
 {
@@ -222,7 +258,21 @@ nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = nfnl_acct_dump,
+			.done = nfnl_acct_done,
 		};
+		if (tb[NFACCT_FILTER]) {
+			struct nlattr *attrs[NFACCT_FILTER_MAX + 1];
+			ret = nla_parse_nested(attrs, NFACCT_FILTER_MAX,
+					       tb[NFACCT_FILTER],
+					       filter_policy);
+			if (ret < 0)
+				return ret;
+
+			c.data = nfacct_filter_alloc(attrs);
+			if (IS_ERR(c.data))
+				return PTR_ERR(c.data);
+		}
+
 		return netlink_dump_start(nfnl, skb, nlh, &c);
 	}
 
@@ -314,6 +364,7 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
 	[NFACCT_PKTS] = { .type = NLA_U64 },
 	[NFACCT_FLAGS] = { .type = NLA_U32 },
 	[NFACCT_QUOTA] = { .type = NLA_U64 },
+	[NFACCT_FILTER] = {.type = NLA_NESTED },
 };
 
 static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
-- 
1.7.9.5

Re: [[PATCH v3]] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

Hi,

On Wed, Aug 20, 2014 at 10:03:18PM +0400, Alexey Perevalov wrote:
> +static struct nfacct_filter *
> +nfacct_filter_alloc(struct nlattr *attrs[NFACCT_FILTER_MAX + 1])
> +{
> +	struct nfacct_filter *filter = kzalloc(sizeof(struct nfacct_filter),
> +					       GFP_KERNEL);
> +	if (!filter)
> +		return ERR_PTR(-ENOMEM);
> +
> +	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_MASK]);
> +	filter->value = nla_get_be32(attrs[NFACCT_FILTER_VALUE]);

We have to use ntohl() here, it's the convention for nfnetlink.

I'm attaching a patch that resolves this plus some cleanups.

Please, let me know if you're OK with it. Thanks.

[-- Attachment #2: 0001-netfilter-nfnetlink_acct-add-filter-support-to-count.patch --]
[-- Type: text/x-diff, Size: 4776 bytes --]

>From eff91543e49c2acb2844b263b59ea0b7cc94024d Mon Sep 17 00:00:00 2001
From: Alexey Perevalov <a.perevalov@samsung.com>
Date: Wed, 20 Aug 2014 22:03:18 +0400
Subject: [PATCH] netfilter: nfnetlink_acct: add filter support to counter
 list/reset

You can use this to skip accounting objects when listing/resetting
via NFNL_MSG_ACCT_GET/NFNL_MSG_ACCT_GET_CTRZERO messages with the
NLM_F_DUMP netlink flag. The filtering covers the following cases:

1. No filter specified. In this case, the client will get old behaviour,
2. List/reset counter object only: In this case, you have to use
   NFACCT_F_QUOTA as mask and value 0.
3. List/reset quota objects only: You have to use NFACCT_F_QUOTA_PKTS
   as mask and value - the same, for byte based quota mask should be
   NFACCT_F_QUOTA_BYTES and value - the same.

If you want to obtain the object with any quota type
(ie. NFACCT_F_QUOTA_PKTS|NFACCT_F_QUOTA_BYTES), you need to perform
two dump requests, one to obtain NFACCT_F_QUOTA_PKTS objects and
another for NFACCT_F_QUOTA_BYTES.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/uapi/linux/netfilter/nfnetlink_acct.h |    8 ++++
 net/netfilter/nfnetlink_acct.c                |   54 +++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
index 51404ec..f3e34db 100644
--- a/include/uapi/linux/netfilter/nfnetlink_acct.h
+++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
@@ -28,9 +28,17 @@ enum nfnl_acct_type {
 	NFACCT_USE,
 	NFACCT_FLAGS,
 	NFACCT_QUOTA,
+	NFACCT_FILTER,
 	__NFACCT_MAX
 };
 #define NFACCT_MAX (__NFACCT_MAX - 1)
 
+enum nfnl_attr_filter_type {
+	NFACCT_FILTER_UNSPEC,
+	NFACCT_FILTER_MASK,
+	NFACCT_FILTER_VALUE,
+	__NFACCT_FILTER_MAX
+};
+#define NFACCT_FILTER_MAX (__NFACCT_FILTER_MAX - 1)
 
 #endif /* _UAPI_NFNL_ACCT_H_ */
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 3ea0eac..c18af2f 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -40,6 +40,11 @@ struct nf_acct {
 	char			data[0];
 };
 
+struct nfacct_filter {
+	u32 value;
+	u32 mask;
+};
+
 #define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
 #define NFACCT_OVERQUOTA_BIT	2	/* NFACCT_F_OVERQUOTA */
 
@@ -181,6 +186,7 @@ static int
 nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct nf_acct *cur, *last;
+	const struct nfacct_filter *filter = cb->data;
 
 	if (cb->args[2])
 		return 0;
@@ -197,6 +203,10 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 			last = NULL;
 		}
+
+		if (filter && (cur->flags & filter->mask) != filter->value)
+			continue;
+
 		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
 				       cb->nlh->nlmsg_seq,
 				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
@@ -211,6 +221,38 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
+static int nfnl_acct_done(struct netlink_callback *cb)
+{
+	kfree(cb->data);
+	return 0;
+}
+
+static const struct nla_policy filter_policy[NFACCT_FILTER_MAX + 1] = {
+	[NFACCT_FILTER_MASK]	= { .type = NLA_U32 },
+	[NFACCT_FILTER_VALUE]	= { .type = NLA_U32 },
+};
+
+static struct nfacct_filter *
+nfacct_filter_alloc(const struct nlattr * const attr)
+{
+	struct nfacct_filter *filter;
+	struct nlattr *tb[NFACCT_FILTER_MAX + 1];
+	int err;
+
+	err = nla_parse_nested(tb, NFACCT_FILTER_MAX, attr, filter_policy);
+	if (err < 0)
+		return ERR_PTR(err);
+
+	filter = kzalloc(sizeof(struct nfacct_filter), GFP_KERNEL);
+	if (!filter)
+		return ERR_PTR(-ENOMEM);
+
+	filter->mask = ntohl(nla_get_be32(tb[NFACCT_FILTER_MASK]));
+	filter->value = ntohl(nla_get_be32(tb[NFACCT_FILTER_VALUE]));
+
+	return filter;
+}
+
 static int
 nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	     const struct nlmsghdr *nlh, const struct nlattr * const tb[])
@@ -222,7 +264,18 @@ nfnl_acct_get(struct sock *nfnl, struct sk_buff *skb,
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		struct netlink_dump_control c = {
 			.dump = nfnl_acct_dump,
+			.done = nfnl_acct_done,
 		};
+
+		if (tb[NFACCT_FILTER]) {
+			struct nfacct_filter *filter;
+
+			filter = nfacct_filter_alloc(tb[NFACCT_FILTER]);
+			if (IS_ERR(filter))
+				return PTR_ERR(filter);
+
+			c.data = filter;
+		}
 		return netlink_dump_start(nfnl, skb, nlh, &c);
 	}
 
@@ -314,6 +367,7 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
 	[NFACCT_PKTS] = { .type = NLA_U64 },
 	[NFACCT_FLAGS] = { .type = NLA_U32 },
 	[NFACCT_QUOTA] = { .type = NLA_U64 },
+	[NFACCT_FILTER] = {.type = NLA_NESTED },
 };
 
 static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
-- 
1.7.10.4

Re: [[PATCH v3]] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Alexey Perevalov]

Hi
On 08/24/2014 05:15 PM, Pablo Neira Ayuso wrote:
> Hi,
>
> On Wed, Aug 20, 2014 at 10:03:18PM +0400, Alexey Perevalov wrote:
>> +static struct nfacct_filter *
>> +nfacct_filter_alloc(struct nlattr *attrs[NFACCT_FILTER_MAX + 1])
>> +{
>> +	struct nfacct_filter *filter = kzalloc(sizeof(struct nfacct_filter),
>> +					       GFP_KERNEL);
>> +	if (!filter)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_MASK]);
>> +	filter->value = nla_get_be32(attrs[NFACCT_FILTER_VALUE]);
> We have to use ntohl() here, it's the convention for nfnetlink.
>
> I'm attaching a patch that resolves this plus some cleanups.
>
> Please, let me know if you're OK with it. Thanks.
Yes, I'm ok with it.

sorry for double braces )

BR,
Alexey

Re: [[PATCH v3]] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Pablo Neira Ayuso]

On Tue, Aug 26, 2014 at 11:15:04PM +0400, Alexey Perevalov wrote:
> Hi
> On 08/24/2014 05:15 PM, Pablo Neira Ayuso wrote:
> >Hi,
> >
> >On Wed, Aug 20, 2014 at 10:03:18PM +0400, Alexey Perevalov wrote:
> >>+static struct nfacct_filter *
> >>+nfacct_filter_alloc(struct nlattr *attrs[NFACCT_FILTER_MAX + 1])
> >>+{
> >>+	struct nfacct_filter *filter = kzalloc(sizeof(struct nfacct_filter),
> >>+					       GFP_KERNEL);
> >>+	if (!filter)
> >>+		return ERR_PTR(-ENOMEM);
> >>+
> >>+	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_MASK]);
> >>+	filter->value = nla_get_be32(attrs[NFACCT_FILTER_VALUE]);
> >We have to use ntohl() here, it's the convention for nfnetlink.
> >
> >I'm attaching a patch that resolves this plus some cleanups.
> >
> >Please, let me know if you're OK with it. Thanks.
>
> Yes, I'm ok with it.

Applied, thanks.

Re: [[PATCH v3]] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Alexey Perevalov]

Hi,

On 08/24/2014 05:15 PM, Pablo Neira Ayuso wrote:
> Hi,
>
> On Wed, Aug 20, 2014 at 10:03:18PM +0400, Alexey Perevalov wrote:
>> +static struct nfacct_filter *
>> +nfacct_filter_alloc(struct nlattr *attrs[NFACCT_FILTER_MAX + 1])
>> +{
>> +	struct nfacct_filter *filter = kzalloc(sizeof(struct nfacct_filter),
>> +					       GFP_KERNEL);
>> +	if (!filter)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	filter->mask = nla_get_be32(attrs[NFACCT_FILTER_MASK]);
>> +	filter->value = nla_get_be32(attrs[NFACCT_FILTER_VALUE]);
> We have to use ntohl() here, it's the convention for nfnetlink.
>
> I'm attaching a patch that resolves this plus some cleanups.
>
> Please, let me know if you're OK with it. Thanks.
Yes, I'm ok with it.

sorry for double braces )
and thanks  for correction.

BR,
Alexey

Re: [PATCH] netfilter: nfnetlink_acct: add filter support to nfacct counter list/reset

[Alexey Perevalov]

On 08/05/2014 07:51 PM, Pablo Neira Ayuso wrote:
> On Mon, Aug 04, 2014 at 07:52:28PM +0400, Alexey Perevalov wrote:
>> Filtering covers following cases:
>> 	1. no filter specified. In this case client will get old
>> behaviour
>> 	2. filter is specified for getting just counters:
>> 		in this case mask should be NFACCT_F_QUOTAS and value 0
>> 	3. filter is specified for getting quotas:
>> 		for packet based quota mask should be
>> NFACCT_F_QUOTA_PKTS and value - the same as mask,
>> 		for byte based quota mask should be
>> NFACCT_F_QUOTA_BYTES and value - the same as mask.
>> 	There is no support for listening/reseting quota of any available type
>> per one request. In case of NFACCT_F_QUOTA two list/reset request is necessary.
> If no filter is specified, any of the available types is reset. So no
> need for the two requests.
I think my sentence wasn't clear enough. I meant
if user sent a NFACCT_F_QUOTA as a mask, there is no
way with current condition to get all types of quotas together.


>
>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>> ---
>>   include/uapi/linux/netfilter/nfnetlink_acct.h |   12 +++++
>>   net/netfilter/Kconfig                         |    9 ++++
>>   net/netfilter/nfnetlink_acct.c                |   61 +++++++++++++++++++++++++
>>   3 files changed, 82 insertions(+)
>>
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> index 51404ec..1683edb 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> @@ -28,9 +28,21 @@ enum nfnl_acct_type {
>>   	NFACCT_USE,
>>   	NFACCT_FLAGS,
>>   	NFACCT_QUOTA,
>> +#ifdef CONFIG_NF_ACCT_FILTER
> Please, remove the ifdef and the Kconfig switch.
>
>> +	NFACCT_FILTER,
>> +#endif
>>   	__NFACCT_MAX
>>   };
>>   #define NFACCT_MAX (__NFACCT_MAX - 1)
>>   
>> +#ifdef CONFIG_NF_ACCT_FILTER
>> +enum nfnl_attr_filter_type {
>> +	NFACCT_FILTER_ATTR_UNSPEC,
>> +	NFACCT_FILTER_ATTR_MASK,
>> +	NFACCT_FILTER_ATTR_VALUE,
>> +	__NFACCT_FILTER_ATTR_MAX
>> +};
>> +#define NFACCT_FILTER_ATTR_MAX (__NFACCT_FILTER_ATTR_MAX - 1)
>> +#endif /* CONFIG_NF_ACCT_FILTER */
>>   
>>   #endif /* _UAPI_NFNL_ACCT_H_ */
>> diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
>> index ad751fe..8e3f882 100644
>> --- a/net/netfilter/Kconfig
>> +++ b/net/netfilter/Kconfig
>> @@ -12,6 +12,15 @@ tristate "Netfilter NFACCT over NFNETLINK interface"
>>   	  If this option is enabled, the kernel will include support
>>   	  for extended accounting via NFNETLINK.
>>   
>> +if NETFILTER_NETLINK_ACCT
>> +config NF_ACCT_FILTER
>> +	bool  'Filter support for nfacct netlink protocol'
>> +	depends on NETFILTER_ADVANCED
>> +	help
>> +	 If this option is enabled, nfacct framework will be able to list
>> +	 counters by filter.
>> +endif
>> +
>>   config NETFILTER_NETLINK_QUEUE
>>   	tristate "Netfilter NFQUEUE over NFNETLINK interface"
>>   	depends on NETFILTER_ADVANCED
>> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
>> index 3ea0eac..53293a4 100644
>> --- a/net/netfilter/nfnetlink_acct.c
>> +++ b/net/netfilter/nfnetlink_acct.c
>> @@ -40,6 +40,13 @@ struct nf_acct {
>>   	char			data[0];
>>   };
>>   
>> +#ifdef CONFIG_NF_ACCT_FILTER
>> +struct nfacct_filter {
>> +	u32 value;
>> +	u32 mask;
>> +};
>> +#endif
>> +
>>   #define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
>>   #define NFACCT_OVERQUOTA_BIT	2	/* NFACCT_F_OVERQUOTA */
>>   
>> @@ -181,6 +188,9 @@ static int
>>   nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>   {
>>   	struct nf_acct *cur, *last;
>> +#ifdef CONFIG_NF_ACCT_FILTER
>> +	const struct nfacct_filter *filter = cb->data;
>> +#endif
>>   
>>   	if (cb->args[2])
>>   		return 0;
>> @@ -197,6 +207,12 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>   
>>   			last = NULL;
>>   		}
>> +
>> +#ifdef CONFIG_NF_ACCT_FILTER
>> +		if (filter && (cur->flags & filter->mask) != filter->value)
>> +			continue;
>> +#endif
>> +
>>   		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
>>   				       cb->nlh->nlmsg_seq,
>>   				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
>> @@ -211,6 +227,44 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>   	return skb->len;
>>   }
>>   
>> +#ifdef CONFIG_NF_ACCT_FILTER
>> +static int
>> +nfnl_acct_done(struct netlink_callback *cb)
>> +{
>> +	if (cb->data)
> You can remove this branch above, kfree already handles NULL pointers
> for us.
>
> Apart from those, this looks good to me. Thanks.
>


-- 
Best regards,
Alexey Perevalov