[PATCH] xt_quota: don't copy quota back to userspace

[PATCH] xt_quota: don't copy quota back to userspace

[Changli Gao]

This patch should be applied after my another patch:
http://patchwork.ozlabs.org/patch/59729/

xt_quota: don't copy quota back to userspace

In nowadays, table entries are per-cpu variables, so it don't make any sense to
copy quota back to one of the variable instances. To keep things simple, this
patch undo the copy.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 include/linux/netfilter/xt_quota.h |    2 +-
 net/netfilter/xt_quota.c           |    2 --
 2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/include/linux/netfilter/xt_quota.h b/include/linux/netfilter/xt_quota.h
index 8dc89df..b0d28c6 100644
--- a/include/linux/netfilter/xt_quota.h
+++ b/include/linux/netfilter/xt_quota.h
@@ -11,9 +11,9 @@ struct xt_quota_priv;
 struct xt_quota_info {
 	u_int32_t		flags;
 	u_int32_t		pad;
+	aligned_u64		quota;
 
 	/* Used internally by the kernel */
-	aligned_u64		quota;
 	struct xt_quota_priv	*master;
 };
 
diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c
index 304b1fd..70eb2b4 100644
--- a/net/netfilter/xt_quota.c
+++ b/net/netfilter/xt_quota.c
@@ -36,8 +36,6 @@ quota_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		/* we do not allow even small packets from now on */
 		priv->quota = 0;
 	}
-	/* Copy quota back to matchinfo so that iptables can display it */
-	q->quota = priv->quota;
 	spin_unlock_bh(&priv->lock);
 
 	return ret;

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Eric Dumazet]

Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit :
> This patch should be applied after my another patch:
> http://patchwork.ozlabs.org/patch/59729/
> 
> xt_quota: don't copy quota back to userspace
> 
> In nowadays, table entries are per-cpu variables, so it don't make any sense to
> copy quota back to one of the variable instances. To keep things simple, this
> patch undo the copy.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

This looks the wrong way to fix this problem.

Also Changli, could you please _not_ include the title of your patches
inside the Changelog ? This is useless.


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Eric Dumazet]

Le vendredi 23 juillet 2010 à 07:27 +0200, Eric Dumazet a écrit :
> Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit :
> > This patch should be applied after my another patch:
> > http://patchwork.ozlabs.org/patch/59729/
> > 
> > xt_quota: don't copy quota back to userspace
> > 
> > In nowadays, table entries are per-cpu variables, so it don't make any sense to
> > copy quota back to one of the variable instances. To keep things simple, this
> > patch undo the copy.
> > 
> > Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> 
> This looks the wrong way to fix this problem.
> 
> Also Changli, could you please _not_ include the title of your patches
> inside the Changelog ? This is useless.
> 

Reading again your patch, I understand only the Changelog is wrong.

We want to copy quota back to userspace, as specified when rule was
setup (so that iptables-save works)

The real thing you are doing is that we dont change the initial quota
during packet processing, only the private quota, shared by all cpus.

Before the patch , iptables -nvL could report an old and not accurate
quota value.

After the patch, iptables -nvL reports the initial quota value, not the
actual value.



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Changli Gao]

On Fri, Jul 23, 2010 at 1:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit :
>> This patch should be applied after my another patch:
>> http://patchwork.ozlabs.org/patch/59729/
>>
>> xt_quota: don't copy quota back to userspace
>>
>> In nowadays, table entries are per-cpu variables, so it don't make any sense to
>> copy quota back to one of the variable instances. To keep things simple, this
>> patch undo the copy.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>
> This looks the wrong way to fix this problem.
>
> Also Changli, could you please _not_ include the title of your patches
> inside the Changelog ? This is useless.
>

Thanks. I remembered  some document mentioned a summary line was
needed. I'll follow your advice.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Changli Gao]

On Fri, Jul 23, 2010 at 1:40 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Reading again your patch, I understand only the Changelog is wrong.
>
> We want to copy quota back to userspace, as specified when rule was
> setup (so that iptables-save works)
>
> The real thing you are doing is that we dont change the initial quota
> during packet processing, only the private quota, shared by all cpus.
>
> Before the patch , iptables -nvL could report an old and not accurate
> quota value.
>
> After the patch, iptables -nvL reports the initial quota value, not the
> actual value.
>

Yes. This module was expected to report the current quota value, when
iptables -nvL or iptables-save  was executed. It seems my patch
changes its ABI. However, report the initial value is also meaningful,
and it keeps the same behavior as the other modules do.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Jan Engelhardt]

On Friday 2010-07-23 06:54, Changli Gao wrote:

>This patch should be applied after my another patch:
>http://patchwork.ozlabs.org/patch/59729/
>
>xt_quota: don't copy quota back to userspace
>
>In nowadays, table entries are per-cpu variables, so it don't make any 
>sense to copy quota back to one of the variable instances. To keep 
>things simple, this patch undo the copy.

I object. This line is on purpose, to give at least a chance of 
reporting back a more-or-less believable value. Without copying
the value back, users have moaned about the counter not decreasing
_at all_.

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Eric Dumazet]

Le vendredi 23 juillet 2010 à 08:20 +0200, Jan Engelhardt a écrit :
> On Friday 2010-07-23 06:54, Changli Gao wrote:
> 
> >This patch should be applied after my another patch:
> >http://patchwork.ozlabs.org/patch/59729/
> >
> >xt_quota: don't copy quota back to userspace
> >
> >In nowadays, table entries are per-cpu variables, so it don't make any 
> >sense to copy quota back to one of the variable instances. To keep 
> >things simple, this patch undo the copy.
> 
> I object. This line is on purpose, to give at least a chance of 
> reporting back a more-or-less believable value. Without copying
> the value back, users have moaned about the counter not decreasing
> _at all_.

Maybe, but current situation is buggy.



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Patrick McHardy]

On 23.07.2010 08:28, Eric Dumazet wrote:
> Le vendredi 23 juillet 2010 à 08:20 +0200, Jan Engelhardt a écrit :
>> On Friday 2010-07-23 06:54, Changli Gao wrote:
>>
>>> This patch should be applied after my another patch:
>>> http://patchwork.ozlabs.org/patch/59729/
>>>
>>> xt_quota: don't copy quota back to userspace
>>>
>>> In nowadays, table entries are per-cpu variables, so it don't make any 
>>> sense to copy quota back to one of the variable instances. To keep 
>>> things simple, this patch undo the copy.
>>
>> I object. This line is on purpose, to give at least a chance of 
>> reporting back a more-or-less believable value. Without copying
>> the value back, users have moaned about the counter not decreasing
>> _at all_.
> 
> Maybe, but current situation is buggy.

Indeed, besides not being able to properly "iptables-save" a rule,
its not possible to delete a specific quota rule since they can't
be distinguished based on the specified quota value:

# iptables -A INPUT -m quota --quota 1000
# iptables -A INPUT -m quota --quota 2000
# iptables -D INPUT -m quota --quota 2000
# iptables -vxnL INPUT
Chain INPUT (policy ACCEPT 2 packets, 96 bytes)
    pkts      bytes target     prot opt in     out     source
    destination
       6      356            all  --  *      *       0.0.0.0/0
  0.0.0.0/0           quota: 1644 bytes

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xt_quota: don't copy quota back to userspace

[Patrick McHardy]

On 23.07.2010 07:40, Eric Dumazet wrote:
> Le vendredi 23 juillet 2010 à 07:27 +0200, Eric Dumazet a écrit :
>> Le vendredi 23 juillet 2010 à 12:54 +0800, Changli Gao a écrit :
>>> This patch should be applied after my another patch:
>>> http://patchwork.ozlabs.org/patch/59729/
>>>
>>> xt_quota: don't copy quota back to userspace
>>>
>>> In nowadays, table entries are per-cpu variables, so it don't make any sense to
>>> copy quota back to one of the variable instances. To keep things simple, this
>>> patch undo the copy.
>>>
>>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>>
>> This looks the wrong way to fix this problem.
>>
>> Also Changli, could you please _not_ include the title of your patches
>> inside the Changelog ? This is useless.
>>
> 
> Reading again your patch, I understand only the Changelog is wrong.
> 
> We want to copy quota back to userspace, as specified when rule was
> setup (so that iptables-save works)
> 
> The real thing you are doing is that we dont change the initial quota
> during packet processing, only the private quota, shared by all cpus.
> 
> Before the patch , iptables -nvL could report an old and not accurate
> quota value.
> 
> After the patch, iptables -nvL reports the initial quota value, not the
> actual value.

I've fixed up the changelog and applied the patch, thanks.

Changli, please also update the userspace extension to not ignore
the quota value on deletion.