[PATCH] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[PATCH] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

Setting a large rps_flow_cnt like 1073741824 (1 << 30) on 32-bit
platform will cause a kernel oops due to insufficient bounds checking.

	if (count > 1<<30) {
		/* Enforce a limit to prevent overflow */
		return -EINVAL;
	}
	count = roundup_pow_of_two(count);
	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));

Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:

	... + (count * sizeof(struct rps_dev_flow))

where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
32 bits.  This patch changes the upper bound to (1 << 28).

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/core/net-sysfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index c71c434..f53a947 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -665,7 +665,7 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 	if (count) {
 		int i;
 
-		if (count > 1<<30) {
+		if (count > 1<<28) {
 			/* Enforce a limit to prevent overflow */
 			return -EINVAL;
 		}
-- 
1.7.5.4

Re: [PATCH] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Eric Dumazet]

Le mercredi 21 décembre 2011 à 13:50 -0500, Xi Wang a écrit :
> Setting a large rps_flow_cnt like 1073741824 (1 << 30) on 32-bit
> platform will cause a kernel oops due to insufficient bounds checking.
> 
> 	if (count > 1<<30) {
> 		/* Enforce a limit to prevent overflow */
> 		return -EINVAL;
> 	}
> 	count = roundup_pow_of_two(count);
> 	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
> 
> Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:
> 
> 	... + (count * sizeof(struct rps_dev_flow))
> 
> where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
> 32 bits.  This patch changes the upper bound to (1 << 28).
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  net/core/net-sysfs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index c71c434..f53a947 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -665,7 +665,7 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
>  	if (count) {
>  		int i;
>  
> -		if (count > 1<<30) {
> +		if (count > 1<<28) {
>  			/* Enforce a limit to prevent overflow */
>  			return -EINVAL;
>  		}


Really, you should remove this magic number and use instead

(INT_MAX - RPS_DEV_FLOW_TABLE_SIZE(0)) / sizeof(struct rps_dev_flow)

Or something like that, because next time we add a field in
rps_dev_flow, test will be obsolete.

Re: [PATCH] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 21 Dec 2011 20:22:24 +0100

> Le mercredi 21 décembre 2011 à 13:50 -0500, Xi Wang a écrit :
>> @@ -665,7 +665,7 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
>>  	if (count) {
>>  		int i;
>>  
>> -		if (count > 1<<30) {
>> +		if (count > 1<<28) {
>>  			/* Enforce a limit to prevent overflow */
>>  			return -EINVAL;
>>  		}
> 
> 
> Really, you should remove this magic number and use instead
> 
> (INT_MAX - RPS_DEV_FLOW_TABLE_SIZE(0)) / sizeof(struct rps_dev_flow)
> 
> Or something like that, because next time we add a field in
> rps_dev_flow, test will be obsolete.

Agreed.

Re: [PATCH] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

Agreed too. ;-)  Thanks for the suggestion.

I will do a v2 without using the magic number.

BTW, a similar magic number (1<<30) is used in rps_sock_flow_sysctl()
at net/core/sysctl_net_core.c.  I will change that as well.

- xi

[PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

Setting a large rps_flow_cnt like (1 << 30) on 32-bit platform will
cause a kernel oops due to insufficient bounds checking.

	if (count > 1<<30) {
		/* Enforce a limit to prevent overflow */
		return -EINVAL;
	}
	count = roundup_pow_of_two(count);
	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));

Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:

	... + (count * sizeof(struct rps_dev_flow))

where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
32 bits.

This patch replaces the magic number (1 << 30) with a symbolic bound.

Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/core/net-sysfs.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index c71c434..385aefe 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -665,11 +665,14 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 	if (count) {
 		int i;
 
-		if (count > 1<<30) {
+		if (count > INT_MAX)
+			return -EINVAL;
+		count = roundup_pow_of_two(count);
+		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
+				/ sizeof(struct rps_dev_flow)) {
 			/* Enforce a limit to prevent overflow */
 			return -EINVAL;
 		}
-		count = roundup_pow_of_two(count);
 		table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
 		if (!table)
 			return -ENOMEM;
-- 
1.7.5.4

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[David Miller]

From: Xi Wang <xi.wang@gmail.com>
Date: Thu, 22 Dec 2011 18:35:22 -0500

> Setting a large rps_flow_cnt like (1 << 30) on 32-bit platform will
> cause a kernel oops due to insufficient bounds checking.
> 
> 	if (count > 1<<30) {
> 		/* Enforce a limit to prevent overflow */
> 		return -EINVAL;
> 	}
> 	count = roundup_pow_of_two(count);
> 	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
> 
> Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:
> 
> 	... + (count * sizeof(struct rps_dev_flow))
> 
> where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
> 32 bits.
> 
> This patch replaces the magic number (1 << 30) with a symbolic bound.
> 
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Applied.

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Eric Dumazet]

Le jeudi 22 décembre 2011 à 18:35 -0500, Xi Wang a écrit :
> Setting a large rps_flow_cnt like (1 << 30) on 32-bit platform will
> cause a kernel oops due to insufficient bounds checking.
> 
> 	if (count > 1<<30) {
> 		/* Enforce a limit to prevent overflow */
> 		return -EINVAL;
> 	}
> 	count = roundup_pow_of_two(count);
> 	table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
> 
> Note that the macro RPS_DEV_FLOW_TABLE_SIZE(count) is defined as:
> 
> 	... + (count * sizeof(struct rps_dev_flow))
> 
> where sizeof(struct rps_dev_flow) is 8.  (1 << 30) * 8 will overflow
> 32 bits.
> 
> This patch replaces the magic number (1 << 30) with a symbolic bound.
> 
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  net/core/net-sysfs.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index c71c434..385aefe 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -665,11 +665,14 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
>  	if (count) {
>  		int i;
>  
> -		if (count > 1<<30) {
> +		if (count > INT_MAX)
> +			return -EINVAL;
> +		count = roundup_pow_of_two(count);
> +		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))

Oh well, you added a bug here, since count is "unsigned int"

Why mixing INT_MAX in the previous test and ULONG_MAX here ?


> +				/ sizeof(struct rps_dev_flow)) {
>  			/* Enforce a limit to prevent overflow */
>  			return -EINVAL;
>  		}
> -		count = roundup_pow_of_two(count);
>  		table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
>  		if (!table)
>  			return -ENOMEM;

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

On Dec 22, 2011, at 11:10 PM, Eric Dumazet wrote:
>> -		if (count > 1<<30) {
>> +		if (count > INT_MAX)
>> +			return -EINVAL;
>> +		count = roundup_pow_of_two(count);
>> +		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> 
> Oh well, you added a bug here, since count is "unsigned int"
> 
> Why mixing INT_MAX in the previous test and ULONG_MAX here ?

The first check (count > INT_MAX) is used to avoid overflowing
roundup_pow_of_two(count), e.g., when count is 0xffffffff.

The second check is to avoid overflowing the size for vmalloc().
It gives a less restrictive upper bound than using INT_MAX/UINT_MAX
on 64-bit platform.

Why do you think it's a bug there?  I agree using two checks looks
ugly though.

- xi

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Eric Dumazet]

Le jeudi 22 décembre 2011 à 23:44 -0500, Xi Wang a écrit :
> On Dec 22, 2011, at 11:10 PM, Eric Dumazet wrote:
> >> -		if (count > 1<<30) {
> >> +		if (count > INT_MAX)
> >> +			return -EINVAL;
> >> +		count = roundup_pow_of_two(count);
> >> +		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> > 
> > Oh well, you added a bug here, since count is "unsigned int"
> > 
> > Why mixing INT_MAX in the previous test and ULONG_MAX here ?
> 
> The first check (count > INT_MAX) is used to avoid overflowing
> roundup_pow_of_two(count), e.g., when count is 0xffffffff.
> 
> The second check is to avoid overflowing the size for vmalloc().
> It gives a less restrictive upper bound than using INT_MAX/UINT_MAX
> on 64-bit platform.
> 
> Why do you think it's a bug there?  I agree using two checks looks
> ugly though.
> 
> - xi

All I wanted to say is that while mixing INT_MAX/ULONG_MAX, you could
have spotted the other bug in the code :

unsigned int count;

count = simple_strtoul(buf, &endp, 0);

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

On Dec 22, 2011, at 11:53 PM, Eric Dumazet wrote:
> All I wanted to say is that while mixing INT_MAX/ULONG_MAX, you could
> have spotted the other bug in the code :
> 
> unsigned int count;
> 
> count = simple_strtoul(buf, &endp, 0);

Are you suggesting to change the type of "count" to unsigned long?
That seems like a separate issue from the bounds checking part.

I don't see the possible 32-bit truncation here is a serious issue though.
The user could even echo "128abc" into rps_flow_cnt and simple_strtoul()
would be happy to pick 128.

- xi

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Eric Dumazet]

Le vendredi 23 décembre 2011 à 00:10 -0500, Xi Wang a écrit :
> On Dec 22, 2011, at 11:53 PM, Eric Dumazet wrote:
> > All I wanted to say is that while mixing INT_MAX/ULONG_MAX, you could
> > have spotted the other bug in the code :
> > 
> > unsigned int count;
> > 
> > count = simple_strtoul(buf, &endp, 0);
> 
> Are you suggesting to change the type of "count" to unsigned long?
> That seems like a separate issue from the bounds checking part.
> 
> I don't see the possible 32-bit truncation here is a serious issue though.
> The user could even echo "128abc" into rps_flow_cnt and simple_strtoul()
> would be happy to pick 128.
> 

32 bit truncation _is_ a bound checking problem too.

Really, mixing INT_MAX / ULONG_MAX is ugly, this should had ring a bell
when writing such hard to read code.

You cannot claim to give more range to 64bit platform, yet not spotting
the 32bit truncation issue.

When fixing a bug, its always a good thing to look things around, and
try to check the whole function.

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Eric Dumazet]

Le vendredi 23 décembre 2011 à 06:16 +0100, Eric Dumazet a écrit :

> 32 bit truncation _is_ a bound checking problem too.
> 
> Really, mixing INT_MAX / ULONG_MAX is ugly, this should had ring a bell
> when writing such hard to read code.
> 
> You cannot claim to give more range to 64bit platform, yet not spotting
> the 32bit truncation issue.
> 
> When fixing a bug, its always a good thing to look things around, and
> try to check the whole function.
> 
> 

By the way, the theorical limit on number of flows on 64bit platform is
2^32  (rxhash being an u32)

Not sure spending 32GB per table would be wise for typical machines :)

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

On Dec 23, 2011, at 12:35 AM, Eric Dumazet wrote:
> By the way, the theorical limit on number of flows on 64bit platform is
> 2^32  (rxhash being an u32)
> 
> Not sure spending 32GB per table would be wise for typical machines :)

True, a large rps_flow_cnt is not that useful. ;-)

Anyway, my point is that if the patch looks confusing to you, then
it is probably not good.  I am happy to see a more elegant fix.

- xi

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Eric Dumazet]

Le vendredi 23 décembre 2011 à 00:56 -0500, Xi Wang a écrit :
> On Dec 23, 2011, at 12:35 AM, Eric Dumazet wrote:
> > By the way, the theorical limit on number of flows on 64bit platform is
> > 2^32  (rxhash being an u32)
> > 
> > Not sure spending 32GB per table would be wise for typical machines :)
> 
> True, a large rps_flow_cnt is not that useful. ;-)
> 
> Anyway, my point is that if the patch looks confusing to you, then
> it is probably not good.  I am happy to see a more elegant fix.
> 

I'll submit following patch for net-next, once your patch is in this
tree.

- Full u32 range on 64bit arches (2^32 flows max)
- Use of kstrtoul() instead of simple_strtoul()

 net/core/net-sysfs.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 385aefe..63bd152 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -618,7 +618,7 @@ static ssize_t show_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 					   char *buf)
 {
 	struct rps_dev_flow_table *flow_table;
-	unsigned int val = 0;
+	unsigned long val = 0;
 
 	rcu_read_lock();
 	flow_table = rcu_dereference(queue->rps_flow_table);
@@ -626,7 +626,7 @@ static ssize_t show_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 		val = flow_table->mask + 1;
 	rcu_read_unlock();
 
-	return sprintf(buf, "%u\n", val);
+	return sprintf(buf, "%lu\n", val);
 }
 
 static void rps_dev_flow_table_release_work(struct work_struct *work)
@@ -650,24 +650,23 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 				     struct rx_queue_attribute *attr,
 				     const char *buf, size_t len)
 {
-	unsigned int count;
-	char *endp;
+	unsigned long i, count;
 	struct rps_dev_flow_table *table, *old_table;
 	static DEFINE_SPINLOCK(rps_dev_flow_lock);
+	int rc;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	count = simple_strtoul(buf, &endp, 0);
-	if (endp == buf)
-		return -EINVAL;
+	rc = kstrtoul(buf, 0, &count);
+	if (rc < 0)
+		return rc;
 
 	if (count) {
-		int i;
-
-		if (count > INT_MAX)
-			return -EINVAL;
 		count = roundup_pow_of_two(count);
+		if (!count ||
+		    count != (unsigned long)(u32)count)
+			return -EINVAL;
 		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
 				/ sizeof(struct rps_dev_flow)) {
 			/* Enforce a limit to prevent overflow */

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

On Dec 23, 2011, at 8:06 AM, Eric Dumazet wrote:

> I'll submit following patch for net-next, once your patch is in this
> tree.

Thanks for doing this. ;-)

> 		count = roundup_pow_of_two(count);
> +		if (!count ||
> +		    count != (unsigned long)(u32)count)
> +			return -EINVAL;
> 		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> 				/ sizeof(struct rps_dev_flow)) {
> 			/* Enforce a limit to prevent overflow */

I would rather avoid undefined behavior in C.

Given count = ULONG_MAX on 64-bit systems, roundup_pow_of_two()
would overflow, and the overflowed result is undefined, e.g.,
on x86-64 it gives 1, not 0.  That's why I used INT_MAX.

BTW, (count > UINT_MAX) is shorter and more easier to understand
than (count != (unsigned long)(u32)count).

- xi

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Eric Dumazet]

Le vendredi 23 décembre 2011 à 09:30 -0500, Xi Wang a écrit :
> On Dec 23, 2011, at 8:06 AM, Eric Dumazet wrote:
> 
> > I'll submit following patch for net-next, once your patch is in this
> > tree.
> 
> Thanks for doing this. ;-)
> 
> > 		count = roundup_pow_of_two(count);
> > +		if (!count ||
> > +		    count != (unsigned long)(u32)count)
> > +			return -EINVAL;
> > 		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> > 				/ sizeof(struct rps_dev_flow)) {
> > 			/* Enforce a limit to prevent overflow */
> 
> I would rather avoid undefined behavior in C.
> 

Unsigned arithmetics is well defined in C. Very well in fact.


> Given count = ULONG_MAX on 64-bit systems, roundup_pow_of_two()
> would overflow, and the overflowed result is undefined, e.g.,
> on x86-64 it gives 1, not 0.  That's why I used INT_MAX.
> 

I'll check roundup_pow_of_two(), this seems a bug to me.

Problem is : INT_MAX doesnt allow the full range of rps : 2^32 flows.

> BTW, (count > UINT_MAX) is shorter and more easier to understand
> than (count != (unsigned long)(u32)count).

You miss the point. UINT_MAX is too small for 64bit arches.

if (count != (unsigned long)(u32)count)

is pretty common stuff in kernel.

Check _kstrtoul() for an example.

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

On Dec 23, 2011, at 9:53 AM, Eric Dumazet wrote:
> Unsigned arithmetics is well defined in C. Very well in fact.

Oversized shifts are undefined in C, no matter it is unsigned
or not; roundup_pow_of_two() is implemented by shifting.

>> BTW, (count > UINT_MAX) is shorter and easier to understand
>> than (count != (unsigned long)(u32)count).
> 
> You miss the point. UINT_MAX is too small for 64bit arches.

I am sorry why is it too small?  what's difference between
(count > UINT_MAX) and (count != (unsigned long)(u32)count)?

- xi

[PATCH net-next] rfs: better sizing of dev_flow_table

[Eric Dumazet]

Aim of this patch is to provide full range of rps_flow_cnt on 64bit arches.

Theorical limit on number of flows is 2^32

Fix some buggy RPS/RFS macros as well.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Tom Herbert <therbert@google.com>
CC: Xi Wang <xi.wang@gmail.com>
CC: Laurent Chavey <chavey@google.com>
---
 include/linux/netdevice.h |    8 +++---
 net/core/net-sysfs.c      |   44 ++++++++++++++++++++++--------------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 6037308..a776a67 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -597,7 +597,7 @@ struct rps_map {
 	struct rcu_head rcu;
 	u16 cpus[0];
 };
-#define RPS_MAP_SIZE(_num) (sizeof(struct rps_map) + (_num * sizeof(u16)))
+#define RPS_MAP_SIZE(_num) (sizeof(struct rps_map) + ((_num) * sizeof(u16)))
 
 /*
  * The rps_dev_flow structure contains the mapping of a flow to a CPU, the
@@ -621,7 +621,7 @@ struct rps_dev_flow_table {
 	struct rps_dev_flow flows[0];
 };
 #define RPS_DEV_FLOW_TABLE_SIZE(_num) (sizeof(struct rps_dev_flow_table) + \
-    (_num * sizeof(struct rps_dev_flow)))
+    ((_num) * sizeof(struct rps_dev_flow)))
 
 /*
  * The rps_sock_flow_table contains mappings of flows to the last CPU
@@ -632,7 +632,7 @@ struct rps_sock_flow_table {
 	u16 ents[0];
 };
 #define	RPS_SOCK_FLOW_TABLE_SIZE(_num) (sizeof(struct rps_sock_flow_table) + \
-    (_num * sizeof(u16)))
+    ((_num) * sizeof(u16)))
 
 #define RPS_NO_CPU 0xffff
 
@@ -684,7 +684,7 @@ struct xps_map {
 	struct rcu_head rcu;
 	u16 queues[0];
 };
-#define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + (_num * sizeof(u16)))
+#define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
 #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))	\
     / sizeof(u16))
 
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4b4d0b0..abf4393 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -622,15 +622,15 @@ static ssize_t show_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 					   char *buf)
 {
 	struct rps_dev_flow_table *flow_table;
-	unsigned int val = 0;
+	unsigned long val = 0;
 
 	rcu_read_lock();
 	flow_table = rcu_dereference(queue->rps_flow_table);
 	if (flow_table)
-		val = flow_table->mask + 1;
+		val = (unsigned long)flow_table->mask + 1;
 	rcu_read_unlock();
 
-	return sprintf(buf, "%u\n", val);
+	return sprintf(buf, "%lu\n", val);
 }
 
 static void rps_dev_flow_table_release_work(struct work_struct *work)
@@ -654,36 +654,46 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 				     struct rx_queue_attribute *attr,
 				     const char *buf, size_t len)
 {
-	unsigned int count;
-	char *endp;
+	unsigned long mask, count;
 	struct rps_dev_flow_table *table, *old_table;
 	static DEFINE_SPINLOCK(rps_dev_flow_lock);
+	int rc;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	count = simple_strtoul(buf, &endp, 0);
-	if (endp == buf)
-		return -EINVAL;
+	rc = kstrtoul(buf, 0, &count);
+	if (rc < 0)
+		return rc;
 
 	if (count) {
-		int i;
-
-		if (count > INT_MAX)
+		mask = count - 1;
+		/* mask = roundup_pow_of_two(count) - 1;
+		 * without overflows...
+		 */
+		while ((mask | (mask >> 1)) != mask)
+			mask |= (mask >> 1);
+		/* On 64 bit arches, must check mask fits in table->mask (u32),
+		 * and on 32bit arches, must check RPS_DEV_FLOW_TABLE_SIZE(mask + 1)
+		 * doesnt overflow.
+		 */
+#if BITS_PER_LONG > 32
+		if (mask > (unsigned long)(u32)mask)
 			return -EINVAL;
-		count = roundup_pow_of_two(count);
-		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
+#else
+		if (mask > (ULONG_MAX - RPS_DEV_FLOW_TABLE_SIZE(1))
 				/ sizeof(struct rps_dev_flow)) {
 			/* Enforce a limit to prevent overflow */
 			return -EINVAL;
 		}
-		table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(count));
+#endif
+		table = vmalloc(RPS_DEV_FLOW_TABLE_SIZE(mask + 1));
 		if (!table)
 			return -ENOMEM;
 
-		table->mask = count - 1;
-		for (i = 0; i < count; i++)
-			table->flows[i].cpu = RPS_NO_CPU;
+		table->mask = mask;
+		for (count = 0; count <= mask; count++)
+			table->flows[count].cpu = RPS_NO_CPU;
 	} else
 		table = NULL;
 

Re: [PATCH net-next] rfs: better sizing of dev_flow_table

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 24 Dec 2011 17:56:49 +0100

> Aim of this patch is to provide full range of rps_flow_cnt on 64bit arches.
> 
> Theorical limit on number of flows is 2^32
> 
> Fix some buggy RPS/RFS macros as well.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Tom Herbert <therbert@google.com>
> CC: Xi Wang <xi.wang@gmail.com>
> CC: Laurent Chavey <chavey@google.com>

Applied, thanks.

Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()

[Xi Wang]

On Dec 23, 2011, at 12:16 AM, Eric Dumazet wrote:
> 32 bit truncation _is_ a bound checking problem too.
> 
> Really, mixing INT_MAX / ULONG_MAX is ugly, this should had ring a bell
> when writing such hard to read code.
> 
> You cannot claim to give more range to 64bit platform, yet not spotting
> the 32bit truncation issue.
> 
> When fixing a bug, its always a good thing to look things around, and
> try to check the whole function.

Okay, seems like we are talking about two different issues here.

1) the truncation of "count" on 64-bit platform.

I don't mind if the user shoots himself in the foot by setting a
large or invalid rps_flow_cnt, as long as the kernel is not affected.
I do agree with you that the kernel could be more friendly and
reject the value rather than picking up part of it.  To be nicer
the kernel may even want to reject something like "123abc" that
simple_strtoul() would accept.  I am happy to see another patch
fixing that.

2) the bounds checking of the vmalloc() size on 32-bit platform.

I agree that the two checks are ugly. ;-)  Do you want to use INT_MAX
in both checks, or do you have something more elegant in mind?

- xi