| Message ID | 1539126880-10072-1-git-send-email-chenbofeng.kernel@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables] extensions: Add tests and description for xt_quota module | expand |
Ah, yes, the (2**64 - 1) + 1 problem.
The fact max allowed remaining is (2**64 - 2) is perhaps surprising...
should we clamp? or warn?
userspace has:
if (cb->entry->id == O_REMAIN) info->remain++;
should this error out in userspace if we end up at zero?
+-m quota --quota 18446744073709551615 --remain 18446744073709551614;;FAIL
this one really should also pass...
kernel has:
if (atomic64_read(&q->counter) > q->quota + 1)
this should probably be:
if (atomic64_read(&q->counter) && atomic64_read(&q->counter) - 1 > q->quota)
Also I think there's something ugly with
-m quota --quota 18446744073709551614
vs
-m quota --quota 18446744073709551615
and thus possibly:
if (current_count <= skb->len) {
should actually be
if (current_count && current_count <= skb->len) {
Maybe all of this would actually be easier if we were counting bytes
used instead of bytes remaining.
On Tue, Oct 09, 2018 at 04:14:40PM -0700, Chenbo Feng wrote: > From: Chenbo Feng <fengc@google.com> > > Introduces some iptables tests for the new --remain option in xt_quota > module. Add a breif description for how to use the --remain option in > the iptables-extension man page. Applied, thanks Chenbo.
On Tue, Oct 09, 2018 at 04:47:20PM -0700, Maciej Żenczykowski wrote: > Ah, yes, the (2**64 - 1) + 1 problem. > > The fact max allowed remaining is (2**64 - 2) is perhaps surprising... > should we clamp? or warn? > > userspace has: > if (cb->entry->id == O_REMAIN) info->remain++; > should this error out in userspace if we end up at zero? > > +-m quota --quota 18446744073709551615 --remain 18446744073709551614;;FAIL > > this one really should also pass... :-) > kernel has: > if (atomic64_read(&q->counter) > q->quota + 1) > this should probably be: > if (atomic64_read(&q->counter) && atomic64_read(&q->counter) - 1 > q->quota) > > Also I think there's something ugly with > -m quota --quota 18446744073709551614 > vs > -m quota --quota 18446744073709551615 > > and thus possibly: > if (current_count <= skb->len) { > should actually be > if (current_count && current_count <= skb->len) { > > Maybe all of this would actually be easier if we were counting bytes > used instead of bytes remaining. I think so. This is still net-next, so noone is using it yet apart from developers? Probably we can still change this to become --consumed rather than --remain. I would take patches for nf-next if you follow that path, no problem. Thanks.
diff --git a/extensions/libxt_quota.man b/extensions/libxt_quota.man index fbecf37..388348f 100644 --- a/extensions/libxt_quota.man +++ b/extensions/libxt_quota.man @@ -5,3 +5,7 @@ byte counter reaches zero). .TP [\fB!\fP] \fB\-\-quota\fP \fIbytes\fP The quota in bytes. +.TP +\fB\-\-remain\fP \fIbytes\fP +The remaining quota in bytes. If not specified, the remaining quota is equal +to the value specified in \fB\-\-quota\fP. diff --git a/extensions/libxt_quota.t b/extensions/libxt_quota.t index c568427..8768e61 100644 --- a/extensions/libxt_quota.t +++ b/extensions/libxt_quota.t @@ -5,3 +5,14 @@ -m quota ! --quota 18446744073709551615;=;OK -m quota --quota 18446744073709551616;;FAIL -m quota;;FAIL +-m quota --quota 0 --remain 0;=;OK +-m quota --quota 0 --remain 1;;FAIL +-m quota --quota 1000 --remain 0;=;OK +-m quota ! --quota 1000 --remain 0;=;OK +-m quota --quota 1000 --remain 1000;=;OK +-m quota ! --quota 1000 --remain 1000;=;OK +-m quota --quota 1000 --remain 1001;;FAIL +-m quota --quota 18446744073709551614 --remain 18446744073709551614;=;OK +-m quota --quota 18446744073709551615 --remain 18446744073709551614;;FAIL +-m quota --quota 18446744073709551616 --remain 18446744073709551615;;FAIL +-m quota --quota 18446744073709551615 --remain 18446744073709551616;;FAIL