netfilter buglog - Jan 2018 - [Bug 1214] New: Allow limit to use any value for time unit

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

            Bug ID: 1214
           Summary: Allow limit to use any value for time unit
           Product: nftables
           Version: unspecified
          Hardware: All
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: nft
          Assignee: pablo at netfilter.org
          Reporter: boite.pour.spam at gmail.com

Currently, it's not possible to set a limit for, let's say, fifteen
minutes.
This is completely arbitrary, and counter productive because there is a huge
gap between "mn" and "hour".

The underlying interface use seconds anyway since the specified unit is
converted to seconds in
http://git.netfilter.org/nftables/tree/src/datatype.c#n1074 , so it should be
possible to change:
"limit 10/mn" to "limit 10/300" (or "limit
10/300s")

This would require an additional line in the code from:
static struct error_record *time_unit_parse(const struct location *loc,
                        const char *str, uint64_t *unit)
{
    if (strcmp(str, "second") == 0)
        *unit = 1ULL;
    else if (strcmp(str, "minute") == 0)
        *unit = 1ULL * 60;
    else if (strcmp(str, "hour") == 0)
        *unit = 1ULL * 60 * 60;
    else if (strcmp(str, "day") == 0)
        *unit = 1ULL * 60 * 60 * 24;
    else if (strcmp(str, "week") == 0)
        *unit = 1ULL * 60 * 60 * 24 * 7;
    else
        return error(loc, "Wrong rate format");

    return NULL;
}

to this:
static struct error_record *time_unit_parse(const struct location *loc,
                        const char *str, uint64_t *unit)
{
    if (strcmp(str, "second") == 0)
        *unit = 1ULL;
    else if (strcmp(str, "minute") == 0)
        *unit = 1ULL * 60;
    else if (strcmp(str, "hour") == 0)
        *unit = 1ULL * 60 * 60;
    else if (strcmp(str, "day") == 0)
        *unit = 1ULL * 60 * 60 * 24;
    else if (strcmp(str, "week") == 0)
        *unit = 1ULL * 60 * 60 * 24 * 7;
        else if (strchr("123456789", *str)) /* Starts with a non zero
number */
                *unit = (unsigned long long)strtol(str, NULL, 10);
    else
        return error(loc, "Wrong rate format");

    return NULL;
}

The bison code should also be modified to allow number here:
limit_stmt        :    LIMIT    RATE    limit_mode    NUM    SLASH    time_unit
   limit_burst
                {
                $$ = limit_stmt_alloc(&@$);
                $$->limit.rate    = $4;
                $$->limit.unit    = $6;
                $$->limit.burst    = $7;
                $$->limit.type    = NFT_LIMIT_PKTS;
                $$->limit.flags = $3;
            }

should read (kind of, untested):
limit_stmt        :    LIMIT    RATE    limit_mode    NUM    SLASH    STRING   
limit_burst
                {
                struct error_record *erec;
                uint64_t unit;

                erec = time_unit_parse(&@$, $5, &unit);
                xfree($5);
                if (erec != NULL) {
                    erec_queue(erec, state->msgs);
                    YYERROR;
                }
                                $$ = limit_stmt_alloc(&@$);
                $$->limit.rate    = $4;
                $$->limit.unit    = unit;
                $$->limit.burst    = $7;
                $$->limit.type    = NFT_LIMIT_PKTS;
                $$->limit.flags = $3;
            }

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180110/bf6acdd1/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

Pablo Neira Ayuso <pablo at netfilter.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #1 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Would you send us a patch for this?

Otherwise, I'll try to convince someone to work on this, but I can make any
promise on delivery time.

Thanks!

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180110/ce7c5fc4/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #2 from Pablo Neira Ayuso <pablo at netfilter.org> ---
(In reply to Pablo Neira Ayuso from comment #1)
> Would you send us a patch for this?
> 
> Otherwise, I'll try to convince someone to work on this, but I can make
any
> promise on delivery time.
I meant, I *cannot* make any promise... :-)

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180110/ec023ebb/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #3 from Cyril <boite.pour.spam at gmail.com> ---
Once I've installed my system with git based nft, I'll make a patch.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180110/8be96ec4/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #4 from Cyril <boite.pour.spam at gmail.com> ---
Created attachment 518
  --> https://bugzilla.netfilter.org/attachment.cgi?id=518&action=edit
Allow any number in time_unit parsing

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180118/4463e937/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

Cyril <boite.pour.spam at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #518 is|0                           |1
           obsolete|                            |

--- Comment #5 from Cyril <boite.pour.spam at gmail.com> ---
Created attachment 519
  --> https://bugzilla.netfilter.org/attachment.cgi?id=519&action=edit
Fixed patch with correct listing when calling nft list

This should solve this bug.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180120/8d748133/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #6 from Pablo Neira Ayuso <pablo at netfilter.org> ---
(In reply to Cyril from comment #5)
> Created attachment 519 [details]
> Fixed patch with correct listing when calling nft list
> 
> This should solve this bug.
Thanks Cyril.

A couple of comments:

static char buffer[20]; 

Can we replace 20 by something more meaningful?

strlen("18446744073709551616");

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180120/d009585d/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #7 from Pablo Neira Ayuso <pablo at netfilter.org> ---
(In reply to Pablo Neira Ayuso from comment #6)
> (In reply to Cyril from comment #5)
> > Created attachment 519 [details]
> > Fixed patch with correct listing when calling nft list
> > 
> > This should solve this bug.
> 
> Thanks Cyril.
> 
> A couple of comments:
> 
> static char buffer[20]; 
> 
> Can we replace 20 by something more meaningful?
> 
> strlen("18446744073709551616");
Or add a comment

/* maximum string length for 64-bit value. */
static char buffer[20]; 

So we remember about the 20 char magic number, this actually needs to be 21
since we need to store \0.

BTW, would you also add tests to nftables/tests/py/any/limit.t

You will also need to update tests/py/any/limit.t.payload to include what nft
--debug=netlink generates.

Thanks !

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180120/e8d5c5f7/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #8 from Pablo Neira Ayuso <pablo at netfilter.org> ---
(In reply to Pablo Neira Ayuso from comment #7)
> (In reply to Pablo Neira Ayuso from comment #6)
> > (In reply to Cyril from comment #5)
> > > Created attachment 519 [details]
> > > Fixed patch with correct listing when calling nft list
> > > 
> > > This should solve this bug.
> > 
> > Thanks Cyril.
> > 
> > A couple of comments:
> > 
> > static char buffer[20]; 
> > 
> > Can we replace 20 by something more meaningful?
> > 
> > strlen("18446744073709551616");
> 
> Or add a comment
> 
> /* maximum string length for 64-bit value. */
> static char buffer[20]; 
> 
> So we remember about the 20 char magic number, this actually needs to be 21
> since we need to store \0.
> 
> BTW, would you also add tests to nftables/tests/py/any/limit.t
> 
> You will also need to update tests/py/any/limit.t.payload to include what
> nft --debug=netlink generates.
> 
> Thanks !
Regarding syntax, I wonder if it is worth to express this including time unit,
ie.

    | NUM SECONDS

So we leave room to support things like "1 packet every 3 weeks" later
on.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180120/16e03c98/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #9 from Cyril <boite.pour.spam at gmail.com> ---
Yes, that seems more logical. I assume that not using a "value before
unit"
means 1 for that unit and is not an error.

So I intend to do the following: 
1. Revert to master's state
2. Add a new rule in Bison to accept "NUM SLASH NUM TIME_UNIT"
3. Change the code in statement.c to print it correctly (as you said, I made a
mistake in the worst case length, and add some comment to justify it)

However, I'm not sure what's the best way for the following:
1. Modify the time_unit_parse to accept a value before the unit itself 
and/or
2. Rename it "time_parse" since it's parsing both time & unit
or
3. Modify the upper function ("rate_parse" something, IDRC) to parse
the value
and keep the time_unit_parse as-is (but that imply "3 week" and not
"3 weeks"
in the textual version of the rule)
or
4. Reject any specification that's not "value unit" thus let all
bisons rules
merge into a single one "NUM STRING" and parse it to accept
"345k/3week" as
well as "12/hour" or "3/900second" and so on. This however
will likely require
large modification to bison's rules since STRING is a "default"
case, so I need
to remove "hour/week/month/second" to avoid failing the grammar. 

I'm not sure about the test itself and the debug-netlink stuff. I'll try
to
hack around but I'll need guidance about it if I don't succeed
implementing it.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180122/ffc1f7b4/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

Cyril <boite.pour.spam at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #519 is|0                           |1
           obsolete|                            |

--- Comment #10 from Cyril <boite.pour.spam at gmail.com> ---
Created attachment 524
  --> https://bugzilla.netfilter.org/attachment.cgi?id=524&action=edit
Add support for period parsing

This follows option #3 in the previous comment:
It adds support for "limit 400/3 minute" or "235kbytes/6
day"

Please notice that this patch will only pass tests if the other patch is
applied to libnftnl.

While libnftnl is sharing the "get_unit" function (same code in both
project),
I think it'll be better to rename and export the function to
"get_period"
(since it's now a period and not just an unit anymore) so we can avoid code
duplication. This is not part of this bug though.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180127/03e658ac/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #11 from Cyril <boite.pour.spam at gmail.com> ---
Created attachment 525
  --> https://bugzilla.netfilter.org/attachment.cgi?id=525&action=edit
Patch for libnftnl to get good dump in tests too.

This is required to pass the tests with debug=netfilter since it's this code
that's called.
However, I guess it will be even better to make this visible and rename it to
get_period instead and remove the duplicate in nftables.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180127/e1808156/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

--- Comment #12 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Thanks a lot for working on this.

Would you send these patch to netfilter-devel at vger.kernel.org?

Please, include your Signed-off-by tag.

Thanks!

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180130/62fa580f/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

rypervenche <sub at ryper.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sub at ryper.org

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180429/0c53b37a/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1214

kfm at plushkava.net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kfm at plushkava.net

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20191208/dbfb36ca/attachment.html>