[PATCH 0/2] fixes for NFACCT_F_OVERQUOTA usage

[PATCH 0/2] fixes for NFACCT_F_OVERQUOTA usage

[Alexey Perevalov]

Hello Pablo,

I chose straightforward approach and I have used bitwise operators instead of
bit helper functions.

Also I found some inconsistency in report after reset for overquota.

This patch set was based on commit 5a7439efd1c5c416f768fc550048ca130cf4bf99
(dated Jul 25 20:08:13 2014 - little bit updated :)

Alexey Perevalov (2):
  netfilter: nfnetlink_acct: avoid using NFACCT_F_OVERQUOTA with bit
    helper funcitons
  netfilter: nfnetlink_acct: dump unmodified nfacct flags

 net/netfilter/nfnetlink_acct.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] netfilter: nfnetlink_acct: avoid using NFACCT_F_OVERQUOTA with bit helper funcitons

[Alexey Perevalov]

Bit helper functions were used for manipulation with NFACCT_F_OVERQUOTA,
but they are accepting pit position, but not a bit mask. As a result
not a third bit for NFACCT_F_OVERQUOTA was set, but forth. Such
behaviour was dangarous and could lead to unexpected overquota report
result.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 net/netfilter/nfnetlink_acct.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 2baa125..127d24e 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -77,7 +77,7 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
 			smp_mb__before_atomic();
 			/* reset overquota flag if quota is enabled. */
 			if ((matching->flags & NFACCT_F_QUOTA))
-				clear_bit(NFACCT_F_OVERQUOTA, &matching->flags);
+				matching->flags &= ~NFACCT_F_OVERQUOTA;
 			return 0;
 		}
 		return -EBUSY;
@@ -148,7 +148,7 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 		bytes = atomic64_xchg(&acct->bytes, 0);
 		smp_mb__before_atomic();
 		if (acct->flags & NFACCT_F_QUOTA)
-			clear_bit(NFACCT_F_OVERQUOTA, &acct->flags);
+			acct->flags &= ~NFACCT_F_OVERQUOTA;
 	} else {
 		pkts = atomic64_read(&acct->pkts);
 		bytes = atomic64_read(&acct->bytes);
@@ -411,8 +411,8 @@ int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
 
 	ret = now > *quota;
 
-	if (now >= *quota &&
-	    !test_and_set_bit(NFACCT_F_OVERQUOTA, &nfacct->flags)) {
+	if (now >= *quota) {
+		nfacct->flags |= NFACCT_F_OVERQUOTA;
 		nfnl_overquota_report(nfacct);
 	}
 
-- 
1.7.9.5

[PATCH 2/2] netfilter: nfnetlink_acct: dump unmodified nfacct flags

[Alexey Perevalov]

NFNL_MSG_ACCT_GET_CTRZERO modifies dumped flags, in this case
client see unmodified (uncleared) counter value and cleared
overquota state - end user doesn't know anything about overquota state,
unless end user subscribed on overquota report.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 net/netfilter/nfnetlink_acct.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 127d24e..ab2e89d 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -129,6 +129,7 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	struct nfgenmsg *nfmsg;
 	unsigned int flags = portid ? NLM_F_MULTI : 0;
 	u64 pkts, bytes;
+	u32 old_flags;
 
 	event |= NFNL_SUBSYS_ACCT << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*nfmsg), flags);
@@ -143,6 +144,7 @@ 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;
 
+	old_flags = acct->flags;
 	if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
 		pkts = atomic64_xchg(&acct->pkts, 0);
 		bytes = atomic64_xchg(&acct->bytes, 0);
@@ -160,7 +162,7 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	if (acct->flags & NFACCT_F_QUOTA) {
 		u64 *quota = (u64 *)acct->data;
 
-		if (nla_put_be32(skb, NFACCT_FLAGS, htonl(acct->flags)) ||
+		if (nla_put_be32(skb, NFACCT_FLAGS, htonl(old_flags)) ||
 		    nla_put_be64(skb, NFACCT_QUOTA, cpu_to_be64(*quota)))
 			goto nla_put_failure;
 	}
-- 
1.7.9.5

Re: [PATCH 2/2] netfilter: nfnetlink_acct: dump unmodified nfacct flags

[Pablo Neira Ayuso]

On Wed, Jul 30, 2014 at 07:17:55PM +0400, Alexey Perevalov wrote:
> NFNL_MSG_ACCT_GET_CTRZERO modifies dumped flags, in this case
> client see unmodified (uncleared) counter value and cleared
> overquota state - end user doesn't know anything about overquota state,
> unless end user subscribed on overquota report.

Indeed, the old flags are relevant information for userspace. Applied,
thanks Alexey.

Re: [PATCH 1/2] netfilter: nfnetlink_acct: avoid using NFACCT_F_OVERQUOTA with bit helper funcitons

[Pablo Neira Ayuso]

On Wed, Jul 30, 2014 at 07:17:54PM +0400, Alexey Perevalov wrote:
> Bit helper functions were used for manipulation with NFACCT_F_OVERQUOTA,
> but they are accepting pit position, but not a bit mask. As a result
> not a third bit for NFACCT_F_OVERQUOTA was set, but forth. Such
> behaviour was dangarous and could lead to unexpected overquota report
> result.
> 
> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> ---
>  net/netfilter/nfnetlink_acct.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
> index 2baa125..127d24e 100644
> --- a/net/netfilter/nfnetlink_acct.c
> +++ b/net/netfilter/nfnetlink_acct.c
> @@ -77,7 +77,7 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>  			smp_mb__before_atomic();
>  			/* reset overquota flag if quota is enabled. */
>  			if ((matching->flags & NFACCT_F_QUOTA))
> -				clear_bit(NFACCT_F_OVERQUOTA, &matching->flags);
> +				matching->flags &= ~NFACCT_F_OVERQUOTA;
>  			return 0;
>  		}
>  		return -EBUSY;
> @@ -148,7 +148,7 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>  		bytes = atomic64_xchg(&acct->bytes, 0);
>  		smp_mb__before_atomic();
>  		if (acct->flags & NFACCT_F_QUOTA)
> -			clear_bit(NFACCT_F_OVERQUOTA, &acct->flags);
> +			acct->flags &= ~NFACCT_F_OVERQUOTA;
>  	} else {
>  		pkts = atomic64_read(&acct->pkts);
>  		bytes = atomic64_read(&acct->bytes);
> @@ -411,8 +411,8 @@ int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
>  
>  	ret = now > *quota;
>  
> -	if (now >= *quota &&
> -	    !test_and_set_bit(NFACCT_F_OVERQUOTA, &nfacct->flags)) {

We cannot do this. The aim of that code it to avoid to deliver several
overquota notification when several cpus are racing to update the
counters and check if they have go over the quota.

I think you have to define NFACCT_F_*_BIT and use it from the bitwise
functions to fix this. You can define these in:

include/uapi/linux/netfilter/nfnetlink_acct.h

And use them to define the enum nfnl_acct_flags.

Thanks.

[PATCH V2] netfilter: nfnetlink_acct: avoid using NFACCT_F_OVERQUOTA with bit helper funcitons

[Alexey Perevalov]

Bit helper functions were used for manipulation with NFACCT_F_OVERQUOTA,
but they are accepting pit position, but not a bit mask. As a result
not a third bit for NFACCT_F_OVERQUOTA was set, but forth. Such
behaviour was dangarous and could lead to unexpected overquota report
result.

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

diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
index 51404ec..81410b7 100644
--- a/include/uapi/linux/netfilter/nfnetlink_acct.h
+++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
@@ -14,10 +14,13 @@ enum nfnl_acct_msg_types {
 	NFNL_MSG_ACCT_MAX
 };
 
+#define NFACCT_OVERQUOTA_BIT 2
+
 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_OVERQUOTA	= (1 << NFACCT_OVERQUOTA_BIT), /* can't be
+		 set from userspace */
 };
 
 enum nfnl_acct_type {
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 11d863c..5f6b1fb 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -77,7 +77,8 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
 			smp_mb__before_atomic();
 			/* reset overquota flag if quota is enabled. */
 			if ((matching->flags & NFACCT_F_QUOTA))
-				clear_bit(NFACCT_F_OVERQUOTA, &matching->flags);
+				clear_bit(NFACCT_OVERQUOTA_BIT,
+					   &matching->flags);
 			return 0;
 		}
 		return -EBUSY;
@@ -150,7 +151,7 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 		bytes = atomic64_xchg(&acct->bytes, 0);
 		smp_mb__before_atomic();
 		if (acct->flags & NFACCT_F_QUOTA)
-			clear_bit(NFACCT_F_OVERQUOTA, &acct->flags);
+			clear_bit(NFACCT_OVERQUOTA_BIT, &acct->flags);
 	} else {
 		pkts = atomic64_read(&acct->pkts);
 		bytes = atomic64_read(&acct->bytes);
@@ -414,7 +415,7 @@ int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
 	ret = now > *quota;
 
 	if (now >= *quota &&
-	    !test_and_set_bit(NFACCT_F_OVERQUOTA, &nfacct->flags)) {
+	    !test_and_set_bit(NFACCT_OVERQUOTA_BIT, &nfacct->flags)) {
 		nfnl_overquota_report(nfacct);
 	}
 
-- 
1.7.9.5

Re: [PATCH V2] netfilter: nfnetlink_acct: avoid using NFACCT_F_OVERQUOTA with bit helper funcitons

[Pablo Neira Ayuso]

On Thu, Jul 31, 2014 at 05:14:05PM +0400, Alexey Perevalov wrote:
> Bit helper functions were used for manipulation with NFACCT_F_OVERQUOTA,
> but they are accepting pit position, but not a bit mask. As a result
> not a third bit for NFACCT_F_OVERQUOTA was set, but forth. Such
> behaviour was dangarous and could lead to unexpected overquota report
> result.

Applied, thanks.

I have made a minor change.

> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> ---
>  include/uapi/linux/netfilter/nfnetlink_acct.h |    5 ++++-
>  net/netfilter/nfnetlink_acct.c                |    7 ++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
> index 51404ec..81410b7 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
> @@ -14,10 +14,13 @@ enum nfnl_acct_msg_types {
>  	NFNL_MSG_ACCT_MAX
>  };
>  
> +#define NFACCT_OVERQUOTA_BIT 2
> +
>  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_OVERQUOTA	= (1 << NFACCT_OVERQUOTA_BIT), /* can't be
> +		 set from userspace */
>  };

I know I asked for NFACCT_OVERQUOTA_BIT to be included here, bit after
seeing the patch it's obvious it doesn't make sense to expose this to
userspace, so I have mangled the patch to define this in
nfnetlink_acct.