[PATCH nf] netfilter: nft_quota: make nft_overquota() really over the quota

[PATCH nf] netfilter: nft_quota: make nft_overquota() really over the quota

[Zhongqiu Duan]

Keep consistency with xt_quota and nfacct.

Fixes: 795595f68d6c ("netfilter: nft_quota: dump consumed quota")
Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
---
 net/netfilter/nft_quota.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
index 9b2d7463d3d3..0bb43c723061 100644
--- a/net/netfilter/nft_quota.c
+++ b/net/netfilter/nft_quota.c
@@ -21,7 +21,7 @@ struct nft_quota {
 static inline bool nft_overquota(struct nft_quota *priv,
 				 const struct sk_buff *skb)
 {
-	return atomic64_add_return(skb->len, priv->consumed) >=
+	return atomic64_add_return(skb->len, priv->consumed) >
 	       atomic64_read(&priv->quota);
 }
 
-- 
2.43.0

Re: [PATCH nf] netfilter: nft_quota: make nft_overquota() really over the quota

[Pablo Neira Ayuso]

On Thu, Apr 10, 2025 at 07:17:47AM +0000, Zhongqiu Duan wrote:
> Keep consistency with xt_quota and nfacct.

Where is the inconsistency?

> Fixes: 795595f68d6c ("netfilter: nft_quota: dump consumed quota")
> Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
> ---
>  net/netfilter/nft_quota.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
> index 9b2d7463d3d3..0bb43c723061 100644
> --- a/net/netfilter/nft_quota.c
> +++ b/net/netfilter/nft_quota.c
> @@ -21,7 +21,7 @@ struct nft_quota {
>  static inline bool nft_overquota(struct nft_quota *priv,
>  				 const struct sk_buff *skb)
>  {
> -	return atomic64_add_return(skb->len, priv->consumed) >=
> +	return atomic64_add_return(skb->len, priv->consumed) >
>  	       atomic64_read(&priv->quota);

From xt_quota:

        if (priv->quota >= skb->len) {
                priv->quota -= skb->len;
                ret = !ret;

Re: [PATCH nf] netfilter: nft_quota: make nft_overquota() really over the quota

[Zhongqiu Duan]

On Mon, Apr 14, 2025 at 11:55:45PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Apr 10, 2025 at 07:17:47AM +0000, Zhongqiu Duan wrote:
> > Keep consistency with xt_quota and nfacct.
> 
> Where is the inconsistency?
> 
> > Fixes: 795595f68d6c ("netfilter: nft_quota: dump consumed quota")
> > Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
> > ---
> >  net/netfilter/nft_quota.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
> > index 9b2d7463d3d3..0bb43c723061 100644
> > --- a/net/netfilter/nft_quota.c
> > +++ b/net/netfilter/nft_quota.c
> > @@ -21,7 +21,7 @@ struct nft_quota {
> >  static inline bool nft_overquota(struct nft_quota *priv,
> >  				 const struct sk_buff *skb)
> >  {
> > -	return atomic64_add_return(skb->len, priv->consumed) >=
> > +	return atomic64_add_return(skb->len, priv->consumed) >
> >  	       atomic64_read(&priv->quota);
> 
> >From xt_quota:
> 
>         if (priv->quota >= skb->len) {
>                 priv->quota -= skb->len;
>                 ret = !ret;

They behave differently in the case of consumed bytes equal to quota.

From nft_quota:

	overquota = nft_overquota(priv, pkt->skb);
	if (overquota ^ nft_quota_invert(priv))
		regs->verdict.code = NFT_BREAK;

The xt_quota compares skb length with remaining quota, but the nft_quota
compares it with consumed bytes.

The xt_quota can match consumed bytes up to quota at maximum. But the
nft_quota break match when consumed bytes equal to quota.

i.e., nft_quota match consumed bytes in [0, quota - 1], not [0, quota].


Also note that after applying this patch, nft_quota obj will report when
consumed bytes exceed the quota, but nfacct can report when consumed
bytes are greater than or equal to the quota.

From nft_quota:

	if (overquota &&
	    !test_and_set_bit(NFT_QUOTA_DEPLETED_BIT, &priv->flags))
		nft_obj_notify(...);

From nfacct:

	if (now >= *quota &&
	    !test_and_set_bit(NFACCT_OVERQUOTA_BIT, &nfacct->flags)) {
		nfnl_overquota_report(net, nfacct);
	}

Should we report when quota is exhausted but not exceeded?


Regards

Re: [PATCH nf] netfilter: nft_quota: make nft_overquota() really over the quota

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

Hi,

On Tue, Apr 15, 2025 at 02:03:59PM +0000, Zhongqiu Duan wrote:
[...]
> They behave differently in the case of consumed bytes equal to quota.
> 
> From nft_quota:
> 
> 	overquota = nft_overquota(priv, pkt->skb);
> 	if (overquota ^ nft_quota_invert(priv))
> 		regs->verdict.code = NFT_BREAK;
> 
> The xt_quota compares skb length with remaining quota, but the nft_quota
> compares it with consumed bytes.
> 
> The xt_quota can match consumed bytes up to quota at maximum. But the
> nft_quota break match when consumed bytes equal to quota.
> 
> i.e., nft_quota match consumed bytes in [0, quota - 1], not [0, quota].

Thanks for explaining.

> Also note that after applying this patch, nft_quota obj will report when
> consumed bytes exceed the quota, but nfacct can report when consumed
> bytes are greater than or equal to the quota.
> 
> From nft_quota:
> 
> 	if (overquota &&
> 	    !test_and_set_bit(NFT_QUOTA_DEPLETED_BIT, &priv->flags))
> 		nft_obj_notify(...);
> 
> From nfacct:
> 
> 	if (now >= *quota &&
> 	    !test_and_set_bit(NFACCT_OVERQUOTA_BIT, &nfacct->flags)) {
> 		nfnl_overquota_report(net, nfacct);
> 	}
> 
> Should we report when quota is exhausted but not exceeded?

I think it is good if nfacct and nft_quota behave in the same way.

I'd suggest you collapse this patch.

Please, route this patch v2 through the nf-next tree.

Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 854 bytes --]

diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
index 0bb43c723061..9fd6985f54c5 100644
--- a/net/netfilter/nft_quota.c
+++ b/net/netfilter/nft_quota.c
@@ -51,13 +51,15 @@ static void nft_quota_obj_eval(struct nft_object *obj,
 			       const struct nft_pktinfo *pkt)
 {
 	struct nft_quota *priv = nft_obj_data(obj);
+	u64 consumed = atomic64_add_return(pkt->skb->len, priv->consumed);
+	u64 quota = atomic64_read(&priv->quota);
 	bool overquota;
 
-	overquota = nft_overquota(priv, pkt->skb);
+	overquota = (consumed > quota);
 	if (overquota ^ nft_quota_invert(priv))
 		regs->verdict.code = NFT_BREAK;
 
-	if (overquota &&
+	if (consumed >= quota &&
 	    !test_and_set_bit(NFT_QUOTA_DEPLETED_BIT, &priv->flags))
 		nft_obj_notify(nft_net(pkt), obj->key.table, obj, 0, 0,
 			       NFT_MSG_NEWOBJ, 0, nft_pf(pkt), 0, GFP_ATOMIC);

Re: [PATCH nf] netfilter: nft_quota: make nft_overquota() really over the quota

[Zhongqiu Duan]

On Thu, Apr 17, 2025 at 01:21:58PM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Tue, Apr 15, 2025 at 02:03:59PM +0000, Zhongqiu Duan wrote:
> [...]
> > They behave differently in the case of consumed bytes equal to quota.
> > 
> > From nft_quota:
> > 
> > 	overquota = nft_overquota(priv, pkt->skb);
> > 	if (overquota ^ nft_quota_invert(priv))
> > 		regs->verdict.code = NFT_BREAK;
> > 
> > The xt_quota compares skb length with remaining quota, but the nft_quota
> > compares it with consumed bytes.
> > 
> > The xt_quota can match consumed bytes up to quota at maximum. But the
> > nft_quota break match when consumed bytes equal to quota.
> > 
> > i.e., nft_quota match consumed bytes in [0, quota - 1], not [0, quota].
> 
> Thanks for explaining.
> 
> > Also note that after applying this patch, nft_quota obj will report when
> > consumed bytes exceed the quota, but nfacct can report when consumed
> > bytes are greater than or equal to the quota.
> > 
> > From nft_quota:
> > 
> > 	if (overquota &&
> > 	    !test_and_set_bit(NFT_QUOTA_DEPLETED_BIT, &priv->flags))
> > 		nft_obj_notify(...);
> > 
> > From nfacct:
> > 
> > 	if (now >= *quota &&
> > 	    !test_and_set_bit(NFACCT_OVERQUOTA_BIT, &nfacct->flags)) {
> > 		nfnl_overquota_report(net, nfacct);
> > 	}
> > 
> > Should we report when quota is exhausted but not exceeded?
> 
> I think it is good if nfacct and nft_quota behave in the same way.
> 
> I'd suggest you collapse this patch.
> 
> Please, route this patch v2 through the nf-next tree.
> 

Okay, I will send v2 to the nf-next tree.

Thanks for your attention.

> Thanks.

> diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
> index 0bb43c723061..9fd6985f54c5 100644
> --- a/net/netfilter/nft_quota.c
> +++ b/net/netfilter/nft_quota.c
> @@ -51,13 +51,15 @@ static void nft_quota_obj_eval(struct nft_object *obj,
>  			       const struct nft_pktinfo *pkt)
>  {
>  	struct nft_quota *priv = nft_obj_data(obj);
> +	u64 consumed = atomic64_add_return(pkt->skb->len, priv->consumed);
> +	u64 quota = atomic64_read(&priv->quota);
>  	bool overquota;
>  
> -	overquota = nft_overquota(priv, pkt->skb);
> +	overquota = (consumed > quota);
>  	if (overquota ^ nft_quota_invert(priv))
>  		regs->verdict.code = NFT_BREAK;
>  
> -	if (overquota &&
> +	if (consumed >= quota &&
>  	    !test_and_set_bit(NFT_QUOTA_DEPLETED_BIT, &priv->flags))
>  		nft_obj_notify(nft_net(pkt), obj->key.table, obj, 0, 0,
>  			       NFT_MSG_NEWOBJ, 0, nft_pf(pkt), 0, GFP_ATOMIC);