News UBSAN warnings in aacraid

Commit Message

Arnd Bergmann Nov. 27, 2017, 9:32 p.m. UTC

On Mon, Nov 27, 2017 at 8:17 PM, Meelis Roos <[e-mail]> wrote:
> Tried 4.15-rc1 on an old 32-bit HP Netserver with aacraid card. Compared
> to 4.14, there are new UBSAN warnings with timer related backtraces, so
> the timespec64 change seems suspicious:

> [   12.228155] UBSAN: Undefined behaviour in drivers/scsi/aacraid/commsup.c:2514:49
> [   12.228229] signed integer overflow:
> [   12.228283] 964297611 * 250 cannot be represented in type 'long int'

Thanks for reporting it! For reference, this is my change that got applied to
aac_command_thread:

@@ -2496,7 +2496,7 @@ int aac_command_thread(void *data)
                }
                if (!time_before(next_check_jiffies,next_jiffies)
                 && ((difference = next_jiffies - jiffies) <= 0)) {
-                       struct timeval now;
+                       struct timespec64 now;
                        int ret;

                        /* Don't even try to talk to adapter if its sick */
@@ -2506,15 +2506,15 @@ int aac_command_thread(void *data)
                        next_check_jiffies = jiffies
                                           + ((long)(unsigned)check_interval)
                                           * HZ;
-                       do_gettimeofday(&now);
+                       ktime_get_real_ts64(&now);

                        /* Synchronize our watches */
-                       if (((1000000 - (1000000 / HZ)) > now.tv_usec)
-                        && (now.tv_usec > (1000000 / HZ)))
-                               difference = (((1000000 - now.tv_usec) * HZ)
-                                 + 500000) / 1000000;
+                       if (((NSEC_PER_SEC - (NSEC_PER_SEC / HZ)) > now.tv_nsec)
+                        && (now.tv_nsec > (NSEC_PER_SEC / HZ)))
+                               difference = (((NSEC_PER_SEC -
now.tv_nsec) * HZ)
+                                 + NSEC_PER_SEC / 2) / NSEC_PER_SEC;
                        else {
-                               if (now.tv_usec > 500000)
+                               if (now.tv_nsec > NSEC_PER_SEC / 2)
                                        ++now.tv_sec;

                                if (dev->sa_firmware)

The problem is that a microsecond number (0 to 999999) multiplied by
HZ (100 to 1024) always fits in a 32-bit integer, but the nanosecond
number doesn't.

We could make that a 64-bit division, but that would be fairly expensive.

I'm trying to understand the bigger picture now, rather than simply
attempting to do a simple conversion, but I don't see what we are
actually trying to compute in 'difference' here.

I think this chunk would solve the problem and result in the
same behavior as before:

                                        ++now.tv_sec;

but I don't see why we add in half a second here. Any ideas?

       Arnd

Comments

Meelis Roos Nov. 28, 2017, 1:05 p.m. UTC | #1

> I think this chunk would solve the problem and result in the
> same behavior as before:
> 
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -2511,8 +2511,8 @@ int aac_command_thread(void *data)
>                         /* Synchronize our watches */
>                         if (((NSEC_PER_SEC - (NSEC_PER_SEC / HZ)) > now.tv_nsec)
>                          && (now.tv_nsec > (NSEC_PER_SEC / HZ)))
> -                               difference = (((NSEC_PER_SEC -
> now.tv_nsec) * HZ)
> -                                 + NSEC_PER_SEC / 2) / NSEC_PER_SEC;
> +                               difference = HZ + HZ / 2 -
> +                                            now.tv_nsec / (NSEC_PER_SEC / HZ);
>                         else {
>                                 if (now.tv_nsec > NSEC_PER_SEC / 2)
>                                         ++now.tv_sec;
> 
> but I don't see why we add in half a second here. Any ideas?

I did not try to understand the details but I can confirm that this 
patch makes the warnings go away.

Arnd Bergmann Nov. 28, 2017, 1:27 p.m. UTC | #2

On Tue, Nov 28, 2017 at 2:05 PM, Meelis Roos <[e-mail]> wrote:
>> I think this chunk would solve the problem and result in the
>> same behavior as before:
>>
>> --- a/drivers/scsi/aacraid/commsup.c
>> +++ b/drivers/scsi/aacraid/commsup.c
>> @@ -2511,8 +2511,8 @@ int aac_command_thread(void *data)
>>                         /* Synchronize our watches */
>>                         if (((NSEC_PER_SEC - (NSEC_PER_SEC / HZ)) > now.tv_nsec)
>>                          && (now.tv_nsec > (NSEC_PER_SEC / HZ)))
>> -                               difference = (((NSEC_PER_SEC -
>> now.tv_nsec) * HZ)
>> -                                 + NSEC_PER_SEC / 2) / NSEC_PER_SEC;
>> +                               difference = HZ + HZ / 2 -
>> +                                            now.tv_nsec / (NSEC_PER_SEC / HZ);
>>                         else {
>>                                 if (now.tv_nsec > NSEC_PER_SEC / 2)
>>                                         ++now.tv_sec;
>>
>> but I don't see why we add in half a second here. Any ideas?
>
> I did not try to understand the details but I can confirm that this
> patch makes the warnings go away.

Thanks for testing! I've wrote it up as a proper patch now, and tried
to capture what I understand about this code and how I got to
the new change.

       Arnd

Patch

--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -2511,8 +2511,8 @@  int aac_command_thread(void *data)
                        /* Synchronize our watches */
                        if (((NSEC_PER_SEC - (NSEC_PER_SEC / HZ)) > now.tv_nsec)
                         && (now.tv_nsec > (NSEC_PER_SEC / HZ)))
-                               difference = (((NSEC_PER_SEC -
now.tv_nsec) * HZ)
-                                 + NSEC_PER_SEC / 2) / NSEC_PER_SEC;
+                               difference = HZ + HZ / 2 -
+                                            now.tv_nsec / (NSEC_PER_SEC / HZ);
                        else {
                                if (now.tv_nsec > NSEC_PER_SEC / 2)