Rework the xt_quota module

From: Chenbo Feng <[email protected]>

The current xt_quota module cannot track the current remaining quota of a specific rule. Everytime an unrelated rule is updated in the same iptables table, the quota will be reset. This is not a very useful function for iptables that get changed at run time. This patch fixes the above problem by adding a new field in the struct that records the current remaining quota.

Fixed a print out bug in verbose print out wrt. inversion.

From: Chenbo Feng <[email protected]>

A major flaw of the current xt_quota module is that quota in a specific rule gets reset every time there is a rule change in the same table. It makes the xt_quota module not very useful in a table in which iptables rules are changed at run time. This fix introduces a new counter that is visible to userspace as the remaining quota of the current rule. When userspace restores the rules in a table, it can restore the counter to the remaining quota instead of resetting it to the full quota.

Signed-off-by Chenbo Feng <[email protected]>

Suggested-by Maciej Żenczykowski <[email protected]>

Reviewed-by Maciej Żenczykowski <[email protected]>

---
 include/uapi/linux/netfilter/xt_quota.h |  8 +++--
 net/netfilter/xt_quota.c                | 55 +++++++++++++--------------------
 2 files changed, 27 insertions(+), 36 deletions(-)

include/uapi/linux/netfilter/xt_quota.h +5 -3

index f3ba5d9..d72fd52 100644
@@ -15,9 +15,11 @@ struct xt_quota_info {
 	__u32 flags;
 	__u32 pad;
 	__aligned_u64 quota;
-
-	/* Used internally by the kernel */
-	struct xt_quota_priv	*master;
+#ifdef __KERNEL__
+	atomic64_t counter;
+#else
+	__aligned_u64 remain;
+#endif
 };
 
 #endif /* _XT_QUOTA_H */

net/netfilter/xt_quota.c +1 -13

index 10d61a6..6afa7f4 100644
@@ -11,11 +11,6 @@
 #include <linux/netfilter/xt_quota.h>
 #include <linux/module.h>
 
-struct xt_quota_priv {
-	spinlock_t	lock;
-	uint64_t	quota;
-};
-
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sam Johnston <[email protected]>");
 MODULE_DESCRIPTION("Xtables: countdown quota match");

@@ -26,54 +21,48 @@ static bool
 quota_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	struct xt_quota_info *q = (void *)par->matchinfo;
-	struct xt_quota_priv *priv = q->master;
+	u64 current_count = atomic64_read(&q->counter);
 	bool ret = q->flags & XT_QUOTA_INVERT;
-
-	spin_lock_bh(&priv->lock);
-	if (priv->quota >= skb->len) {
-		priv->quota -= skb->len;
-		ret = !ret;
-	} else {
-		/* we do not allow even small packets from now on */

A few questions, see below.

First one is, don't we need a new match revision for this new option?

So 1 means, don't keep updating, quota is depleted?

This current_count = 1 would be exposed to userspace too, right?

Hm, this semantics are going to be a bit awkwards to users I think, I would prefer to expose this in a different way.

net/netfilter/xt_quota.c static bool · line 39

Quoted 9 lines

net/netfilter/xt_quota.c +9 -0

+             if (current_count <= skb->len) {
+                     atomic64_set(&q->counter, 1);
+                     return ret;
+             }
+             old_count = current_count;
+             new_count = current_count - skb->len;
+             current_count = atomic64_cmpxchg(&q->counter, old_count,
+                                              new_count);
+     } while (current_count != old_count);

Probably we simplify this via atomic64_add_return()?

I guess problem is userspace may get a current counter that is larger than the quota, but we could handle this from userspace iptables to print a value that equals the quota, ie. from userspace, before printing:

        if (consumed > quota)
                printf("--consumed %PRIu64 ", quota);
        else
                printf("--consumed %PRIu64 ", consumed);

Quoted 2 lines

+ return !ret;
}

Thanks !

Quoted 3 lines

A few questions, see below.

First one is, don't we need a new match revision for this new option?

We were very careful to do this in a way that doesn't need a new revision.

That's what basically dictates most of the design.

If we bump the revision then you don't get fixed semantics unless you update both kernel and userspace iptables versions...

and additionally we basically end up with two copies of xt_quota in the kernel source since there's pretty much nothing that can be shared between the two.

Quoted 1 line

So 1 means, don't keep updating, quota is depleted?

The counter is always 1 higher then the remaining quota.
So 1 means 0.
And 0 means uninitialized and is only present on input from userspace.

Quoted 4 lines

This current_count = 1 would be exposed to userspace too, right?

Hm, this semantics are going to be a bit awkwards to users I think, I would prefer to expose this in a different way.

New userspace iptables hides this from users by adding/decrementing by one as needed on ingress/egress from kernel.
Old iptables never looks at this field.

Quoted 10 lines

Quoted 9 lines

net/netfilter/xt_quota.c +9 -0

+             if (current_count <= skb->len) {
+                     atomic64_set(&q->counter, 1);
+                     return ret;
+             }
+             old_count = current_count;
+             new_count = current_count - skb->len;
+             current_count = atomic64_cmpxchg(&q->counter, old_count,
+                                              new_count);
+     } while (current_count != old_count);

Quoted 1 line

Probably we simplify this via atomic64_add_return()?

Unfortunately that doesn't work because it's racy if current value is 2 and two (or three) threads both add -1 you end up at zero (or even +lots).

Quoted 4 lines

I guess problem is userspace may get a current counter that is larger than the quota, but we could handle this from userspace iptables to print a value that equals the quota, ie. from userspace, before printing:

I'm not sure what you mean.

Hi Maciej!

Quoted 6 lines

Quoted 4 lines

I guess problem is userspace may get a current counter that is larger than the quota, but we could handle this from userspace iptables to print a value that equals the quota, ie. from userspace, before printing:

I'm not sure what you mean.

I mean: Instead of using atomic64_set() to set the counter to 1 once we went over quota,

Well, you will need a kernel + userspace update anyway, right?

Quoted 2 lines

I mean: Instead of using atomic64_set() to set the counter to 1 once we went over quota,

incomplete sentence, sorry:

I mean: Instead of using atomic64_set() to set the counter to 1 once we go overquota, we just keep updating 'consumed' bytes.

ie. we don't express things in 'remaining bytes' logic, but we account for 'bytes we already consumed'. So we never go negative - I know understand what you mean about -1... I think we are each other thinking from our respective approach proposal.

Quoted 1 line

Well, you will need a kernel + userspace update anyway, right?

It's true you need new iptables userspace to *see* during dump and/or manually *set* during restore the remain counter.

However, (and I believe Chenbo tested this) just a new kernel is enough to fix the problem of modifications within the table resetting the counter.
This is because the data gets copied out of kernel and back into kernel by old iptables without any further modifications. ie. the new kernel not clearing the field on copy to userspace and honouring it on copy to kernel is sufficient.

So iptables-save | iptables-restore doesn't work, but iptables -A foo does.

(currently iptables -t X -{A,D} foo clears all xt_quota counters in table X even when foo is utterly unrelated)

Quoted 7 lines

Quoted 2 lines

I mean: Instead of using atomic64_set() to set the counter to 1 once we went over quota,

incomplete sentence, sorry:

I mean: Instead of using atomic64_set() to set the counter to 1 once we go overquota, we just keep updating 'consumed' bytes.

I guess it's a fair point that with a u64 we won't ever realistically overflow the number of sent bytes, so this could be a running counter of matched bytes...

and we don't even need to update it if it was over the quota when we first looked at it, so we'll go over by at most # of cpus * max size of gso packet bytes.

Quoted 4 lines

ie. we don't express things in 'remaining bytes' logic, but we account for 'bytes we already consumed'. So we never go negative - I know understand what you mean about -1... I think we are each other thinking from our respective approach proposal.

I guess our decision was probably driven by xt_quota2 use on android where infinite quota is often used as a temporary placeholder.

Quoted 5 lines · Maciej Żenczykowski

Quoted 1 line

Well, you will need a kernel + userspace update anyway, right?

It's true you need new iptables userspace to *see* during dump and/or manually *set* during restore the remain counter.

Quoted 7 lines

However, (and I believe Chenbo tested this) just a new kernel is enough to fix the problem of modifications within the table resetting the counter.
This is because the data gets copied out of kernel and back into kernel by old iptables without any further modifications. ie. the new kernel not clearing the field on copy to userspace and honouring it on copy to kernel is sufficient.

I see, Willem removed this behaviour in newer kernels. The private area is now zeroed, is that what you mean right? So I guess this cannot be done transparently.

Anyway, I think the --remain approach to fix this longstanding problem from iptables :-).

Quoted 5 lines · Maciej Żenczykowski

Quoted 1 line

Well, you will need a kernel + userspace update anyway, right?

It's true you need new iptables userspace to *see* during dump and/or manually *set* during restore the remain counter.

Quoted 7 lines

However, (and I believe Chenbo tested this) just a new kernel is enough to fix the problem of modifications within the table resetting the counter.
This is because the data gets copied out of kernel and back into kernel by old iptables without any further modifications. ie. the new kernel not clearing the field on copy to userspace and honouring it on copy to kernel is sufficient.

I see, Willem removed this behaviour in newer kernels. The private area is now zeroed, is that what you mean right? So I guess this cannot be done transparently.

Anyway, I think the --remain approach to fix this longstanding problem from iptables :-).

Quoted 21 lines

So iptables-save | iptables-restore doesn't work, but iptables -A foo does.

(currently iptables -t X -{A,D} foo clears all xt_quota counters in table X even when foo is utterly unrelated)

Quoted 7 lines

Quoted 2 lines

I mean: Instead of using atomic64_set() to set the counter to 1 once we went over quota,

incomplete sentence, sorry:

I mean: Instead of using atomic64_set() to set the counter to 1 once we go overquota, we just keep updating 'consumed' bytes.

I guess it's a fair point that with a u64 we won't ever realistically overflow the number of sent bytes, so this could be a running counter of matched bytes...

and we don't even need to update it if it was over the quota when we first looked at it, so we'll go over by at most # of cpus * max size of gso packet bytes.

Quoted 7 lines

Quoted 4 lines

ie. we don't express things in 'remaining bytes' logic, but we account for 'bytes we already consumed'. So we never go negative - I know understand what you mean about -1... I think we are each other thinking from our respective approach proposal.

I guess our decision was probably driven by xt_quota2 use on android where infinite quota is often used as a temporary placeholder.

I see, thanks for explaining.

Thanks.

On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <[email protected]> wrote:

Do you mean the remain field will be zeroed when copying the xt_quota_info struct out of the kernel? I believe that is decided by the usersize defined in struct xt_match and this patch set it to the full struct size. So the whole xt_quota_info struct will be copied into userspace including the field stores the remaining quota. The userspace will not be aware of it if the ipatbles is not updated but it should not modify it as well. I have tested the behavior with net-next branch and it seems working. Am I missing something recently updated?

Hi Chenbo,

Quoted 1 line · Chenbo Feng

On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <[email protected]> wrote:

[...]

Quoted 9 lines

Do you mean the remain field will be zeroed when copying the xt_quota_info struct out of the kernel? I believe that is decided by the usersize defined in struct xt_match and this patch set it to the full struct size. So the whole xt_quota_info struct will be copied into userspace including the field stores the remaining quota. The userspace will not be aware of it if the ipatbles is not updated but it should not modify it as well. I have tested the behavior with net-next branch and it seems working. Am I missing something recently updated?

Hm, I see, I overlook that your patch removes this:

- .usersize = offsetof(struct xt_quota_info, master),

BTW, is iptables -D command working with your patch?

Telling this because if .usersize is removed, then IIRC userspace compares this new remain field with userspace value and deletion will break.

Patch that I was referring before is this one from Willem:

commit f32815d21d4d8287336fb9cef4d2d9e0866214c2

Author: Willem de Bruijn <[email protected]>
Date:   Mon Jan 2 17:19:40 2017 -0500

xtables: add xt_match, xt_target and data copy_to_user functions

xt_entry_target, xt_entry_match and their private data may contain kernel data.
[...]
Private data is defined in xt_match and xt_target. All matches and targets that maintain kernel data store this at the tail of their private structure. Extend xt_match and xt_target with .usersize to limit how many bytes of data are copied. The remainder is cleared.

Let me know, thanks !

Quoted 42 lines · Pablo Neira Ayuso

Hi Chenbo,

Quoted 1 line · Chenbo Feng

On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <[email protected]> wrote:

[...]

Quoted 9 lines

Do you mean the remain field will be zeroed when copying the xt_quota_info struct out of the kernel? I believe that is decided by the usersize defined in struct xt_match and this patch set it to the full struct size. So the whole xt_quota_info struct will be copied into userspace including the field stores the remaining quota. The userspace will not be aware of it if the ipatbles is not updated but it should not modify it as well. I have tested the behavior with net-next branch and it seems working. Am I missing something recently updated?

Hm, I see, I overlook that your patch removes this:

- .usersize = offsetof(struct xt_quota_info, master),

BTW, is iptables -D command working with your patch?

Telling this because if .usersize is removed, then IIRC userspace compares this new remain field with userspace value and deletion will break.

Patch that I was referring before is this one from Willem:

commit f32815d21d4d8287336fb9cef4d2d9e0866214c2

Author: Willem de Bruijn <[email protected]>
Date:   Mon Jan 2 17:19:40 2017 -0500

xtables: add xt_match, xt_target and data copy_to_user functions

xt_entry_target, xt_entry_match and their private data may contain kernel data.
[...]
Private data is defined in xt_match and xt_target. All matches and targets that maintain kernel data store this at the tail of their private structure. Extend xt_match and xt_target with .usersize to limit how many bytes of data are copied. The remainder is cleared.

Quoted 1 line

Let me know, thanks !

The delete operation is decided by the userspacesize defined in userspace ipatbles. I think it is unrelated to the usersize we talk about here. For old userspace iptables, the userspacesize is offsetof(struct xt_quota_info, master) so it will not compare the rest if the struct. And for new iptables we use offsetof(struct xt_quota_info, remain). Either way the userspace does not consider the remain field when comparing rules so we can do ipatbles rule deletion with or without specifying --remain option or even specify --remain to a wrong number. We decide to make it this way since the --remain field is changing all the time when there is network traffic going on and it's hard to compare the remaining quota for new ipatbles as well.

Quoted 45 lines · Chenbo Feng

Quoted 42 lines · Pablo Neira Ayuso

Hi Chenbo,

Quoted 1 line · Chenbo Feng

On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <[email protected]> wrote:

[...]

Quoted 9 lines

Do you mean the remain field will be zeroed when copying the xt_quota_info struct out of the kernel? I believe that is decided by the usersize defined in struct xt_match and this patch set it to the full struct size. So the whole xt_quota_info struct will be copied into userspace including the field stores the remaining quota. The userspace will not be aware of it if the ipatbles is not updated but it should not modify it as well. I have tested the behavior with net-next branch and it seems working. Am I missing something recently updated?

Hm, I see, I overlook that your patch removes this:

- .usersize = offsetof(struct xt_quota_info, master),

BTW, is iptables -D command working with your patch?

Telling this because if .usersize is removed, then IIRC userspace compares this new remain field with userspace value and deletion will break.

Patch that I was referring before is this one from Willem:

commit f32815d21d4d8287336fb9cef4d2d9e0866214c2

Author: Willem de Bruijn <[email protected]>
Date:   Mon Jan 2 17:19:40 2017 -0500

xtables: add xt_match, xt_target and data copy_to_user functions

xt_entry_target, xt_entry_match and their private data may contain kernel data.
[...]
Private data is defined in xt_match and xt_target. All matches and targets that maintain kernel data store this at the tail of their private structure. Extend xt_match and xt_target with .usersize to limit how many bytes of data are copied. The remainder is cleared.

Quoted 1 line

Let me know, thanks !

Quoted 11 lines

The delete operation is decided by the userspacesize defined in userspace ipatbles. I think it is unrelated to the usersize we talk about here. For old userspace iptables, the userspacesize is offsetof(struct xt_quota_info, master) so it will not compare the rest if the struct. And for new iptables we use offsetof(struct xt_quota_info, remain). Either way the userspace does not consider the remain field when comparing rules so we can do ipatbles rule deletion with or without specifying --remain option or even specify --remain to a wrong number. We decide to make it this way since the --remain field is changing all the time when there is network traffic going on and it's hard to compare the remaining quota for new ipatbles as well.

Thanks for explaining and your patience. Patch looks good then.

Thanks!

include/uapi/linux/netfilter/xt_quota.h +0 -0

index f3ba5d9..d72fd52 100644
@@ -15,9 +15,11 @@ struct xt_quota_info {
 	__u32 flags;
 	__u32 pad;

While on a 32-bit arch the tail pointer is 32 bits, the struct itself contains (and has always contained) a 64-bit value 'quota' with a forced 64-bit alignment which forces 64-bit alignment of the entire struct and thus also forces tail padding to make it a multiple of 64-bits. ie. size doesn't change - even on 32 bits.

Chenbo was testing compatibility on both 32 and 64 bit x86 VMs.

From: Chenbo Feng <[email protected]>

A major flaw of the current xt_quota module is that quota in a specific rule gets reset every time there is a rule change in the same table. It makes the xt_quota module not very useful in a table in which iptables rules are changed at run time. This fix introduces a new counter that is visible to userspace as the remaining quota of the current rule. When userspace restores the rules in a table, it can restore the counter to the remaining quota instead of resetting it to the full quota.