Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core

[Bruce Evans]

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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core

[Conrad Meyer]

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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core

[Hans Petter Selasky]

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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core

[Colin Percival]

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, , delay);

-- 
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core

[Hans Petter Selasky]

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, , delay);






___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r321985 - head/sys/ofed/drivers/infiniband/core

[Conrad Meyer]

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, , delay);
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"