Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core
On Fri, 4 Aug 2017, Colin Percival wrote: On 08/03/17 23:28, Hans Petter Selasky wrote: On 08/03/17 16:37, Conrad Meyer wrote: Is it not important that the subtraction and result are evaluated without truncation? ticks is a circular counter. Assuming time = 0 and jiffies = -1U, then "delay" becomes a very large negative value, because long is used, and the delay <= 0 check, is no longer working like expected. Casting to "int" or truncating is the right thing to do in this case. Signed integer overflow is undefined. Using 'int' is liable to cause problems after 2^32 ticks. There is no (new) overflow here. Converting a higher rank type to int has implementation-defined behaviour. I have yet to see an implementation that defines its behaviour except in its source code, but all implementations that I have seen do the right thing of truncating 2's complement integers. Perverse implementations would produce specific garbage values or perhaps rand(). They just have to document this. 'int overflows after 2^31-1 ticks if int is 32 bits. It is using u_int that is liable to cause problems after 2^32 ticks. Using long and u_long causes exactly the same problems if long is 32 bits. Modified: head/sys/ofed/drivers/infiniband/core/addr.c == --- head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:14:43 2017(r321984) +++ head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:18:25 2017(r321985) @@ -187,10 +187,10 @@ EXPORT_SYMBOL(rdma_translate_ip); static void set_timeout(unsigned long time) { - unsigned long delay; + int delay; /* under FreeBSD ticks are 32-bit */ delay = time - jiffies; There is no overflow here. The RHS is calculated in u_long arithmetic which has truncation instead of overflow. First, jiffies is converted to u_long in a standards-defined way. If jiffies is non-negative, then its value is unchanged by the conversion. I think jiffies can't be negative, but if it is then there are complications -- the value must be changed; it is changed by adding a multiple of ULONG_MAX which is normally 1, but it not easy to see that the multiple is always 1 (if that uis the case). Then the subtraction can't overflow, but it produces a garbage result of time < jiffies. As usual, using unsigned is a bug. Here it breaks calculations of negative differences. - if ((long)delay <= 0) + if (delay <= 0) delay = 1; Then we cast to long in the old code, and assign the RHS to int in the new code. Both are implementation-defined. The cast does nothing different from assignment except it might change compiler warnings. The old code seems to be correct enough. Suppose time is 59 and jiffies is 60. Then time - jiffies is 0xLU on 32-bit systems and 0xLU on 64-bit systems. If the implementation-defined behaviour were documented, then we would know that the result of casting either of these to long is -1L. This is < 0, so delay gets fixed up to +1. Similarly with the new code -- the assignment converts the huge unsigned values to -1. Problems with the type mismatch might occur later, but I don't see any. On 64-bit systems, the difference can be really huge without tripping the (long)delay <= 0 test. Above INT_MAX, later truncation to int in APIs using ticks will give garbage (possibly 0 or negative). I guess that is the problem. But even on 32-bit systems, the difference can be INT_MAX and that seems too large to be correct. If jiffies are 1/1000 seconds, then it is thewell-known magic number 24+ days. User code might generate that, but kernel driver code shouldn't. Bruce ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core
On Thu, Aug 3, 2017 at 11:40 PM, Colin Percival wrote: > On 08/03/17 23:28, Hans Petter Selasky wrote: >> On 08/03/17 16:37, Conrad Meyer wrote: >>> Is it not important that the subtraction and result are evaluated >>> without truncation? >> >> ticks is a circular counter. Assuming time = 0 and jiffies = -1U, then >> "delay" >> becomes a very large negative value, because long is used, and the delay <= 0 >> check, is no longer working like expected. >> >> Casting to "int" or truncating is the right thing to do in this case. > > Signed integer overflow is undefined. Using 'int' is liable to cause problems > after 2^32 ticks. It is undefined in C, but defined in practice with -fwrapv, which the kernel relies upon already. Best, Conrad ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core
On 08/04/17 08:40, Colin Percival wrote: Casting to "int" or truncating is the right thing to do in this case. Signed integer overflow is undefined. Using 'int' is liable to cause problems after 2^32 ticks. Hi, If you check the code how this function is used, you'll see that the argument passed is computed from: jiffies + n, where "n" is a relatively small value. The <= 0 check is basically there to catch cases where the jiffies value has changed between the point where it was set and the point where the relative ticks timeout value "n" was extracted. Basically the code is doing like this, reading the value of "jiffies" twice. delay = (jiffies0 + n) - jiffies1; if (delay <= 0) delay = 1; And then "delay" should be "int", because we are usually not dealing with timeouts greater than a few hundred seconds or so. --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core
On 08/03/17 23:28, Hans Petter Selasky wrote: > On 08/03/17 16:37, Conrad Meyer wrote: >> Is it not important that the subtraction and result are evaluated >> without truncation? > > ticks is a circular counter. Assuming time = 0 and jiffies = -1U, then "delay" > becomes a very large negative value, because long is used, and the delay <= 0 > check, is no longer working like expected. > > Casting to "int" or truncating is the right thing to do in this case. Signed integer overflow is undefined. Using 'int' is liable to cause problems after 2^32 ticks. Colin Percival >>> Log: >>>Ticks are 32-bit in FreeBSD. >>> >>>MFC after:3 days >>>Sponsored by: Mellanox Technologies >>> >>> Modified: >>>head/sys/ofed/drivers/infiniband/core/addr.c >>> >>> Modified: head/sys/ofed/drivers/infiniband/core/addr.c >>> == >>> --- head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:14:43 >>> 2017(r321984) >>> +++ head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:18:25 >>> 2017(r321985) >>> @@ -187,10 +187,10 @@ EXPORT_SYMBOL(rdma_translate_ip); >>> >>> static void set_timeout(unsigned long time) >>> { >>> - unsigned long delay; >>> + int delay; /* under FreeBSD ticks are 32-bit */ >>> >>> delay = time - jiffies; >>> - if ((long)delay <= 0) >>> + if (delay <= 0) >>> delay = 1; >>> >>> mod_delayed_work(addr_wq, &work, delay); -- Colin Percival Security Officer Emeritus, FreeBSD | The power to serve Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core
On 08/03/17 16:37, Conrad Meyer wrote: Hey Hans, Is it not important that the subtraction and result are evaluated without truncation? Hi, ticks is a circular counter. Assuming time = 0 and jiffies = -1U, then "delay" becomes a very large negative value, because long is used, and the delay <= 0 check, is no longer working like expected. Casting to "int" or truncating is the right thing to do in this case. --HPS Thanks, Conrad On Thu, Aug 3, 2017 at 2:18 AM, Hans Petter Selasky wrote: Author: hselasky Date: Thu Aug 3 09:18:25 2017 New Revision: 321985 URL: https://svnweb.freebsd.org/changeset/base/321985 Log: Ticks are 32-bit in FreeBSD. MFC after:3 days Sponsored by: Mellanox Technologies Modified: head/sys/ofed/drivers/infiniband/core/addr.c Modified: head/sys/ofed/drivers/infiniband/core/addr.c == --- head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:14:43 2017(r321984) +++ head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:18:25 2017(r321985) @@ -187,10 +187,10 @@ EXPORT_SYMBOL(rdma_translate_ip); static void set_timeout(unsigned long time) { - unsigned long delay; + int delay; /* under FreeBSD ticks are 32-bit */ delay = time - jiffies; - if ((long)delay <= 0) + if (delay <= 0) delay = 1; mod_delayed_work(addr_wq, &work, delay); ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core
Hey Hans, Is it not important that the subtraction and result are evaluated without truncation? Thanks, Conrad On Thu, Aug 3, 2017 at 2:18 AM, Hans Petter Selasky wrote: > Author: hselasky > Date: Thu Aug 3 09:18:25 2017 > New Revision: 321985 > URL: https://svnweb.freebsd.org/changeset/base/321985 > > Log: > Ticks are 32-bit in FreeBSD. > > MFC after:3 days > Sponsored by: Mellanox Technologies > > Modified: > head/sys/ofed/drivers/infiniband/core/addr.c > > Modified: head/sys/ofed/drivers/infiniband/core/addr.c > == > --- head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:14:43 > 2017(r321984) > +++ head/sys/ofed/drivers/infiniband/core/addr.cThu Aug 3 09:18:25 > 2017(r321985) > @@ -187,10 +187,10 @@ EXPORT_SYMBOL(rdma_translate_ip); > > static void set_timeout(unsigned long time) > { > - unsigned long delay; > + int delay; /* under FreeBSD ticks are 32-bit */ > > delay = time - jiffies; > - if ((long)delay <= 0) > + if (delay <= 0) > delay = 1; > > mod_delayed_work(addr_wq, &work, delay); > ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"