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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
---
  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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Netfitler Users]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux