Patchwork iptables: xt_hashlimit fix

login
register
mail settings
Submitter Jan Engelhardt
Date Feb. 28, 2009, 1:54 a.m.
Message ID <alpine.LSU.2.00.0902280253370.20614@fbirervta.pbzchgretzou.qr>
Download mbox | patch
Permalink /patch/23865/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Jan Engelhardt - Feb. 28, 2009, 1:54 a.m.
On Friday 2009-02-20 19:33, Jan Engelhardt wrote:
>On Friday 2009-02-20 19:10, Eric Dumazet wrote:
>>
>>Damned this broke xt_hashlimit, version=0
>>Look file "net/netfilter/xt_hashlimit.c" line 706
>>
>>    /* Ugly hack: For SMP, we only want to use one set */
>>    r->u.master = r;
>>
>>So, it appears some modules are using pointers to themselves, what a hack :(
>>We probably need an audit of other modules.
>
>xt_limit and xt_statistic are affected; I'll happily fix that up.

Please have a look!

---8<---
parent b2bd9ab764d65237232c4aad5ab8d5d8b5714f72 (v2.6.29-rc6-31-gb2bd9ab)
commit 3e7ee1dcc808b8eec82eddfa0436f78e31d2004a
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Sat Feb 28 02:49:28 2009 +0100

netfilter: xtables: avoid pointer to self

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/linux/netfilter/xt_limit.h     |    9 +++--
 include/linux/netfilter/xt_statistic.h |    7 ++--
 net/netfilter/xt_limit.c               |   40 +++++++++++++++++------
 net/netfilter/xt_statistic.c           |   28 +++++++++++++---
 4 files changed, 61 insertions(+), 23 deletions(-)
Eric Dumazet - Feb. 28, 2009, 6:56 a.m.
Jan Engelhardt a écrit :
> On Friday 2009-02-20 19:33, Jan Engelhardt wrote:
>> On Friday 2009-02-20 19:10, Eric Dumazet wrote:
>>> Damned this broke xt_hashlimit, version=0
>>> Look file "net/netfilter/xt_hashlimit.c" line 706
>>>
>>>    /* Ugly hack: For SMP, we only want to use one set */
>>>    r->u.master = r;
>>>
>>> So, it appears some modules are using pointers to themselves, what a hack :(
>>> We probably need an audit of other modules.
>> xt_limit and xt_statistic are affected; I'll happily fix that up.
> 
> Please have a look!
> 
> ---8<---
> parent b2bd9ab764d65237232c4aad5ab8d5d8b5714f72 (v2.6.29-rc6-31-gb2bd9ab)
> commit 3e7ee1dcc808b8eec82eddfa0436f78e31d2004a
> Author: Jan Engelhardt <jengelh@medozas.de>
> Date:   Sat Feb 28 02:49:28 2009 +0100
> 
> netfilter: xtables: avoid pointer to self
> 
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

Seems good to me ! Thanks Jan !

Reviewed-by: Eric Dumazet <dada1@cosmosbay.com>

Are you sure xt_quota doesnt need some tweak, or should we not care of changes
done in quota while temporary tables are installed (iptables -L) ?

> ---
>  include/linux/netfilter/xt_limit.h     |    9 +++--
>  include/linux/netfilter/xt_statistic.h |    7 ++--
>  net/netfilter/xt_limit.c               |   40 +++++++++++++++++------
>  net/netfilter/xt_statistic.c           |   28 +++++++++++++---
>  4 files changed, 61 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/netfilter/xt_limit.h b/include/linux/netfilter/xt_limit.h
> index b3ce653..fda222c 100644
> --- a/include/linux/netfilter/xt_limit.h
> +++ b/include/linux/netfilter/xt_limit.h
> @@ -4,6 +4,8 @@
>  /* timings are in milliseconds. */
>  #define XT_LIMIT_SCALE 10000
>  
> +struct xt_limit_priv;
> +
>  /* 1/10,000 sec period => max of 10,000/sec.  Min rate is then 429490
>     seconds, or one every 59 hours. */
>  struct xt_rateinfo {
> @@ -11,11 +13,10 @@ struct xt_rateinfo {
>  	u_int32_t burst;  /* Period multiplier for upper limit. */
>  
>  	/* Used internally by the kernel */
> -	unsigned long prev;
> -	u_int32_t credit;
> +	unsigned long prev; /* moved to xt_limit_priv */
> +	u_int32_t credit; /* moved to xt_limit_priv */
>  	u_int32_t credit_cap, cost;
>  
> -	/* Ugly, ugly fucker. */
> -	struct xt_rateinfo *master;
> +	struct xt_limit_priv *master;
>  };
>  #endif /*_XT_RATE_H*/
> diff --git a/include/linux/netfilter/xt_statistic.h b/include/linux/netfilter/xt_statistic.h
> index 3d38bc9..8f521ab 100644
> --- a/include/linux/netfilter/xt_statistic.h
> +++ b/include/linux/netfilter/xt_statistic.h
> @@ -13,6 +13,8 @@ enum xt_statistic_flags {
>  };
>  #define XT_STATISTIC_MASK		0x1
>  
> +struct xt_statistic_priv;
> +
>  struct xt_statistic_info {
>  	u_int16_t			mode;
>  	u_int16_t			flags;
> @@ -23,11 +25,10 @@ struct xt_statistic_info {
>  		struct {
>  			u_int32_t	every;
>  			u_int32_t	packet;
> -			/* Used internally by the kernel */
> -			u_int32_t	count;
> +			u_int32_t	count; /* unused */
>  		} nth;
>  	} u;
> -	struct xt_statistic_info	*master __attribute__((aligned(8)));
> +	struct xt_statistic_priv *master __attribute__((aligned(8)));
>  };
>  
>  #endif /* _XT_STATISTIC_H */
> diff --git a/net/netfilter/xt_limit.c b/net/netfilter/xt_limit.c
> index c908d69..2e8089e 100644
> --- a/net/netfilter/xt_limit.c
> +++ b/net/netfilter/xt_limit.c
> @@ -14,6 +14,11 @@
>  #include <linux/netfilter/x_tables.h>
>  #include <linux/netfilter/xt_limit.h>
>  
> +struct xt_limit_priv {
> +	unsigned long prev;
> +	uint32_t credit;
> +};
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Herve Eychenne <rv@wallfire.org>");
>  MODULE_DESCRIPTION("Xtables: rate-limit match");
> @@ -60,18 +65,18 @@ static DEFINE_SPINLOCK(limit_lock);
>  static bool
>  limit_mt(const struct sk_buff *skb, const struct xt_match_param *par)
>  {
> -	struct xt_rateinfo *r =
> -		((const struct xt_rateinfo *)par->matchinfo)->master;
> +	const struct xt_rateinfo *r = par->matchinfo;
> +	struct xt_limit_priv *priv = r->master;
>  	unsigned long now = jiffies;
>  
>  	spin_lock_bh(&limit_lock);
> -	r->credit += (now - xchg(&r->prev, now)) * CREDITS_PER_JIFFY;
> -	if (r->credit > r->credit_cap)
> -		r->credit = r->credit_cap;
> +	priv->credit += (now - xchg(&priv->prev, now)) * CREDITS_PER_JIFFY;
> +	if (priv->credit > r->credit_cap)
> +		priv->credit = r->credit_cap;
>  
> -	if (r->credit >= r->cost) {
> +	if (priv->credit >= r->cost) {
>  		/* We're not limited. */
> -		r->credit -= r->cost;
> +		priv->credit -= r->cost;
>  		spin_unlock_bh(&limit_lock);
>  		return true;
>  	}
> @@ -95,6 +100,7 @@ user2credits(u_int32_t user)
>  static bool limit_mt_check(const struct xt_mtchk_param *par)
>  {
>  	struct xt_rateinfo *r = par->matchinfo;
> +	struct xt_limit_priv *priv;
>  
>  	/* Check for overflow. */
>  	if (r->burst == 0
> @@ -104,19 +110,30 @@ static bool limit_mt_check(const struct xt_mtchk_param *par)
>  		return false;
>  	}
>  
> -	/* For SMP, we only want to use one set of counters. */
> -	r->master = r;
> +	priv = kmalloc(sizeof(*priv), GFP_KERNEL);
> +	if (priv == NULL)
> +		return -ENOMEM;
> +
> +	/* For SMP, we only want to use one set of state. */
> +	r->master = priv;
>  	if (r->cost == 0) {
>  		/* User avg in seconds * XT_LIMIT_SCALE: convert to jiffies *
>  		   128. */
> -		r->prev = jiffies;
> -		r->credit = user2credits(r->avg * r->burst);	 /* Credits full. */
> +		priv->prev = jiffies;
> +		priv->credit = user2credits(r->avg * r->burst); /* Credits full. */
>  		r->credit_cap = user2credits(r->avg * r->burst); /* Credits full. */
>  		r->cost = user2credits(r->avg);
>  	}
>  	return true;
>  }
>  
> +static void limit_mt_destroy(const struct xt_mtdtor_param *par)
> +{
> +	const struct xt_rateinfo *info = par->matchinfo;
> +
> +	kfree(info->master);
> +}
> +
>  #ifdef CONFIG_COMPAT
>  struct compat_xt_rateinfo {
>  	u_int32_t avg;
> @@ -167,6 +184,7 @@ static struct xt_match limit_mt_reg __read_mostly = {
>  	.family           = NFPROTO_UNSPEC,
>  	.match            = limit_mt,
>  	.checkentry       = limit_mt_check,
> +	.destroy          = limit_mt_destroy,
>  	.matchsize        = sizeof(struct xt_rateinfo),
>  #ifdef CONFIG_COMPAT
>  	.compatsize       = sizeof(struct compat_xt_rateinfo),
> diff --git a/net/netfilter/xt_statistic.c b/net/netfilter/xt_statistic.c
> index 0d75141..d8c0f8f 100644
> --- a/net/netfilter/xt_statistic.c
> +++ b/net/netfilter/xt_statistic.c
> @@ -16,6 +16,10 @@
>  #include <linux/netfilter/xt_statistic.h>
>  #include <linux/netfilter/x_tables.h>
>  
> +struct xt_statistic_priv {
> +	uint32_t count;
> +};
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Patrick McHardy <kaber@trash.net>");
>  MODULE_DESCRIPTION("Xtables: statistics-based matching (\"Nth\", random)");
> @@ -27,7 +31,7 @@ static DEFINE_SPINLOCK(nth_lock);
>  static bool
>  statistic_mt(const struct sk_buff *skb, const struct xt_match_param *par)
>  {
> -	struct xt_statistic_info *info = (void *)par->matchinfo;
> +	const struct xt_statistic_info *info = par->matchinfo;
>  	bool ret = info->flags & XT_STATISTIC_INVERT;
>  
>  	switch (info->mode) {
> @@ -36,10 +40,9 @@ statistic_mt(const struct sk_buff *skb, const struct xt_match_param *par)
>  			ret = !ret;
>  		break;
>  	case XT_STATISTIC_MODE_NTH:
> -		info = info->master;
>  		spin_lock_bh(&nth_lock);
> -		if (info->u.nth.count++ == info->u.nth.every) {
> -			info->u.nth.count = 0;
> +		if (info->master->count++ == info->u.nth.every) {
> +			info->master->count = 0;
>  			ret = !ret;
>  		}
>  		spin_unlock_bh(&nth_lock);
> @@ -56,16 +59,31 @@ static bool statistic_mt_check(const struct xt_mtchk_param *par)
>  	if (info->mode > XT_STATISTIC_MODE_MAX ||
>  	    info->flags & ~XT_STATISTIC_MASK)
>  		return false;
> -	info->master = info;
> +
> +	info->master = kzalloc(sizeof(*info->master), GFP_KERNEL);
> +	if (info->master == NULL) {
> +		printk(KERN_ERR KBUILD_MODNAME ": Out of memory\n");
> +		return false;
> +	}
> +	info->master->count = info->u.nth.count;
> +
>  	return true;
>  }
>  
> +static void statistic_mt_destroy(const struct xt_mtdtor_param *par)
> +{
> +	const struct xt_statistic_info *info = par->matchinfo;
> +
> +	kfree(info->master);
> +}
> +
>  static struct xt_match xt_statistic_mt_reg __read_mostly = {
>  	.name       = "statistic",
>  	.revision   = 0,
>  	.family     = NFPROTO_UNSPEC,
>  	.match      = statistic_mt,
>  	.checkentry = statistic_mt_check,
> +	.destroy    = statistic_mt_destroy,
>  	.matchsize  = sizeof(struct xt_statistic_info),
>  	.me         = THIS_MODULE,
>  };


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt - Feb. 28, 2009, 8:22 a.m.
On Saturday 2009-02-28 07:56, Eric Dumazet wrote:
>> commit 3e7ee1dcc808b8eec82eddfa0436f78e31d2004a
>> Author: Jan Engelhardt <jengelh@medozas.de>
>> Date:   Sat Feb 28 02:49:28 2009 +0100
>> 
>> netfilter: xtables: avoid pointer to self
>
>Seems good to me ! Thanks Jan !
>
>Reviewed-by: Eric Dumazet <dada1@cosmosbay.com>
>
>Are you sure xt_quota doesnt need some tweak, or should we not care of changes
>done in quota while temporary tables are installed (iptables -L) ?

Oh right, xt_quota.. thanks for noticing! Patch will follow.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/netfilter/xt_limit.h b/include/linux/netfilter/xt_limit.h
index b3ce653..fda222c 100644
--- a/include/linux/netfilter/xt_limit.h
+++ b/include/linux/netfilter/xt_limit.h
@@ -4,6 +4,8 @@ 
 /* timings are in milliseconds. */
 #define XT_LIMIT_SCALE 10000
 
+struct xt_limit_priv;
+
 /* 1/10,000 sec period => max of 10,000/sec.  Min rate is then 429490
    seconds, or one every 59 hours. */
 struct xt_rateinfo {
@@ -11,11 +13,10 @@  struct xt_rateinfo {
 	u_int32_t burst;  /* Period multiplier for upper limit. */
 
 	/* Used internally by the kernel */
-	unsigned long prev;
-	u_int32_t credit;
+	unsigned long prev; /* moved to xt_limit_priv */
+	u_int32_t credit; /* moved to xt_limit_priv */
 	u_int32_t credit_cap, cost;
 
-	/* Ugly, ugly fucker. */
-	struct xt_rateinfo *master;
+	struct xt_limit_priv *master;
 };
 #endif /*_XT_RATE_H*/
diff --git a/include/linux/netfilter/xt_statistic.h b/include/linux/netfilter/xt_statistic.h
index 3d38bc9..8f521ab 100644
--- a/include/linux/netfilter/xt_statistic.h
+++ b/include/linux/netfilter/xt_statistic.h
@@ -13,6 +13,8 @@  enum xt_statistic_flags {
 };
 #define XT_STATISTIC_MASK		0x1
 
+struct xt_statistic_priv;
+
 struct xt_statistic_info {
 	u_int16_t			mode;
 	u_int16_t			flags;
@@ -23,11 +25,10 @@  struct xt_statistic_info {
 		struct {
 			u_int32_t	every;
 			u_int32_t	packet;
-			/* Used internally by the kernel */
-			u_int32_t	count;
+			u_int32_t	count; /* unused */
 		} nth;
 	} u;
-	struct xt_statistic_info	*master __attribute__((aligned(8)));
+	struct xt_statistic_priv *master __attribute__((aligned(8)));
 };
 
 #endif /* _XT_STATISTIC_H */
diff --git a/net/netfilter/xt_limit.c b/net/netfilter/xt_limit.c
index c908d69..2e8089e 100644
--- a/net/netfilter/xt_limit.c
+++ b/net/netfilter/xt_limit.c
@@ -14,6 +14,11 @@ 
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_limit.h>
 
+struct xt_limit_priv {
+	unsigned long prev;
+	uint32_t credit;
+};
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Herve Eychenne <rv@wallfire.org>");
 MODULE_DESCRIPTION("Xtables: rate-limit match");
@@ -60,18 +65,18 @@  static DEFINE_SPINLOCK(limit_lock);
 static bool
 limit_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 {
-	struct xt_rateinfo *r =
-		((const struct xt_rateinfo *)par->matchinfo)->master;
+	const struct xt_rateinfo *r = par->matchinfo;
+	struct xt_limit_priv *priv = r->master;
 	unsigned long now = jiffies;
 
 	spin_lock_bh(&limit_lock);
-	r->credit += (now - xchg(&r->prev, now)) * CREDITS_PER_JIFFY;
-	if (r->credit > r->credit_cap)
-		r->credit = r->credit_cap;
+	priv->credit += (now - xchg(&priv->prev, now)) * CREDITS_PER_JIFFY;
+	if (priv->credit > r->credit_cap)
+		priv->credit = r->credit_cap;
 
-	if (r->credit >= r->cost) {
+	if (priv->credit >= r->cost) {
 		/* We're not limited. */
-		r->credit -= r->cost;
+		priv->credit -= r->cost;
 		spin_unlock_bh(&limit_lock);
 		return true;
 	}
@@ -95,6 +100,7 @@  user2credits(u_int32_t user)
 static bool limit_mt_check(const struct xt_mtchk_param *par)
 {
 	struct xt_rateinfo *r = par->matchinfo;
+	struct xt_limit_priv *priv;
 
 	/* Check for overflow. */
 	if (r->burst == 0
@@ -104,19 +110,30 @@  static bool limit_mt_check(const struct xt_mtchk_param *par)
 		return false;
 	}
 
-	/* For SMP, we only want to use one set of counters. */
-	r->master = r;
+	priv = kmalloc(sizeof(*priv), GFP_KERNEL);
+	if (priv == NULL)
+		return -ENOMEM;
+
+	/* For SMP, we only want to use one set of state. */
+	r->master = priv;
 	if (r->cost == 0) {
 		/* User avg in seconds * XT_LIMIT_SCALE: convert to jiffies *
 		   128. */
-		r->prev = jiffies;
-		r->credit = user2credits(r->avg * r->burst);	 /* Credits full. */
+		priv->prev = jiffies;
+		priv->credit = user2credits(r->avg * r->burst); /* Credits full. */
 		r->credit_cap = user2credits(r->avg * r->burst); /* Credits full. */
 		r->cost = user2credits(r->avg);
 	}
 	return true;
 }
 
+static void limit_mt_destroy(const struct xt_mtdtor_param *par)
+{
+	const struct xt_rateinfo *info = par->matchinfo;
+
+	kfree(info->master);
+}
+
 #ifdef CONFIG_COMPAT
 struct compat_xt_rateinfo {
 	u_int32_t avg;
@@ -167,6 +184,7 @@  static struct xt_match limit_mt_reg __read_mostly = {
 	.family           = NFPROTO_UNSPEC,
 	.match            = limit_mt,
 	.checkentry       = limit_mt_check,
+	.destroy          = limit_mt_destroy,
 	.matchsize        = sizeof(struct xt_rateinfo),
 #ifdef CONFIG_COMPAT
 	.compatsize       = sizeof(struct compat_xt_rateinfo),
diff --git a/net/netfilter/xt_statistic.c b/net/netfilter/xt_statistic.c
index 0d75141..d8c0f8f 100644
--- a/net/netfilter/xt_statistic.c
+++ b/net/netfilter/xt_statistic.c
@@ -16,6 +16,10 @@ 
 #include <linux/netfilter/xt_statistic.h>
 #include <linux/netfilter/x_tables.h>
 
+struct xt_statistic_priv {
+	uint32_t count;
+};
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Patrick McHardy <kaber@trash.net>");
 MODULE_DESCRIPTION("Xtables: statistics-based matching (\"Nth\", random)");
@@ -27,7 +31,7 @@  static DEFINE_SPINLOCK(nth_lock);
 static bool
 statistic_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 {
-	struct xt_statistic_info *info = (void *)par->matchinfo;
+	const struct xt_statistic_info *info = par->matchinfo;
 	bool ret = info->flags & XT_STATISTIC_INVERT;
 
 	switch (info->mode) {
@@ -36,10 +40,9 @@  statistic_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 			ret = !ret;
 		break;
 	case XT_STATISTIC_MODE_NTH:
-		info = info->master;
 		spin_lock_bh(&nth_lock);
-		if (info->u.nth.count++ == info->u.nth.every) {
-			info->u.nth.count = 0;
+		if (info->master->count++ == info->u.nth.every) {
+			info->master->count = 0;
 			ret = !ret;
 		}
 		spin_unlock_bh(&nth_lock);
@@ -56,16 +59,31 @@  static bool statistic_mt_check(const struct xt_mtchk_param *par)
 	if (info->mode > XT_STATISTIC_MODE_MAX ||
 	    info->flags & ~XT_STATISTIC_MASK)
 		return false;
-	info->master = info;
+
+	info->master = kzalloc(sizeof(*info->master), GFP_KERNEL);
+	if (info->master == NULL) {
+		printk(KERN_ERR KBUILD_MODNAME ": Out of memory\n");
+		return false;
+	}
+	info->master->count = info->u.nth.count;
+
 	return true;
 }
 
+static void statistic_mt_destroy(const struct xt_mtdtor_param *par)
+{
+	const struct xt_statistic_info *info = par->matchinfo;
+
+	kfree(info->master);
+}
+
 static struct xt_match xt_statistic_mt_reg __read_mostly = {
 	.name       = "statistic",
 	.revision   = 0,
 	.family     = NFPROTO_UNSPEC,
 	.match      = statistic_mt,
 	.checkentry = statistic_mt_check,
+	.destroy    = statistic_mt_destroy,
 	.matchsize  = sizeof(struct xt_statistic_info),
 	.me         = THIS_MODULE,
 };