| Message ID | 1406570272-3704-2-git-send-email-a.perevalov@samsung.com |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
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. -- 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
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.
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. -- 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
---------------------------------------- > 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
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(+)
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;
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(-)