diff mbox

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

Message ID 1324645592.2223.9.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Dec. 23, 2011, 1:06 p.m. UTC 
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(-)



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

Comments

Xi Wang Dec. 23, 2011, 2:30 p.m. UTC | #1 
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
--
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
Eric Dumazet Dec. 23, 2011, 2:53 p.m. UTC | #2 
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.



--
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
Xi Wang Dec. 23, 2011, 3:07 p.m. UTC | #3 
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
--
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
diff mbox

Patch

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 */