All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework
@ 2014-04-01 22:52 mathieu.poirier
  2014-04-01 22:52 ` [PATCH 2/2] Extend accounting capabilities to support quotas mathieu.poirier
  2014-04-07 15:20 ` [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework Pablo Neira Ayuso
  0 siblings, 2 replies; 4+ messages in thread
From: mathieu.poirier @ 2014-04-01 22:52 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel, netfilter, mathieu.poirier

From: Mathieu Poirier <mathieu.poirier@linaro.org>

Nf_acct objects already support accounting at the byte and packet
level.  As such it is a natural extention to add the possiblity to
define a ceiling limit for both metrics.

All the support for quotas itself is added to nfnetlink acctounting
framework to stay coherent with current accounting object management.
Quota limit checks are implemented in xt nfacct filter where
statistic collection is already done.

Pablo Niera Ayuso has also contributed to this feature.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/linux/netfilter/nfnetlink_acct.h      |  8 ++-
 include/uapi/linux/netfilter/nfnetlink.h      |  2 +
 include/uapi/linux/netfilter/nfnetlink_acct.h |  9 +++
 net/netfilter/nfnetlink_acct.c                | 86 +++++++++++++++++++++++++++
 net/netfilter/xt_nfacct.c                     |  6 ++
 5 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/include/linux/netfilter/nfnetlink_acct.h b/include/linux/netfilter/nfnetlink_acct.h
index b2e85e5..6ec9757 100644
--- a/include/linux/netfilter/nfnetlink_acct.h
+++ b/include/linux/netfilter/nfnetlink_acct.h
@@ -3,11 +3,17 @@
 
 #include <uapi/linux/netfilter/nfnetlink_acct.h>
 
+enum {
+	NFACCT_NO_QUOTA		= -1,
+	NFACCT_UNDERQUOTA,
+	NFACCT_OVERQUOTA,
+};
 
 struct nf_acct;
 
 struct nf_acct *nfnl_acct_find_get(const char *filter_name);
 void nfnl_acct_put(struct nf_acct *acct);
 void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct);
-
+extern int nfnl_acct_overquota(const struct sk_buff *skb,
+			      struct nf_acct *nfacct);
 #endif /* _NFNL_ACCT_H */
diff --git a/include/uapi/linux/netfilter/nfnetlink.h b/include/uapi/linux/netfilter/nfnetlink.h
index 596ddd4..354a7e5 100644
--- a/include/uapi/linux/netfilter/nfnetlink.h
+++ b/include/uapi/linux/netfilter/nfnetlink.h
@@ -20,6 +20,8 @@ enum nfnetlink_groups {
 #define NFNLGRP_CONNTRACK_EXP_DESTROY	NFNLGRP_CONNTRACK_EXP_DESTROY
 	NFNLGRP_NFTABLES,
 #define NFNLGRP_NFTABLES                NFNLGRP_NFTABLES
+	NFNLGRP_ACCT_QUOTA,
+#define NFNLGRP_ACCT_QUOTA		NFNLGRP_ACCT_QUOTA
 	__NFNLGRP_MAX,
 };
 #define NFNLGRP_MAX	(__NFNLGRP_MAX - 1)
diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
index c7b6269..51404ec 100644
--- a/include/uapi/linux/netfilter/nfnetlink_acct.h
+++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
@@ -10,15 +10,24 @@ enum nfnl_acct_msg_types {
 	NFNL_MSG_ACCT_GET,
 	NFNL_MSG_ACCT_GET_CTRZERO,
 	NFNL_MSG_ACCT_DEL,
+	NFNL_MSG_ACCT_OVERQUOTA,
 	NFNL_MSG_ACCT_MAX
 };
 
+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 */
+};
+
 enum nfnl_acct_type {
 	NFACCT_UNSPEC,
 	NFACCT_NAME,
 	NFACCT_PKTS,
 	NFACCT_BYTES,
 	NFACCT_USE,
+	NFACCT_FLAGS,
+	NFACCT_QUOTA,
 	__NFACCT_MAX
 };
 #define NFACCT_MAX (__NFACCT_MAX - 1)
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index c7b6d46..dae35eb 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -32,18 +32,24 @@ static LIST_HEAD(nfnl_acct_list);
 struct nf_acct {
 	atomic64_t		pkts;
 	atomic64_t		bytes;
+	unsigned long		flags;
 	struct list_head	head;
 	atomic_t		refcnt;
 	char			name[NFACCT_NAME_MAX];
 	struct rcu_head		rcu_head;
+	char			data[0];
 };
 
+#define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
+
 static int
 nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
 	     const struct nlmsghdr *nlh, const struct nlattr * const tb[])
 {
 	struct nf_acct *nfacct, *matching = NULL;
 	char *acct_name;
+	unsigned int size = 0;
+	u32 flags = 0;
 
 	if (!tb[NFACCT_NAME])
 		return -EINVAL;
@@ -68,15 +74,39 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
 			/* reset counters if you request a replacement. */
 			atomic64_set(&matching->pkts, 0);
 			atomic64_set(&matching->bytes, 0);
+			smp_mb__before_clear_bit();
+			/* reset overquota flag if quota is enabled. */
+			if ((matching->flags & NFACCT_F_QUOTA))
+				clear_bit(NFACCT_F_OVERQUOTA, &matching->flags);
 			return 0;
 		}
 		return -EBUSY;
 	}
 
 	nfacct = kzalloc(sizeof(struct nf_acct), GFP_KERNEL);
+	if (tb[NFACCT_FLAGS]) {
+		flags = ntohl(nla_get_be32(tb[NFACCT_FLAGS]));
+		if (flags & ~NFACCT_F_QUOTA)
+			return -EOPNOTSUPP;
+		if ((flags & NFACCT_F_QUOTA) == NFACCT_F_QUOTA)
+			return -EINVAL;
+		if (flags & NFACCT_F_OVERQUOTA)
+			return -EINVAL;
+
+		size += sizeof(u64);
+	}
+
+	nfacct = kzalloc(sizeof(struct nf_acct) + size, GFP_KERNEL);
 	if (nfacct == NULL)
 		return -ENOMEM;
 
+	if (flags & NFACCT_F_QUOTA) {
+		u64 *quota = (u64 *)nfacct->data;
+
+		*quota = be64_to_cpu(nla_get_be64(tb[NFACCT_QUOTA]));
+		nfacct->flags = flags;
+	}
+
 	strncpy(nfacct->name, nla_data(tb[NFACCT_NAME]), NFACCT_NAME_MAX);
 
 	if (tb[NFACCT_BYTES]) {
@@ -117,6 +147,10 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
 		pkts = atomic64_xchg(&acct->pkts, 0);
 		bytes = atomic64_xchg(&acct->bytes, 0);
+		if (acct->flags & NFACCT_F_QUOTA) {
+			smp_mb__before_clear_bit();
+			clear_bit(NFACCT_F_OVERQUOTA, &acct->flags);
+		}
 	} else {
 		pkts = atomic64_read(&acct->pkts);
 		bytes = atomic64_read(&acct->bytes);
@@ -125,7 +159,13 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	    nla_put_be64(skb, NFACCT_BYTES, cpu_to_be64(bytes)) ||
 	    nla_put_be32(skb, NFACCT_USE, htonl(atomic_read(&acct->refcnt))))
 		goto nla_put_failure;
+	if (acct->flags & NFACCT_F_QUOTA) {
+		u64 *quota = (u64 *)acct->data;
 
+		if (nla_put_be32(skb, NFACCT_FLAGS, htonl(acct->flags)) ||
+		    nla_put_be64(skb, NFACCT_QUOTA, cpu_to_be64(*quota)))
+			goto nla_put_failure;
+	}
 	nlmsg_end(skb, nlh);
 	return skb->len;
 
@@ -270,6 +310,8 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
 	[NFACCT_NAME] = { .type = NLA_NUL_STRING, .len = NFACCT_NAME_MAX-1 },
 	[NFACCT_BYTES] = { .type = NLA_U64 },
 	[NFACCT_PKTS] = { .type = NLA_U64 },
+	[NFACCT_FLAGS] = { .type = NLA_U32 },
+	[NFACCT_QUOTA] = { .type = NLA_U64 },
 };
 
 static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
@@ -336,6 +378,50 @@ void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct)
 }
 EXPORT_SYMBOL_GPL(nfnl_acct_update);
 
+static void nfnl_overquota_report(struct nf_acct *nfacct)
+{
+	int ret;
+	struct sk_buff *skb;
+
+	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (skb == NULL)
+		return;
+
+	ret = nfnl_acct_fill_info(skb, 0, 0, NFNL_MSG_ACCT_OVERQUOTA, 0,
+				  nfacct);
+	if (ret <= 0) {
+		kfree_skb(skb);
+		return;
+	}
+	netlink_broadcast(init_net.nfnl, skb, 0, NFNLGRP_ACCT_QUOTA,
+			  GFP_ATOMIC);
+}
+
+int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
+{
+	u64 now;
+	u64 *quota;
+	bool ret = NFACCT_UNDERQUOTA;
+
+	/* no place here if we don't have a quota */
+	if (!(nfacct->flags & NFACCT_F_QUOTA))
+		return NFACCT_NO_QUOTA;
+
+	quota = (u64 *)nfacct->data;
+	now = (nfacct->flags & NFACCT_F_QUOTA_PKTS) ?
+	       atomic64_read(&nfacct->pkts) : atomic64_read(&nfacct->bytes);
+
+	ret = now > *quota;
+
+	if (now >= *quota &&
+	    !test_and_set_bit(NFACCT_F_OVERQUOTA, &nfacct->flags)) {
+		nfnl_overquota_report(nfacct);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nfnl_acct_overquota);
+
 static int __init nfnl_acct_init(void)
 {
 	int ret;
diff --git a/net/netfilter/xt_nfacct.c b/net/netfilter/xt_nfacct.c
index b3be0ef..94b0f09 100644
--- a/net/netfilter/xt_nfacct.c
+++ b/net/netfilter/xt_nfacct.c
@@ -21,10 +21,16 @@ MODULE_ALIAS("ip6t_nfacct");
 
 static bool nfacct_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
+	bool overquota;
 	const struct xt_nfacct_match_info *info = par->targinfo;
 
 	nfnl_acct_update(skb, info->nfacct);
 
+	overquota = nfnl_acct_overquota(skb, info->nfacct);
+
+	if (overquota == NFACCT_UNDERQUOTA)
+		return false;
+
 	return true;
 }
 
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] Extend accounting capabilities to support quotas
  2014-04-01 22:52 [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework mathieu.poirier
@ 2014-04-01 22:52 ` mathieu.poirier
  2014-04-07 15:20 ` [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework Pablo Neira Ayuso
  1 sibling, 0 replies; 4+ messages in thread
From: mathieu.poirier @ 2014-04-01 22:52 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel, netfilter, mathieu.poirier

From: Mathieu Poirier <mathieu.poirier@linaro.org>

The accounting framework already supports accounting at the
quota and byte level.  As such it is a natural extention to
add a ceiling limit to those metrics.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 include/libnetfilter_acct/libnetfilter_acct.h |  2 +
 include/linux/netfilter/nfnetlink.h           |  4 ++
 include/linux/netfilter/nfnetlink_acct.h      |  9 ++++
 src/libnetfilter_acct.c                       | 67 +++++++++++++++++++++++++--
 4 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/include/libnetfilter_acct/libnetfilter_acct.h b/include/libnetfilter_acct/libnetfilter_acct.h
index b00e366..c6ed858 100644
--- a/include/libnetfilter_acct/libnetfilter_acct.h
+++ b/include/libnetfilter_acct/libnetfilter_acct.h
@@ -14,6 +14,8 @@ enum nfacct_attr_type {
 	NFACCT_ATTR_NAME = 0,
 	NFACCT_ATTR_PKTS,
 	NFACCT_ATTR_BYTES,
+	NFACCT_ATTR_FLAGS,
+	NFACCT_ATTR_QUOTA,
 };
 
 struct nfacct *nfacct_alloc(void);
diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index 4a4efaf..d3e0ea8 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -18,6 +18,10 @@ enum nfnetlink_groups {
 #define NFNLGRP_CONNTRACK_EXP_UPDATE	NFNLGRP_CONNTRACK_EXP_UPDATE
 	NFNLGRP_CONNTRACK_EXP_DESTROY,
 #define NFNLGRP_CONNTRACK_EXP_DESTROY	NFNLGRP_CONNTRACK_EXP_DESTROY
+	NFNLGRP_NFTABLES,
+#define NFNLGRP_NFTABLES		NFNLGRP_NFTABLES
+	NFNLGRP_ACCT_QUOTA,
+#define NFNLGRP_ACCT_QUOTA		NFNLGRP_ACCT_QUOTA
 	__NFNLGRP_MAX,
 };
 #define NFNLGRP_MAX	(__NFNLGRP_MAX - 1)
diff --git a/include/linux/netfilter/nfnetlink_acct.h b/include/linux/netfilter/nfnetlink_acct.h
index c7b6269..6b8c935 100644
--- a/include/linux/netfilter/nfnetlink_acct.h
+++ b/include/linux/netfilter/nfnetlink_acct.h
@@ -10,15 +10,24 @@ enum nfnl_acct_msg_types {
 	NFNL_MSG_ACCT_GET,
 	NFNL_MSG_ACCT_GET_CTRZERO,
 	NFNL_MSG_ACCT_DEL,
+	NFNL_MSG_ACCT_OVERQUOTA,
 	NFNL_MSG_ACCT_MAX
 };
 
+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 */
+};
+
 enum nfnl_acct_type {
 	NFACCT_UNSPEC,
 	NFACCT_NAME,
 	NFACCT_PKTS,
 	NFACCT_BYTES,
 	NFACCT_USE,
+	NFACCT_FLAGS,
+	NFACCT_QUOTA,
 	__NFACCT_MAX
 };
 #define NFACCT_MAX (__NFACCT_MAX - 1)
diff --git a/src/libnetfilter_acct.c b/src/libnetfilter_acct.c
index 77f58ce..0c1a758 100644
--- a/src/libnetfilter_acct.c
+++ b/src/libnetfilter_acct.c
@@ -61,6 +61,8 @@ struct nfacct {
 	uint64_t	pkts;
 	uint64_t	bytes;
 	uint32_t	bitset;
+	uint32_t	flags;
+	uint64_t	quota;
 };
 
 /**
@@ -114,6 +116,14 @@ nfacct_attr_set(struct nfacct *nfacct, enum nfacct_attr_type type,
 		nfacct->bytes = *((uint64_t *) data);
 		nfacct->bitset |= (1 << NFACCT_ATTR_BYTES);
 		break;
+	case NFACCT_ATTR_FLAGS:
+		nfacct->flags = *((uint32_t *) data);
+		nfacct->bitset |= (1 << NFACCT_ATTR_FLAGS);
+		break;
+	case NFACCT_ATTR_QUOTA:
+		nfacct->quota = *((uint64_t *) data);
+		nfacct->bitset |= (1 << NFACCT_ATTR_QUOTA);
+		break;
 	}
 }
 EXPORT_SYMBOL(nfacct_attr_set);
@@ -164,6 +174,12 @@ nfacct_attr_unset(struct nfacct *nfacct, enum nfacct_attr_type type)
 	case NFACCT_ATTR_BYTES:
 		nfacct->bitset &= ~(1 << NFACCT_ATTR_BYTES);
 		break;
+	case NFACCT_ATTR_FLAGS:
+		nfacct->bitset &= ~(1 << NFACCT_ATTR_FLAGS);
+		break;
+	case NFACCT_ATTR_QUOTA:
+		nfacct->bitset &= ~(1 << NFACCT_ATTR_QUOTA);
+		break;
 	}
 }
 EXPORT_SYMBOL(nfacct_attr_unset);
@@ -193,6 +209,14 @@ const void *nfacct_attr_get(struct nfacct *nfacct, enum nfacct_attr_type type)
 		if (nfacct->bitset & (1 << NFACCT_ATTR_BYTES))
 			ret = &nfacct->bytes;
 		break;
+	case NFACCT_ATTR_FLAGS:
+		if (nfacct->bitset & (1 << NFACCT_ATTR_FLAGS))
+			ret = &nfacct->flags;
+		break;
+	case NFACCT_ATTR_QUOTA:
+		if (nfacct->bitset & (1 << NFACCT_ATTR_QUOTA))
+			ret = &nfacct->quota;
+		break;
 	}
 	return ret;
 }
@@ -232,13 +256,35 @@ static int
 nfacct_snprintf_plain(char *buf, size_t rem, struct nfacct *nfacct,
 		      uint16_t flags)
 {
-	int ret;
+	int ret, temp;
+	char *walking_buf;
+
+	temp = rem;
+	walking_buf = buf;
 
 	if (flags & NFACCT_SNPRINTF_F_FULL) {
-		ret = snprintf(buf, rem,
-			"{ pkts = %.20"PRIu64", bytes = %.20"PRIu64" } = %s;",
+		ret = snprintf(walking_buf, temp,
+			"{ pkts = %.20"PRIu64", bytes = %.20"PRIu64"",
 			nfacct_attr_get_u64(nfacct, NFACCT_ATTR_PKTS),
-			nfacct_attr_get_u64(nfacct, NFACCT_ATTR_BYTES),
+			nfacct_attr_get_u64(nfacct, NFACCT_ATTR_BYTES));
+
+		if (nfacct->flags) {
+			uint32_t mode;
+
+			mode = nfacct_attr_get_u64(nfacct, NFACCT_ATTR_FLAGS);
+
+			walking_buf += ret;
+			temp -= ret;
+			ret = snprintf(walking_buf, temp,
+				", quota = %.20"PRIu64", mode = %s",
+				nfacct_attr_get_u64(nfacct, NFACCT_ATTR_QUOTA),
+				mode == NFACCT_F_QUOTA_BYTES ?
+				"byte" : "packet");
+		}
+
+		walking_buf += ret;
+		temp -= ret;
+		ret = snprintf(walking_buf, temp, " } = %s;",
 			nfacct_attr_get_str(nfacct, NFACCT_ATTR_NAME));
 	} else {
 		ret = snprintf(buf, rem, "%s\n",
@@ -424,6 +470,12 @@ void nfacct_nlmsg_build_payload(struct nlmsghdr *nlh, struct nfacct *nfacct)
 
 	if (nfacct->bitset & (1 << NFACCT_ATTR_BYTES))
 		mnl_attr_put_u64(nlh, NFACCT_BYTES, htobe64(nfacct->bytes));
+
+	if (nfacct->bitset & (1 << NFACCT_ATTR_FLAGS))
+		mnl_attr_put_u32(nlh, NFACCT_FLAGS, htobe32(nfacct->flags));
+
+	if (nfacct->bitset & (1 << NFACCT_ATTR_QUOTA))
+		mnl_attr_put_u64(nlh, NFACCT_QUOTA, htobe64(nfacct->quota));
 }
 EXPORT_SYMBOL(nfacct_nlmsg_build_payload);
 
@@ -479,6 +531,13 @@ nfacct_nlmsg_parse_payload(const struct nlmsghdr *nlh, struct nfacct *nfacct)
 	nfacct_attr_set_u64(nfacct, NFACCT_ATTR_BYTES,
 			    be64toh(mnl_attr_get_u64(tb[NFACCT_BYTES])));
 
+	if (tb[NFACCT_FLAGS] && tb[NFACCT_QUOTA]) {
+		uint32_t flags = be32toh(mnl_attr_get_u32(tb[NFACCT_FLAGS]));
+		nfacct_attr_set(nfacct, NFACCT_ATTR_FLAGS, &flags);
+		nfacct_attr_set_u64(nfacct, NFACCT_ATTR_QUOTA,
+			    be64toh(mnl_attr_get_u64(tb[NFACCT_QUOTA])));
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(nfacct_nlmsg_parse_payload);
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework
  2014-04-01 22:52 [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework mathieu.poirier
  2014-04-01 22:52 ` [PATCH 2/2] Extend accounting capabilities to support quotas mathieu.poirier
@ 2014-04-07 15:20 ` Pablo Neira Ayuso
  2014-04-07 15:41   ` Mathieu Poirier
  1 sibling, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2014-04-07 15:20 UTC (permalink / raw)
  To: mathieu.poirier; +Cc: netfilter-devel, netfilter

Hi Mathieu,

On Tue, Apr 01, 2014 at 04:52:51PM -0600, mathieu.poirier@linaro.org wrote:
> From: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> Nf_acct objects already support accounting at the byte and packet
> level.  As such it is a natural extention to add the possiblity to
> define a ceiling limit for both metrics.
> 
> All the support for quotas itself is added to nfnetlink acctounting
> framework to stay coherent with current accounting object management.
> Quota limit checks are implemented in xt nfacct filter where
> statistic collection is already done.

This looks good, we didn't reach the merge window though (it closed
the same day you sent me this). So please address some minor issues
here and resend, I'll keep this in my local tree until the net-next
merge window opens back.

As said, comments below.

> Pablo Niera Ayuso has also contributed to this feature.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  include/linux/netfilter/nfnetlink_acct.h      |  8 ++-
>  include/uapi/linux/netfilter/nfnetlink.h      |  2 +
>  include/uapi/linux/netfilter/nfnetlink_acct.h |  9 +++
>  net/netfilter/nfnetlink_acct.c                | 86 +++++++++++++++++++++++++++
>  net/netfilter/xt_nfacct.c                     |  6 ++
>  5 files changed, 110 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netfilter/nfnetlink_acct.h b/include/linux/netfilter/nfnetlink_acct.h
> index b2e85e5..6ec9757 100644
> --- a/include/linux/netfilter/nfnetlink_acct.h
> +++ b/include/linux/netfilter/nfnetlink_acct.h
> @@ -3,11 +3,17 @@
>  
>  #include <uapi/linux/netfilter/nfnetlink_acct.h>
>  
> +enum {
> +	NFACCT_NO_QUOTA		= -1,
> +	NFACCT_UNDERQUOTA,
> +	NFACCT_OVERQUOTA,
> +};
>  
>  struct nf_acct;
>  
>  struct nf_acct *nfnl_acct_find_get(const char *filter_name);
>  void nfnl_acct_put(struct nf_acct *acct);
>  void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct);
> -
> +extern int nfnl_acct_overquota(const struct sk_buff *skb,
> +			      struct nf_acct *nfacct);
>  #endif /* _NFNL_ACCT_H */
> diff --git a/include/uapi/linux/netfilter/nfnetlink.h b/include/uapi/linux/netfilter/nfnetlink.h
> index 596ddd4..354a7e5 100644
> --- a/include/uapi/linux/netfilter/nfnetlink.h
> +++ b/include/uapi/linux/netfilter/nfnetlink.h
> @@ -20,6 +20,8 @@ enum nfnetlink_groups {
>  #define NFNLGRP_CONNTRACK_EXP_DESTROY	NFNLGRP_CONNTRACK_EXP_DESTROY
>  	NFNLGRP_NFTABLES,
>  #define NFNLGRP_NFTABLES                NFNLGRP_NFTABLES
> +	NFNLGRP_ACCT_QUOTA,
> +#define NFNLGRP_ACCT_QUOTA		NFNLGRP_ACCT_QUOTA
>  	__NFNLGRP_MAX,
>  };
>  #define NFNLGRP_MAX	(__NFNLGRP_MAX - 1)
> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
> index c7b6269..51404ec 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
> @@ -10,15 +10,24 @@ enum nfnl_acct_msg_types {
>  	NFNL_MSG_ACCT_GET,
>  	NFNL_MSG_ACCT_GET_CTRZERO,
>  	NFNL_MSG_ACCT_DEL,
> +	NFNL_MSG_ACCT_OVERQUOTA,
>  	NFNL_MSG_ACCT_MAX
>  };
>  
> +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 */
> +};
> +
>  enum nfnl_acct_type {
>  	NFACCT_UNSPEC,
>  	NFACCT_NAME,
>  	NFACCT_PKTS,
>  	NFACCT_BYTES,
>  	NFACCT_USE,
> +	NFACCT_FLAGS,
> +	NFACCT_QUOTA,
>  	__NFACCT_MAX
>  };
>  #define NFACCT_MAX (__NFACCT_MAX - 1)
> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
> index c7b6d46..dae35eb 100644
> --- a/net/netfilter/nfnetlink_acct.c
> +++ b/net/netfilter/nfnetlink_acct.c
> @@ -32,18 +32,24 @@ static LIST_HEAD(nfnl_acct_list);
>  struct nf_acct {
>  	atomic64_t		pkts;
>  	atomic64_t		bytes;
> +	unsigned long		flags;
>  	struct list_head	head;
>  	atomic_t		refcnt;
>  	char			name[NFACCT_NAME_MAX];
>  	struct rcu_head		rcu_head;
> +	char			data[0];
>  };
>  
> +#define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
> +
>  static int
>  nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>  	     const struct nlmsghdr *nlh, const struct nlattr * const tb[])
>  {
>  	struct nf_acct *nfacct, *matching = NULL;
>  	char *acct_name;
> +	unsigned int size = 0;
> +	u32 flags = 0;
>  
>  	if (!tb[NFACCT_NAME])
>  		return -EINVAL;
> @@ -68,15 +74,39 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>  			/* reset counters if you request a replacement. */
>  			atomic64_set(&matching->pkts, 0);
>  			atomic64_set(&matching->bytes, 0);
> +			smp_mb__before_clear_bit();
> +			/* reset overquota flag if quota is enabled. */
> +			if ((matching->flags & NFACCT_F_QUOTA))
> +				clear_bit(NFACCT_F_OVERQUOTA, &matching->flags);
>  			return 0;
>  		}
>  		return -EBUSY;
>  	}
>  
>  	nfacct = kzalloc(sizeof(struct nf_acct), GFP_KERNEL);
> +	if (tb[NFACCT_FLAGS]) {
> +		flags = ntohl(nla_get_be32(tb[NFACCT_FLAGS]));
> +		if (flags & ~NFACCT_F_QUOTA)
> +			return -EOPNOTSUPP;
> +		if ((flags & NFACCT_F_QUOTA) == NFACCT_F_QUOTA)
> +			return -EINVAL;
> +		if (flags & NFACCT_F_OVERQUOTA)
> +			return -EINVAL;
> +
> +		size += sizeof(u64);
> +	}
> +
> +	nfacct = kzalloc(sizeof(struct nf_acct) + size, GFP_KERNEL);
>  	if (nfacct == NULL)
>  		return -ENOMEM;
>  
> +	if (flags & NFACCT_F_QUOTA) {
> +		u64 *quota = (u64 *)nfacct->data;
> +
> +		*quota = be64_to_cpu(nla_get_be64(tb[NFACCT_QUOTA]));
> +		nfacct->flags = flags;
> +	}
> +
>  	strncpy(nfacct->name, nla_data(tb[NFACCT_NAME]), NFACCT_NAME_MAX);
>  
>  	if (tb[NFACCT_BYTES]) {
> @@ -117,6 +147,10 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>  	if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
>  		pkts = atomic64_xchg(&acct->pkts, 0);
>  		bytes = atomic64_xchg(&acct->bytes, 0);
> +		if (acct->flags & NFACCT_F_QUOTA) {
> +			smp_mb__before_clear_bit();
                        ^------------------------^

I think you have to move this just before the if branch.

> +			clear_bit(NFACCT_F_OVERQUOTA, &acct->flags);
> +		}
>  	} else {
>  		pkts = atomic64_read(&acct->pkts);
>  		bytes = atomic64_read(&acct->bytes);
> @@ -125,7 +159,13 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>  	    nla_put_be64(skb, NFACCT_BYTES, cpu_to_be64(bytes)) ||
>  	    nla_put_be32(skb, NFACCT_USE, htonl(atomic_read(&acct->refcnt))))
>  		goto nla_put_failure;
> +	if (acct->flags & NFACCT_F_QUOTA) {
> +		u64 *quota = (u64 *)acct->data;
>  
> +		if (nla_put_be32(skb, NFACCT_FLAGS, htonl(acct->flags)) ||
> +		    nla_put_be64(skb, NFACCT_QUOTA, cpu_to_be64(*quota)))
> +			goto nla_put_failure;
> +	}
>  	nlmsg_end(skb, nlh);
>  	return skb->len;
>  
> @@ -270,6 +310,8 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
>  	[NFACCT_NAME] = { .type = NLA_NUL_STRING, .len = NFACCT_NAME_MAX-1 },
>  	[NFACCT_BYTES] = { .type = NLA_U64 },
>  	[NFACCT_PKTS] = { .type = NLA_U64 },
> +	[NFACCT_FLAGS] = { .type = NLA_U32 },
> +	[NFACCT_QUOTA] = { .type = NLA_U64 },
>  };
>  
>  static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
> @@ -336,6 +378,50 @@ void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct)
>  }
>  EXPORT_SYMBOL_GPL(nfnl_acct_update);
>  
> +static void nfnl_overquota_report(struct nf_acct *nfacct)
> +{
> +	int ret;
> +	struct sk_buff *skb;
> +
> +	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> +	if (skb == NULL)
> +		return;
> +
> +	ret = nfnl_acct_fill_info(skb, 0, 0, NFNL_MSG_ACCT_OVERQUOTA, 0,
> +				  nfacct);
> +	if (ret <= 0) {
> +		kfree_skb(skb);
> +		return;
> +	}
> +	netlink_broadcast(init_net.nfnl, skb, 0, NFNLGRP_ACCT_QUOTA,
> +			  GFP_ATOMIC);
> +}
> +
> +int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
> +{
> +	u64 now;
> +	u64 *quota;
> +	bool ret = NFACCT_UNDERQUOTA;

enums are u32, so you have to use int there at least.

> +	/* no place here if we don't have a quota */
> +	if (!(nfacct->flags & NFACCT_F_QUOTA))
> +		return NFACCT_NO_QUOTA;
> +
> +	quota = (u64 *)nfacct->data;
> +	now = (nfacct->flags & NFACCT_F_QUOTA_PKTS) ?
> +	       atomic64_read(&nfacct->pkts) : atomic64_read(&nfacct->bytes);
> +
> +	ret = now > *quota;
> +
> +	if (now >= *quota &&

This should be:

        if (now > *quota &&

I believe we report once we're at least 1 byte over quota, right?

> +	    !test_and_set_bit(NFACCT_F_OVERQUOTA, &nfacct->flags)) {
> +		nfnl_overquota_report(nfacct);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nfnl_acct_overquota);
> +
>  static int __init nfnl_acct_init(void)
>  {
>  	int ret;
> diff --git a/net/netfilter/xt_nfacct.c b/net/netfilter/xt_nfacct.c
> index b3be0ef..94b0f09 100644
> --- a/net/netfilter/xt_nfacct.c
> +++ b/net/netfilter/xt_nfacct.c
> @@ -21,10 +21,16 @@ MODULE_ALIAS("ip6t_nfacct");
>  
>  static bool nfacct_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  {
> +	bool overquota;

this needs to be convert to enum nfacct_quota_state or simply int as
it is not a boolean anymore.

>  	const struct xt_nfacct_match_info *info = par->targinfo;
>  
>  	nfnl_acct_update(skb, info->nfacct);
> 
> +	overquota = nfnl_acct_overquota(skb, info->nfacct);
> +
> +	if (overquota == NFACCT_UNDERQUOTA)
> +		return false;
> +
>  	return true;

We can save a couple of lines with this:

        quota_state = nfnl_acct_overquota(skb, info->nfacct);

        return quota_state == NFACCT_UNDERQUOTA ? false : true;

It would be also great if you look at the nfacct tool extensions. I'd
like to have a monitor mode that spots quota warnings for testing
this.

Thanks!

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework
  2014-04-07 15:20 ` [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework Pablo Neira Ayuso
@ 2014-04-07 15:41   ` Mathieu Poirier
  0 siblings, 0 replies; 4+ messages in thread
From: Mathieu Poirier @ 2014-04-07 15:41 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, netfilter

On 7 April 2014 09:20, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Mathieu,
>
> On Tue, Apr 01, 2014 at 04:52:51PM -0600, mathieu.poirier@linaro.org wrote:
>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>> Nf_acct objects already support accounting at the byte and packet
>> level.  As such it is a natural extention to add the possiblity to
>> define a ceiling limit for both metrics.
>>
>> All the support for quotas itself is added to nfnetlink acctounting
>> framework to stay coherent with current accounting object management.
>> Quota limit checks are implemented in xt nfacct filter where
>> statistic collection is already done.
>
> This looks good, we didn't reach the merge window though (it closed
> the same day you sent me this). So please address some minor issues
> here and resend, I'll keep this in my local tree until the net-next
> merge window opens back.

Perfect.

>
> As said, comments below.
>
>> Pablo Niera Ayuso has also contributed to this feature.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  include/linux/netfilter/nfnetlink_acct.h      |  8 ++-
>>  include/uapi/linux/netfilter/nfnetlink.h      |  2 +
>>  include/uapi/linux/netfilter/nfnetlink_acct.h |  9 +++
>>  net/netfilter/nfnetlink_acct.c                | 86 +++++++++++++++++++++++++++
>>  net/netfilter/xt_nfacct.c                     |  6 ++
>>  5 files changed, 110 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/netfilter/nfnetlink_acct.h b/include/linux/netfilter/nfnetlink_acct.h
>> index b2e85e5..6ec9757 100644
>> --- a/include/linux/netfilter/nfnetlink_acct.h
>> +++ b/include/linux/netfilter/nfnetlink_acct.h
>> @@ -3,11 +3,17 @@
>>
>>  #include <uapi/linux/netfilter/nfnetlink_acct.h>
>>
>> +enum {
>> +     NFACCT_NO_QUOTA         = -1,
>> +     NFACCT_UNDERQUOTA,
>> +     NFACCT_OVERQUOTA,
>> +};
>>
>>  struct nf_acct;
>>
>>  struct nf_acct *nfnl_acct_find_get(const char *filter_name);
>>  void nfnl_acct_put(struct nf_acct *acct);
>>  void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct);
>> -
>> +extern int nfnl_acct_overquota(const struct sk_buff *skb,
>> +                           struct nf_acct *nfacct);
>>  #endif /* _NFNL_ACCT_H */
>> diff --git a/include/uapi/linux/netfilter/nfnetlink.h b/include/uapi/linux/netfilter/nfnetlink.h
>> index 596ddd4..354a7e5 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink.h
>> @@ -20,6 +20,8 @@ enum nfnetlink_groups {
>>  #define NFNLGRP_CONNTRACK_EXP_DESTROY        NFNLGRP_CONNTRACK_EXP_DESTROY
>>       NFNLGRP_NFTABLES,
>>  #define NFNLGRP_NFTABLES                NFNLGRP_NFTABLES
>> +     NFNLGRP_ACCT_QUOTA,
>> +#define NFNLGRP_ACCT_QUOTA           NFNLGRP_ACCT_QUOTA
>>       __NFNLGRP_MAX,
>>  };
>>  #define NFNLGRP_MAX  (__NFNLGRP_MAX - 1)
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> index c7b6269..51404ec 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> @@ -10,15 +10,24 @@ enum nfnl_acct_msg_types {
>>       NFNL_MSG_ACCT_GET,
>>       NFNL_MSG_ACCT_GET_CTRZERO,
>>       NFNL_MSG_ACCT_DEL,
>> +     NFNL_MSG_ACCT_OVERQUOTA,
>>       NFNL_MSG_ACCT_MAX
>>  };
>>
>> +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 */
>> +};
>> +
>>  enum nfnl_acct_type {
>>       NFACCT_UNSPEC,
>>       NFACCT_NAME,
>>       NFACCT_PKTS,
>>       NFACCT_BYTES,
>>       NFACCT_USE,
>> +     NFACCT_FLAGS,
>> +     NFACCT_QUOTA,
>>       __NFACCT_MAX
>>  };
>>  #define NFACCT_MAX (__NFACCT_MAX - 1)
>> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
>> index c7b6d46..dae35eb 100644
>> --- a/net/netfilter/nfnetlink_acct.c
>> +++ b/net/netfilter/nfnetlink_acct.c
>> @@ -32,18 +32,24 @@ static LIST_HEAD(nfnl_acct_list);
>>  struct nf_acct {
>>       atomic64_t              pkts;
>>       atomic64_t              bytes;
>> +     unsigned long           flags;
>>       struct list_head        head;
>>       atomic_t                refcnt;
>>       char                    name[NFACCT_NAME_MAX];
>>       struct rcu_head         rcu_head;
>> +     char                    data[0];
>>  };
>>
>> +#define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
>> +
>>  static int
>>  nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>>            const struct nlmsghdr *nlh, const struct nlattr * const tb[])
>>  {
>>       struct nf_acct *nfacct, *matching = NULL;
>>       char *acct_name;
>> +     unsigned int size = 0;
>> +     u32 flags = 0;
>>
>>       if (!tb[NFACCT_NAME])
>>               return -EINVAL;
>> @@ -68,15 +74,39 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>>                       /* reset counters if you request a replacement. */
>>                       atomic64_set(&matching->pkts, 0);
>>                       atomic64_set(&matching->bytes, 0);
>> +                     smp_mb__before_clear_bit();
>> +                     /* reset overquota flag if quota is enabled. */
>> +                     if ((matching->flags & NFACCT_F_QUOTA))
>> +                             clear_bit(NFACCT_F_OVERQUOTA, &matching->flags);
>>                       return 0;
>>               }
>>               return -EBUSY;
>>       }
>>
>>       nfacct = kzalloc(sizeof(struct nf_acct), GFP_KERNEL);
>> +     if (tb[NFACCT_FLAGS]) {
>> +             flags = ntohl(nla_get_be32(tb[NFACCT_FLAGS]));
>> +             if (flags & ~NFACCT_F_QUOTA)
>> +                     return -EOPNOTSUPP;
>> +             if ((flags & NFACCT_F_QUOTA) == NFACCT_F_QUOTA)
>> +                     return -EINVAL;
>> +             if (flags & NFACCT_F_OVERQUOTA)
>> +                     return -EINVAL;
>> +
>> +             size += sizeof(u64);
>> +     }
>> +
>> +     nfacct = kzalloc(sizeof(struct nf_acct) + size, GFP_KERNEL);
>>       if (nfacct == NULL)
>>               return -ENOMEM;
>>
>> +     if (flags & NFACCT_F_QUOTA) {
>> +             u64 *quota = (u64 *)nfacct->data;
>> +
>> +             *quota = be64_to_cpu(nla_get_be64(tb[NFACCT_QUOTA]));
>> +             nfacct->flags = flags;
>> +     }
>> +
>>       strncpy(nfacct->name, nla_data(tb[NFACCT_NAME]), NFACCT_NAME_MAX);
>>
>>       if (tb[NFACCT_BYTES]) {
>> @@ -117,6 +147,10 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>       if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
>>               pkts = atomic64_xchg(&acct->pkts, 0);
>>               bytes = atomic64_xchg(&acct->bytes, 0);
>> +             if (acct->flags & NFACCT_F_QUOTA) {
>> +                     smp_mb__before_clear_bit();
>                         ^------------------------^
>
> I think you have to move this just before the if branch.

I've been wondering about that while looking at your code. Why execute
a memory barrier if we aren't sure that clear_bit() will be called?
I'm thinking you have a valid reason that I don't see.

>
>> +                     clear_bit(NFACCT_F_OVERQUOTA, &acct->flags);
>> +             }
>>       } else {
>>               pkts = atomic64_read(&acct->pkts);
>>               bytes = atomic64_read(&acct->bytes);
>> @@ -125,7 +159,13 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>           nla_put_be64(skb, NFACCT_BYTES, cpu_to_be64(bytes)) ||
>>           nla_put_be32(skb, NFACCT_USE, htonl(atomic_read(&acct->refcnt))))
>>               goto nla_put_failure;
>> +     if (acct->flags & NFACCT_F_QUOTA) {
>> +             u64 *quota = (u64 *)acct->data;
>>
>> +             if (nla_put_be32(skb, NFACCT_FLAGS, htonl(acct->flags)) ||
>> +                 nla_put_be64(skb, NFACCT_QUOTA, cpu_to_be64(*quota)))
>> +                     goto nla_put_failure;
>> +     }
>>       nlmsg_end(skb, nlh);
>>       return skb->len;
>>
>> @@ -270,6 +310,8 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
>>       [NFACCT_NAME] = { .type = NLA_NUL_STRING, .len = NFACCT_NAME_MAX-1 },
>>       [NFACCT_BYTES] = { .type = NLA_U64 },
>>       [NFACCT_PKTS] = { .type = NLA_U64 },
>> +     [NFACCT_FLAGS] = { .type = NLA_U32 },
>> +     [NFACCT_QUOTA] = { .type = NLA_U64 },
>>  };
>>
>>  static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
>> @@ -336,6 +378,50 @@ void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct)
>>  }
>>  EXPORT_SYMBOL_GPL(nfnl_acct_update);
>>
>> +static void nfnl_overquota_report(struct nf_acct *nfacct)
>> +{
>> +     int ret;
>> +     struct sk_buff *skb;
>> +
>> +     skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>> +     if (skb == NULL)
>> +             return;
>> +
>> +     ret = nfnl_acct_fill_info(skb, 0, 0, NFNL_MSG_ACCT_OVERQUOTA, 0,
>> +                               nfacct);
>> +     if (ret <= 0) {
>> +             kfree_skb(skb);
>> +             return;
>> +     }
>> +     netlink_broadcast(init_net.nfnl, skb, 0, NFNLGRP_ACCT_QUOTA,
>> +                       GFP_ATOMIC);
>> +}
>> +
>> +int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
>> +{
>> +     u64 now;
>> +     u64 *quota;
>> +     bool ret = NFACCT_UNDERQUOTA;
>
> enums are u32, so you have to use int there at least.

Of course - the side effects of doing things hastily.

>
>> +     /* no place here if we don't have a quota */
>> +     if (!(nfacct->flags & NFACCT_F_QUOTA))
>> +             return NFACCT_NO_QUOTA;
>> +
>> +     quota = (u64 *)nfacct->data;
>> +     now = (nfacct->flags & NFACCT_F_QUOTA_PKTS) ?
>> +            atomic64_read(&nfacct->pkts) : atomic64_read(&nfacct->bytes);
>> +
>> +     ret = now > *quota;
>> +
>> +     if (now >= *quota &&
>
> This should be:
>
>         if (now > *quota &&
>
> I believe we report once we're at least 1 byte over quota, right?

I take quota limits to be inclusive.  As such if we set a limit of 50
packets, I expect the 50th packet to be allowed through but not the
51st one.  On the flip side there is no way to tell when that 51st
packet will arrive - it can potentially take a long time and as such
decided it was worthy to send the notification right away.

>
>> +         !test_and_set_bit(NFACCT_F_OVERQUOTA, &nfacct->flags)) {
>> +             nfnl_overquota_report(nfacct);
>> +     }
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(nfnl_acct_overquota);
>> +
>>  static int __init nfnl_acct_init(void)
>>  {
>>       int ret;
>> diff --git a/net/netfilter/xt_nfacct.c b/net/netfilter/xt_nfacct.c
>> index b3be0ef..94b0f09 100644
>> --- a/net/netfilter/xt_nfacct.c
>> +++ b/net/netfilter/xt_nfacct.c
>> @@ -21,10 +21,16 @@ MODULE_ALIAS("ip6t_nfacct");
>>
>>  static bool nfacct_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>  {
>> +     bool overquota;
>
> this needs to be convert to enum nfacct_quota_state or simply int as
> it is not a boolean anymore.

Evidently yes.

>
>>       const struct xt_nfacct_match_info *info = par->targinfo;
>>
>>       nfnl_acct_update(skb, info->nfacct);
>>
>> +     overquota = nfnl_acct_overquota(skb, info->nfacct);
>> +
>> +     if (overquota == NFACCT_UNDERQUOTA)
>> +             return false;
>> +
>>       return true;
>
> We can save a couple of lines with this:
>
>         quota_state = nfnl_acct_overquota(skb, info->nfacct);
>
>         return quota_state == NFACCT_UNDERQUOTA ? false : true;

ack

>
> It would be also great if you look at the nfacct tool extensions. I'd
> like to have a monitor mode that spots quota warnings for testing
> this.

I have most of the nfacct changes done but didn't send the patches as
it wasn't ready for submission yet.  Once we agreed on the kernel side
I was going to send the userspace.  Ok for the monitoring mode, I
already have a test application that does that.

Where will you want the patches to be sent?  Netdev and netfilter-dev
even if they are userspace?

>
> Thanks!

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-04-07 15:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-01 22:52 [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework mathieu.poirier
2014-04-01 22:52 ` [PATCH 2/2] Extend accounting capabilities to support quotas mathieu.poirier
2014-04-07 15:20 ` [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework Pablo Neira Ayuso
2014-04-07 15:41   ` Mathieu Poirier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.