Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Aug 14, 2024 at 06:00:55PM +0200, Phil Sutter wrote:
> On Wed, Aug 14, 2024 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> > Bail out if rate are used:
> > 
> >  ruleset.nft:5:77-106: Error: Wrong rate format, expecting bytes or kbytes or mbytes
> >  add rule netdev firewall PROTECTED_IPS update @quota_temp_before { ip daddr quota over 45000 mbytes/second } add @quota_trigger { ip daddr }
> >                                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > improve error reporting while at this.
> > 
> > Fixes: 6615676d825e ("src: add per-bytes limit")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > v2: - change patch subject
> >     - use strndup() to fetch units in rate_parse() so limit rate does not break.
> > 
> >  src/datatype.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/datatype.c b/src/datatype.c
> > index d398a9c8c618..297c5d0409d5 100644
> > --- a/src/datatype.c
> > +++ b/src/datatype.c
> > @@ -1485,14 +1485,14 @@ static struct error_record *time_unit_parse(const struct location *loc,
> >  struct error_record *data_unit_parse(const struct location *loc,
> >  				     const char *str, uint64_t *rate)
> >  {
> > -	if (strncmp(str, "bytes", strlen("bytes")) == 0)
> > +	if (strcmp(str, "bytes") == 0)
> >  		*rate = 1ULL;
> > -	else if (strncmp(str, "kbytes", strlen("kbytes")) == 0)
> > +	else if (strcmp(str, "kbytes") == 0)
> >  		*rate = 1024;
> > -	else if (strncmp(str, "mbytes", strlen("mbytes")) == 0)
> > +	else if (strcmp(str, "mbytes") == 0)
> >  		*rate = 1024 * 1024;
> >  	else
> > -		return error(loc, "Wrong rate format");
> > +		return error(loc, "Wrong unit format, expecting bytes, kbytes or mbytes");
> >  
> >  	return NULL;
> >  }
> 
> I have local commits which introduce KBYTES and MBYTES keywords and
> thereby kill the need for quota_unit and limit_bytes cases in
> parser_bison.y. It still needs testing and is surely not solving all
> issues there are, but I find it nicer than the partially redundant code
> we have right now.

Is this allowing for compact representation? ie. kbytes/second,
because I remember this was the issue to follow this poor man
approach.

> My motivation for this was to maybe improve parser's ability to handle
> lack of spaces in input. I still see the scanner fall into the generic
> "string" token case which requires manual dissection in the parser.
> 
> What is your motivation for the above changes?

A user that accidentally used:

        quota over 10mbytes/second

which is currently accepted, the /second part is misleading, as the
commit described, this is now rejected after this patch.

> Maybe we could collect parser limitations around these units and see
> what helps "the most"?

I am fine with handling this from the parser instead, there is now
flex start conditions that can help to enable these tokens on demand.