[PATCH net-next iptables] Rework the xt_quota module

[PATCH net-next iptables] Rework the xt_quota module

[Chenbo Feng]

From: Chenbo Feng <fengc@google.com>

MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The current xt_quota module uses an additional kernel struct to store
the remaining quota of an iptables match and does not expose it to
userspace. The major flaw of the current implementation is the quota
will be reset whenever an unrelated rule gets updated in the same table
as an xt_quota rule exists. Such behavior makes xt_quota rules not very
useful in a dynamically changing iptables setup.

To fix the problem above, a new remain field is introduced to replace
the kernel struct pointer. It is used in the kernel as an atomic64_t and
treated as an __aligned_u64 when it is copied to userspace. Since the
struct xt_quota_info uses __aligned_u64 to record the quota, it is
guaranteed the padding space at the end of the current struct is 64bit
wide even on a 32bit machine. So we can safely pass the current quota
from kernel to userspace without adding an extra revision to the
xt_quota module. Userspace can set the remaining quota with new
"--remain x" option when restoring a rule that is previously saved.

For general quota rule insertion/deletion, the user can choose whether
to specify the remaining quota or not. If a quota rule is inserted
without specifying the remaining quota, it will be set to the original
quota. As for rule deletion, the remaining part is not matched since it
is a dynamically changing field when there is live networking traffic
and makes no sense to specify the exact value of current remaining
quota.

For an old kernel with new iptables with this fix, the remain field will
not get copied into kernel memory because of the usersize definition in
kernel xt_quota module. So it will still act the same as old xt_quota
module. When dumping or saving the iptables rule, the kernel pointer
will not get copied to userspace and iptables will ignore the remaining
field if it is just zero initilized memory.

For a kernel with fix running against old iptables, insertion or
deletion of an unrelated rule no longer resets the quota since the
remain field will be copied out of kernel and updated back during the
rule insertion/deletion process.

kernel changes:

Chenbo Feng (1):
  netfilter: xt_quota: fix the behavior of xt_quota module

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

iptables changes:

Chenbo Feng (1):
  extensions: libxt_quota: Allow setting the remaining quota

 extensions/libxt_quota.c           | 23 +++++++++++++++++++++--
 include/linux/netfilter/xt_quota.h |  8 +++++---
 2 files changed, 26 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH iptables] extensions: libxt_quota: Allow setting the remaining quota

[Chenbo Feng]

From: Chenbo Feng <fengc@google.com>

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.

Signed-off-by: Chenbo Feng <fengc@google.com>
Suggested-by: Maciej Żenczykowski <maze@google.com>
Reviewed-by: Maciej Żenczykowski <maze@google.com>
---
 extensions/libxt_quota.c           | 25 +++++++++++++++++++++++--
 include/linux/netfilter/xt_quota.h |  8 +++++---
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/extensions/libxt_quota.c b/extensions/libxt_quota.c
index bad77d2..6371aa0 100644
--- a/extensions/libxt_quota.c
+++ b/extensions/libxt_quota.c
@@ -9,26 +9,36 @@
 
 enum {
 	O_QUOTA = 0,
+	O_REMAIN = 1,
 };
 
 static const struct xt_option_entry quota_opts[] = {
 	{.name = "quota", .id = O_QUOTA, .type = XTTYPE_UINT64,
 	 .flags = XTOPT_MAND | XTOPT_INVERT | XTOPT_PUT,
 	 XTOPT_POINTER(struct xt_quota_info, quota)},
+	{.name = "remain", .id = O_REMAIN, .type = XTTYPE_UINT64,
+	 .flags = XTOPT_PUT, XTOPT_POINTER(struct xt_quota_info, remain)},
 	XTOPT_TABLEEND,
 };
 
 static void quota_help(void)
 {
 	printf("quota match options:\n"
-	       "[!] --quota quota		quota (bytes)\n");
+	       "[!] --quota quota		quota (bytes)\n"
+	       "    --remain remain		remain (bytes)\n");
 }
 
 static void
 quota_print(const void *ip, const struct xt_entry_match *match, int numeric)
 {
 	const struct xt_quota_info *q = (const void *)match->data;
+	if (q->flags & XT_QUOTA_INVERT)
+		printf(" !");
 	printf(" quota: %llu bytes", (unsigned long long)q->quota);
+	if (q->remain) {
+		printf(" remain: %llu bytes",
+			(unsigned long long)q->remain - 1);
+	}
 }
 
 static void
@@ -39,6 +49,10 @@ quota_save(const void *ip, const struct xt_entry_match *match)
 	if (q->flags & XT_QUOTA_INVERT)
 		printf(" !");
 	printf(" --quota %llu", (unsigned long long) q->quota);
+	if (q->remain) {
+		printf(" --remain %llu",
+			(unsigned long long) q->remain - 1);
+	}
 }
 
 static void quota_parse(struct xt_option_call *cb)
@@ -48,6 +62,8 @@ static void quota_parse(struct xt_option_call *cb)
 	xtables_option_parse(cb);
 	if (cb->invert)
 		info->flags |= XT_QUOTA_INVERT;
+	if (cb->entry->id == O_REMAIN)
+		info->remain++;
 }
 
 static int quota_xlate(struct xt_xlate *xl,
@@ -66,7 +82,12 @@ static struct xtables_match quota_match = {
 	.name		= "quota",
 	.version	= XTABLES_VERSION,
 	.size		= XT_ALIGN(sizeof (struct xt_quota_info)),
-	.userspacesize	= offsetof(struct xt_quota_info, master),
+	/*
+	 * This size is only used for rule matching purpose when deleting
+	 * rules. The real size copied out from new kernel xt_quota module
+	 * is the whole struct xt_quota_info.
+	 */
+	.userspacesize	= offsetof(struct xt_quota_info, remain),
 	.help		= quota_help,
 	.print		= quota_print,
 	.save		= quota_save,
diff --git a/include/linux/netfilter/xt_quota.h b/include/linux/netfilter/xt_quota.h
index 9314723..d817aab 100644
--- a/include/linux/netfilter/xt_quota.h
+++ b/include/linux/netfilter/xt_quota.h
@@ -14,9 +14,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 */
-- 
2.7.4

Re: [PATCH iptables] extensions: libxt_quota: Allow setting the remaining quota

[Pablo Neira Ayuso]

On Mon, Oct 01, 2018 at 06:23:07PM -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> 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.

Applied, thanks.

Please, send me a patch to:

#1 add tests for iptables-tests.py, see .t file under extensions/
#2 document this new option in the manpage.

Thanks again.

[PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Chenbo Feng]

From: Chenbo Feng <fengc@google.com>

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 <fengc@google.com>
Suggested-by: Maciej Żenczykowski <maze@google.com>
Reviewed-by: Maciej Żenczykowski <maze@google.com>
---
 include/uapi/linux/netfilter/xt_quota.h |  8 +++--
 net/netfilter/xt_quota.c                | 55 +++++++++++++--------------------
 2 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/include/uapi/linux/netfilter/xt_quota.h b/include/uapi/linux/netfilter/xt_quota.h
index f3ba5d9..d72fd52 100644
--- a/include/uapi/linux/netfilter/xt_quota.h
+++ b/include/uapi/linux/netfilter/xt_quota.h
@@ -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 */
diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c
index 10d61a6..6afa7f4 100644
--- a/net/netfilter/xt_quota.c
+++ b/net/netfilter/xt_quota.c
@@ -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 <samj@samj.net>");
 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 */
-		priv->quota = 0;
-	}
-	spin_unlock_bh(&priv->lock);
-
-	return ret;
+	u64 old_count, new_count;
+
+	do {
+		if (current_count == 1)
+			return ret;
+		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);
+	return !ret;
 }
 
 static int quota_mt_check(const struct xt_mtchk_param *par)
 {
 	struct xt_quota_info *q = par->matchinfo;
 
+	BUILD_BUG_ON(sizeof(atomic64_t) != sizeof(__aligned_u64));
+
 	if (q->flags & ~XT_QUOTA_MASK)
 		return -EINVAL;
+	if (atomic64_read(&q->counter) > q->quota + 1)
+		return -ERANGE;
 
-	q->master = kmalloc(sizeof(*q->master), GFP_KERNEL);
-	if (q->master == NULL)
-		return -ENOMEM;
-
-	spin_lock_init(&q->master->lock);
-	q->master->quota = q->quota;
+	if (atomic64_read(&q->counter) == 0)
+		atomic64_set(&q->counter, q->quota + 1);
 	return 0;
 }
 
-static void quota_mt_destroy(const struct xt_mtdtor_param *par)
-{
-	const struct xt_quota_info *q = par->matchinfo;
-
-	kfree(q->master);
-}
-
 static struct xt_match quota_mt_reg __read_mostly = {
 	.name       = "quota",
 	.revision   = 0,
 	.family     = NFPROTO_UNSPEC,
 	.match      = quota_mt,
 	.checkentry = quota_mt_check,
-	.destroy    = quota_mt_destroy,
 	.matchsize  = sizeof(struct xt_quota_info),
-	.usersize   = offsetof(struct xt_quota_info, master),
 	.me         = THIS_MODULE,
 };
 
-- 
2.7.4

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

Hi,

On Mon, Oct 01, 2018 at 06:23:08PM -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> 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.

A few questions, see below.

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

> Signed-off-by: Chenbo Feng <fengc@google.com>
> Suggested-by: Maciej Żenczykowski <maze@google.com>
> Reviewed-by: Maciej Żenczykowski <maze@google.com>
> ---
>  include/uapi/linux/netfilter/xt_quota.h |  8 +++--
>  net/netfilter/xt_quota.c                | 55 +++++++++++++--------------------
>  2 files changed, 27 insertions(+), 36 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/xt_quota.h b/include/uapi/linux/netfilter/xt_quota.h
> index f3ba5d9..d72fd52 100644
> --- a/include/uapi/linux/netfilter/xt_quota.h
> +++ b/include/uapi/linux/netfilter/xt_quota.h
> @@ -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 */
> diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c
> index 10d61a6..6afa7f4 100644
> --- a/net/netfilter/xt_quota.c
> +++ b/net/netfilter/xt_quota.c
> @@ -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 <samj@samj.net>");
>  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 */
> -		priv->quota = 0;
> -	}
> -	spin_unlock_bh(&priv->lock);
> -
> -	return ret;
> +	u64 old_count, new_count;
> +
> +	do {
> +		if (current_count == 1)
> +			return ret;

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.

> +		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);

> +	return !ret;
>  }

Thanks !

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Maciej Żenczykowski]

> 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.

> 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.

> 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.

>> +             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()?

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).

> 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.

>
>         if (consumed > quota)
>                 printf("--consumed %PRIu64 ", quota);
>         else
>                 printf("--consumed %PRIu64 ", consumed);
>
>> +     return !ret;
>>  }
>
> Thanks !

Maciej Żenczykowski, Kernel Networking Developer @ Google

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Maciej Żenczykowski]

(perhaps the field should be called remain_plus_one?)

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

Hi Maciej!

On Tue, Oct 02, 2018 at 01:24:29AM -0700, Maciej Żenczykowski wrote:
> > 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...

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

> 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.

OK. Well, there is still one downside though from user perspective: A
user with new iptables userspace that supports --remain and old kernel
will not get this to work, since this option will be silently accepted
by the kernel, but it will be simply ignored.

I agree having revision infrastructure is limited and results in more
duplicated code, but this is what we have in iptables.

> > 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.

OK.

> > 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.

I see, indeed.

> >> +             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()?
> 
> 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).

Hm, not sure what you mean with adding -1. I mean replacing all this
code above with something like:

        ret = (atomic64_add_return(skb->len, &consumed) >= quota) ^ XT_QUOTA_INVERT;

But I might be missing anything from your response on why this is
racy.

> > 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,

Please, don't get this as some sort of push back / I'm saying no to
this as it is.

I just would like we are aware of possible downsides with this
approach. Having said this, I'll take it as is if you insist that this
is the right approach :-)

Thanks !

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

On Tue, Oct 02, 2018 at 12:11:19PM +0200, Pablo Neira Ayuso wrote:
> Hi Maciej!
> 
[...]
> On Tue, Oct 02, 2018 at 01:24:29AM -0700, Maciej Żenczykowski wrote:
> > > 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,

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.

:-)

Thanks!

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Maciej Żenczykowski]

> 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)

>> 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.

> 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.

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

On Tue, Oct 02, 2018 at 03:38:24AM -0700, Maciej Żenczykowski wrote:
> > 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.

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 :-).

> 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)
> 
> >> 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.
> 
> > 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.

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

On Tue, Oct 02, 2018 at 12:51:25PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 02, 2018 at 03:38:24AM -0700, Maciej Żenczykowski wrote:
> > > 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.
> 
> 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 :-).

Argh, broken sentence: I mean, I think it's the way to go for
iptables.

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Chenbo Feng]

On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Tue, Oct 02, 2018 at 03:38:24AM -0700, Maciej Żenczykowski wrote:
> > > 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.
>
> 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.
>
Sorry, resend in plain text mode.

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?
> Anyway, I think the --remain approach to fix this longstanding
> problem from iptables :-).
>
> > 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)
> >
> > >> 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.
> >
> > > 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.

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

Hi Chenbo,

On Tue, Oct 02, 2018 at 10:45:58AM -0700, Chenbo Feng wrote:
> On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <pablo@netfilter.org> 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?

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 <willemb@google.com>
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 !

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Chenbo Feng]

On Tue, Oct 2, 2018 at 11:15 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Chenbo,
>
> On Tue, Oct 02, 2018 at 10:45:58AM -0700, Chenbo Feng wrote:
> > On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <pablo@netfilter.org> 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?
>
> 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 <willemb@google.com>
> 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 !

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.

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

On Tue, Oct 02, 2018 at 11:28:21AM -0700, Chenbo Feng wrote:
> On Tue, Oct 2, 2018 at 11:15 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Hi Chenbo,
> >
> > On Tue, Oct 02, 2018 at 10:45:58AM -0700, Chenbo Feng wrote:
> > > On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <pablo@netfilter.org> 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?
> >
> > 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 <willemb@google.com>
> > 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 !
> 
> 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!

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Maciej Żenczykowski]

Could you clarify what exact behaviour Willem removed?

On Tue, Oct 2, 2018 at 11:43 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Oct 02, 2018 at 11:28:21AM -0700, Chenbo Feng wrote:
>> On Tue, Oct 2, 2018 at 11:15 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> >
>> > Hi Chenbo,
>> >
>> > On Tue, Oct 02, 2018 at 10:45:58AM -0700, Chenbo Feng wrote:
>> > > On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <pablo@netfilter.org> 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?
>> >
>> > 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 <willemb@google.com>
>> > 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 !
>>
>> 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!

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

On Mon, Oct 01, 2018 at 06:23:08PM -0700, Chenbo Feng wrote:
[...]
> diff --git a/include/uapi/linux/netfilter/xt_quota.h b/include/uapi/linux/netfilter/xt_quota.h
> index f3ba5d9..d72fd52 100644
> --- a/include/uapi/linux/netfilter/xt_quota.h
> +++ b/include/uapi/linux/netfilter/xt_quota.h
> @@ -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
>  };

Sorry, just noticed, one more question though:

Would this break backward compatibility with existing iptables 32-bits
binaries?

New kernel will hit size mismatch given that master pointer area used
to be 32-bits but now it is 64-bits long?

Let me know, thanks!

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Maciej Żenczykowski]

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.

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

On Wed, Oct 03, 2018 at 02:26:05AM -0700, Maciej Żenczykowski wrote:
> 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.

Great! Hopefully this is my last question. Thanks for addressing them all.

Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module

[Pablo Neira Ayuso]

On Mon, Oct 01, 2018 at 06:23:08PM -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> 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.

Applied, thanks.