xt_quota: don't copy quota back to userspace

PATCH In review Changli Gao 4 participants 9 messages Jul 23, 2010

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Changli Gao

author

2010-07-23 04:54 UTC #0 lore raw

PATCH 1/1 xt_quota: don't copy quota back to userspace

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 <[email protected]>

----

 include/linux/netfilter/xt_quota.h |    2 +-
 net/netfilter/xt_quota.c           |    2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

include/linux/netfilter/xt_quota.h +1 -1

index 8dc89df..b0d28c6 100644
@@ -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;
 };

net/netfilter/xt_quota.c +0 -2

index 304b1fd..70eb2b4 100644
@@ -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;

Eric Dumazet

2010-07-23 05:27 UTC #1 lore raw

Quoted 10 lines · 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 <[email protected]>

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2010-07-23 05:40 UTC #2 lore raw

Quoted 10 lines · Eric Dumazet

Quoted 9 lines · 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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Changli Gao <[email protected]>
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Changli Gao

author

2010-07-23 05:52 UTC #3 lore raw

Quoted 6 lines · Eric Dumazet

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)

Quoted 9 lines

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.

Signature · 2 lines

Regards,
Changli Gao([email protected])

Patrick McHardy

2010-07-23 12:10 UTC #4 lore raw

In reply to Eric Dumazet

Quoted 24 lines · Eric Dumazet

Quoted 10 lines · Eric Dumazet
Quoted 9 lines · 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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Changli Gao <[email protected]>
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)

Quoted 8 lines

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.

Changli Gao

author

2010-07-23 05:43 UTC #5 lore raw

In reply to Eric Dumazet

Quoted 10 lines · Eric Dumazet

Quoted 9 lines · 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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Changli Gao <[email protected]>
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.

Signature · 6 lines

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

Jan Engelhardt

2010-07-23 06:20 UTC #6 lore raw

In reply to Changli Gao

Quoted 8 lines · 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.

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_.

Eric Dumazet

2010-07-23 06:28 UTC #7 lore raw

Quoted 7 lines · Jan Engelhardt

Quoted 5 lines · 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

Quoted 8 lines

Quoted 3 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2010-07-23 12:03 UTC #8 lore raw

Quoted 12 lines · Eric Dumazet

Quoted 11 lines · Jan Engelhardt
Quoted 8 lines · 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.

Quoted 6 lines

Quoted 4 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html