Message ID | 1324645592.2223.9.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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
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
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 --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 */