* News UBSAN warnings in aacraid
@ 2017-11-27 19:17 Meelis Roos
2017-11-27 21:32 ` Arnd Bergmann
0 siblings, 1 reply; 4+ messages in thread
From: Meelis Roos @ 2017-11-27 19:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Dave Carroll, Martin K. Petersen, linux-scsi, Linux Kernel list
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.228058] ================================================================================
[ 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'
[ 12.228347] CPU: 1 PID: 276 Comm: aacraid Not tainted 4.15.0-rc1 #80
[ 12.228404] Hardware name: Hewlett Packard HP NetServer/HP System Board, BIOS 4.06.46 PW 06/25/2003
[ 12.228477] Call Trace:
[ 12.228560] dump_stack+0x48/0x65
[ 12.228620] ubsan_epilogue+0xe/0x40
[ 12.228677] handle_overflow+0xad/0xc0
[ 12.228754] ? del_timer_sync+0x39/0x50
[ 12.228818] ? __getnstimeofday64+0x4d/0x200
[ 12.228877] __ubsan_handle_mul_overflow+0x12/0x20
[ 12.229037] aac_command_thread+0x1243/0x1290 [aacraid]
[ 12.229109] ? pick_next_task_fair+0x27f/0x760
[ 12.229175] ? __schedule+0x1b1/0x8e0
[ 12.229245] ? wake_up_q+0xa0/0xa0
[ 12.229311] kthread+0x13d/0x1f0
[ 12.229390] ? aac_send_hosttime+0xf0/0xf0 [aacraid]
[ 12.229449] ? __kthread_create_worker+0x110/0x110
[ 12.229516] ret_from_fork+0x19/0x24
[ 12.229571] ================================================================================
[ 12.292055] ================================================================================
[ 12.292130] UBSAN: Undefined behaviour in drivers/scsi/aacraid/commsup.c:2515:7
[ 12.292200] signed integer overflow:
[ 12.292252] 1734571608 + 500000000 cannot be represented in type 'long int'
[ 12.292312] CPU: 1 PID: 276 Comm: aacraid Not tainted 4.15.0-rc1 #80
[ 12.292368] Hardware name: Hewlett Packard HP NetServer/HP System Board, BIOS 4.06.46 PW 06/25/2003
[ 12.292439] Call Trace:
[ 12.292494] dump_stack+0x48/0x65
[ 12.292550] ubsan_epilogue+0xe/0x40
[ 12.292606] handle_overflow+0xad/0xc0
[ 12.292665] ? del_timer_sync+0x39/0x50
[ 12.292722] ? __getnstimeofday64+0x4d/0x200
[ 12.292780] __ubsan_handle_add_overflow+0x12/0x20
[ 12.292861] aac_command_thread+0x1259/0x1290 [aacraid]
[ 12.292922] ? pick_next_task_fair+0x27f/0x760
[ 12.292980] ? __schedule+0x1b1/0x8e0
[ 12.293037] ? wake_up_q+0xa0/0xa0
[ 12.293094] kthread+0x13d/0x1f0
[ 12.293172] ? aac_send_hosttime+0xf0/0xf0 [aacraid]
[ 12.293231] ? __kthread_create_worker+0x110/0x110
[ 12.293289] ret_from_fork+0x19/0x24
[ 12.293345] ================================================================================
--
Meelis Roos (mroos@linux.ee)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: News UBSAN warnings in aacraid
2017-11-27 19:17 News UBSAN warnings in aacraid Meelis Roos
@ 2017-11-27 21:32 ` Arnd Bergmann
2017-11-28 13:05 ` Meelis Roos
0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2017-11-27 21:32 UTC (permalink / raw)
To: Meelis Roos
Cc: Dave Carroll, Martin K. Petersen, linux-scsi, Linux Kernel list
On Mon, Nov 27, 2017 at 8:17 PM, Meelis Roos <mroos@linux.ee> 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:
--- 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?
Arnd
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: News UBSAN warnings in aacraid
2017-11-27 21:32 ` Arnd Bergmann
@ 2017-11-28 13:05 ` Meelis Roos
2017-11-28 13:27 ` Arnd Bergmann
0 siblings, 1 reply; 4+ messages in thread
From: Meelis Roos @ 2017-11-28 13:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Dave Carroll, Martin K. Petersen, linux-scsi, Linux Kernel list
> 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.
--
Meelis Roos (mroos@linux.ee)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: News UBSAN warnings in aacraid
2017-11-28 13:05 ` Meelis Roos
@ 2017-11-28 13:27 ` Arnd Bergmann
0 siblings, 0 replies; 4+ messages in thread
From: Arnd Bergmann @ 2017-11-28 13:27 UTC (permalink / raw)
To: Meelis Roos
Cc: Dave Carroll, Martin K. Petersen, linux-scsi, Linux Kernel list
On Tue, Nov 28, 2017 at 2:05 PM, Meelis Roos <mroos@linux.ee> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-11-28 13:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-27 19:17 News UBSAN warnings in aacraid Meelis Roos
2017-11-27 21:32 ` Arnd Bergmann
2017-11-28 13:05 ` Meelis Roos
2017-11-28 13:27 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®