Re: Another gcc corruption bug (was Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings)

2016-10-13 Thread Josh Poimboeuf 
On Thu, Oct 13, 2016 at 07:57:41PM +0200, Denys Vlasenko wrote:
> On 10/13/2016 02:46 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
> > >  :
> > >0: 55  push   %rbp
> > >1: 48 89 e5mov%rsp,%rbp
> > >4: 53  push   %rbx
> > >5: 48 89 fbmov%rdi,%rbx
> > >8: 48 83 ec 08 sub$0x8,%rsp
> > >c: e8 00 00 00 00  callq  11 
> > >   d: R_X86_64_PC32__sanitizer_cov_trace_pc-0x4
> > >   11: 8b 03   mov(%rbx),%eax
> > >   13: 85 c0   test   %eax,%eax
> > >   15: 75 11   jne28 
> > >   17: 48 83 c4 08 add$0x8,%rsp
> > >   1b: 5b  pop%rbx
> > >   1c: 5d  pop%rbp
> > >   1d: e9 00 00 00 00  jmpq   22 
> > >   1e: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
> > >   22: 66 0f 1f 44 00 00   nopw   0x0(%rax,%rax,1)
> > >   28: e8 00 00 00 00  callq  2d 
> > >   29: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
> > >   2d: 48 8b 7b 10 mov0x10(%rbx),%rdi
> > >   31: e8 00 00 00 00  callq  36 
> > >   32: R_X86_64_PC32   ioread32-0x4
> > >   36: 89 05 00 00 00 00   mov%eax,0x0(%rip)# 3c 
> > > 
> > >   38: R_X86_64_PC32   snic_log_q_error_err_status-0x4
> > >   3c: 83 3b 01cmpl   $0x1,(%rbx)
> > >   3f: 76 d6   jbe17 
> > >   41: e8 00 00 00 00  callq  46 
> > >   42: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
> > 
> > I opened a bug:
> > 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966
> > 
> 
> Surprisingly, it's really "not a bug". The only way you can end up in this 
> branch
> is if you have a bug and run off the end of wq[1] array member: i.e.
> if snic->wq_count >= 2. (See gcc BZ for smaller example)
> 
> It's debatable whether it's okay for gcc to just let buggy code to run off
> and execute something random. It is surely surprising, and not debug-friendly.
> 
> An option to emit a crashing instruction (HLT, INT3, that sort of thing)
> instead of just stopping code generation might be useful.

Ah, you're right.

IMO it's still a gcc bug though.  Instead of following a bad pointer, it
would instead start executing some random function.  That takes
"undefined behavior" to a new level.

-- 
Josh

Re: Another gcc corruption bug (was Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings)

2016-10-13 Thread Josh Poimboeuf 
On Thu, Oct 13, 2016 at 07:57:41PM +0200, Denys Vlasenko wrote:
> On 10/13/2016 02:46 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
> > >  :
> > >0: 55  push   %rbp
> > >1: 48 89 e5mov%rsp,%rbp
> > >4: 53  push   %rbx
> > >5: 48 89 fbmov%rdi,%rbx
> > >8: 48 83 ec 08 sub$0x8,%rsp
> > >c: e8 00 00 00 00  callq  11 
> > >   d: R_X86_64_PC32__sanitizer_cov_trace_pc-0x4
> > >   11: 8b 03   mov(%rbx),%eax
> > >   13: 85 c0   test   %eax,%eax
> > >   15: 75 11   jne28 
> > >   17: 48 83 c4 08 add$0x8,%rsp
> > >   1b: 5b  pop%rbx
> > >   1c: 5d  pop%rbp
> > >   1d: e9 00 00 00 00  jmpq   22 
> > >   1e: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
> > >   22: 66 0f 1f 44 00 00   nopw   0x0(%rax,%rax,1)
> > >   28: e8 00 00 00 00  callq  2d 
> > >   29: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
> > >   2d: 48 8b 7b 10 mov0x10(%rbx),%rdi
> > >   31: e8 00 00 00 00  callq  36 
> > >   32: R_X86_64_PC32   ioread32-0x4
> > >   36: 89 05 00 00 00 00   mov%eax,0x0(%rip)# 3c 
> > > 
> > >   38: R_X86_64_PC32   snic_log_q_error_err_status-0x4
> > >   3c: 83 3b 01cmpl   $0x1,(%rbx)
> > >   3f: 76 d6   jbe17 
> > >   41: e8 00 00 00 00  callq  46 
> > >   42: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
> > 
> > I opened a bug:
> > 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966
> > 
> 
> Surprisingly, it's really "not a bug". The only way you can end up in this 
> branch
> is if you have a bug and run off the end of wq[1] array member: i.e.
> if snic->wq_count >= 2. (See gcc BZ for smaller example)
> 
> It's debatable whether it's okay for gcc to just let buggy code to run off
> and execute something random. It is surely surprising, and not debug-friendly.
> 
> An option to emit a crashing instruction (HLT, INT3, that sort of thing)
> instead of just stopping code generation might be useful.

Ah, you're right.

IMO it's still a gcc bug though.  Instead of following a bad pointer, it
would instead start executing some random function.  That takes
"undefined behavior" to a new level.

-- 
Josh

Re: Another gcc corruption bug (was Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings)

2016-10-13 Thread Denys Vlasenko 

On 10/13/2016 02:46 PM, Josh Poimboeuf wrote:

On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:

On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:

Notice how it just falls off the end of the function.  We had a similar
bug before:

  https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble


I remember that nightmare :(


  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646

I'm not sure yet if this is the same gcc bug or a different one.  Maybe
it's related to the new GCC_PLUGIN_SANCOV?


I've reduced one of the test cases to this now:

/* gcc-6  -O2 -fno-strict-aliasing -fno-reorder-blocks -fno-omit-frame-pointer  
-Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall -Werror -c snic_res.c -o 
snic_res.o */
typedef int spinlock_t;
extern unsigned int ioread32(void *);
struct vnic_wq_ctrl {
unsigned int error_status;
};
struct vnic_wq {
struct vnic_wq_ctrl *ctrl;
} mempool_t;
struct snic {
unsigned int wq_count;
__attribute__ ((__aligned__)) struct vnic_wq wq[1];
spinlock_t wq_lock[1];
};
unsigned int snic_log_q_error_err_status;
void snic_log_q_error(struct snic *snic)
{
unsigned int i;
for (i = 0; i < snic->wq_count; i++)
snic_log_q_error_err_status =
ioread32(>wq[i].ctrl->error_status);
}

which gets compiled into

 :
   0:   55  push   %rbp
   1:   48 89 e5mov%rsp,%rbp
   4:   53  push   %rbx
   5:   48 89 fbmov%rdi,%rbx
   8:   48 83 ec 08 sub$0x8,%rsp
   c:   e8 00 00 00 00  callq  11 
d: R_X86_64_PC32__sanitizer_cov_trace_pc-0x4
  11:   8b 03   mov(%rbx),%eax
  13:   85 c0   test   %eax,%eax
  15:   75 11   jne28 
  17:   48 83 c4 08 add$0x8,%rsp
  1b:   5b  pop%rbx
  1c:   5d  pop%rbp
  1d:   e9 00 00 00 00  jmpq   22 
1e: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
  22:   66 0f 1f 44 00 00   nopw   0x0(%rax,%rax,1)
  28:   e8 00 00 00 00  callq  2d 
29: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
  2d:   48 8b 7b 10 mov0x10(%rbx),%rdi
  31:   e8 00 00 00 00  callq  36 
32: R_X86_64_PC32   ioread32-0x4
  36:   89 05 00 00 00 00   mov%eax,0x0(%rip)# 3c 

38: R_X86_64_PC32   snic_log_q_error_err_status-0x4
  3c:   83 3b 01cmpl   $0x1,(%rbx)
  3f:   76 d6   jbe17 
  41:   e8 00 00 00 00  callq  46 
42: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4


I opened a bug:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966



Surprisingly, it's really "not a bug". The only way you can end up in this 
branch
is if you have a bug and run off the end of wq[1] array member: i.e.
if snic->wq_count >= 2. (See gcc BZ for smaller example)

It's debatable whether it's okay for gcc to just let buggy code to run off
and execute something random. It is surely surprising, and not debug-friendly.

An option to emit a crashing instruction (HLT, INT3, that sort of thing)
instead of just stopping code generation might be useful.

Re: Another gcc corruption bug (was Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings)

2016-10-13 Thread Denys Vlasenko 

On 10/13/2016 02:46 PM, Josh Poimboeuf wrote:

On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:

On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:

Notice how it just falls off the end of the function.  We had a similar
bug before:

  https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble


I remember that nightmare :(


  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646

I'm not sure yet if this is the same gcc bug or a different one.  Maybe
it's related to the new GCC_PLUGIN_SANCOV?


I've reduced one of the test cases to this now:

/* gcc-6  -O2 -fno-strict-aliasing -fno-reorder-blocks -fno-omit-frame-pointer  
-Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall -Werror -c snic_res.c -o 
snic_res.o */
typedef int spinlock_t;
extern unsigned int ioread32(void *);
struct vnic_wq_ctrl {
unsigned int error_status;
};
struct vnic_wq {
struct vnic_wq_ctrl *ctrl;
} mempool_t;
struct snic {
unsigned int wq_count;
__attribute__ ((__aligned__)) struct vnic_wq wq[1];
spinlock_t wq_lock[1];
};
unsigned int snic_log_q_error_err_status;
void snic_log_q_error(struct snic *snic)
{
unsigned int i;
for (i = 0; i < snic->wq_count; i++)
snic_log_q_error_err_status =
ioread32(>wq[i].ctrl->error_status);
}

which gets compiled into

 :
   0:   55  push   %rbp
   1:   48 89 e5mov%rsp,%rbp
   4:   53  push   %rbx
   5:   48 89 fbmov%rdi,%rbx
   8:   48 83 ec 08 sub$0x8,%rsp
   c:   e8 00 00 00 00  callq  11 
d: R_X86_64_PC32__sanitizer_cov_trace_pc-0x4
  11:   8b 03   mov(%rbx),%eax
  13:   85 c0   test   %eax,%eax
  15:   75 11   jne28 
  17:   48 83 c4 08 add$0x8,%rsp
  1b:   5b  pop%rbx
  1c:   5d  pop%rbp
  1d:   e9 00 00 00 00  jmpq   22 
1e: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
  22:   66 0f 1f 44 00 00   nopw   0x0(%rax,%rax,1)
  28:   e8 00 00 00 00  callq  2d 
29: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
  2d:   48 8b 7b 10 mov0x10(%rbx),%rdi
  31:   e8 00 00 00 00  callq  36 
32: R_X86_64_PC32   ioread32-0x4
  36:   89 05 00 00 00 00   mov%eax,0x0(%rip)# 3c 

38: R_X86_64_PC32   snic_log_q_error_err_status-0x4
  3c:   83 3b 01cmpl   $0x1,(%rbx)
  3f:   76 d6   jbe17 
  41:   e8 00 00 00 00  callq  46 
42: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4


I opened a bug:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966



Surprisingly, it's really "not a bug". The only way you can end up in this 
branch
is if you have a bug and run off the end of wq[1] array member: i.e.
if snic->wq_count >= 2. (See gcc BZ for smaller example)

It's debatable whether it's okay for gcc to just let buggy code to run off
and execute something random. It is surely surprising, and not debug-friendly.

An option to emit a crashing instruction (HLT, INT3, that sort of thing)
instead of just stopping code generation might be useful.

Another gcc corruption bug (was Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings)

2016-10-13 Thread Josh Poimboeuf 
On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
> On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
> > Notice how it just falls off the end of the function.  We had a similar
> > bug before:
> > 
> >   https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble
> 
> I remember that nightmare :(
> 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> > 
> > I'm not sure yet if this is the same gcc bug or a different one.  Maybe
> > it's related to the new GCC_PLUGIN_SANCOV?
> 
> I've reduced one of the test cases to this now:
> 
> /* gcc-6  -O2 -fno-strict-aliasing -fno-reorder-blocks 
> -fno-omit-frame-pointer  -Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall 
> -Werror -c snic_res.c -o snic_res.o */
> typedef int spinlock_t;
> extern unsigned int ioread32(void *);
> struct vnic_wq_ctrl {
>   unsigned int error_status;
> };
> struct vnic_wq {
>   struct vnic_wq_ctrl *ctrl;
> } mempool_t;
> struct snic {
>   unsigned int wq_count;
>   __attribute__ ((__aligned__)) struct vnic_wq wq[1];
>   spinlock_t wq_lock[1];
> };
> unsigned int snic_log_q_error_err_status;
> void snic_log_q_error(struct snic *snic)
> {
>   unsigned int i;
>   for (i = 0; i < snic->wq_count; i++)
>   snic_log_q_error_err_status =
>   ioread32(>wq[i].ctrl->error_status);
> }
> 
> which gets compiled into
> 
>  :
>0: 55  push   %rbp
>1: 48 89 e5mov%rsp,%rbp
>4: 53  push   %rbx
>5: 48 89 fbmov%rdi,%rbx
>8: 48 83 ec 08 sub$0x8,%rsp
>c: e8 00 00 00 00  callq  11 
>   d: R_X86_64_PC32__sanitizer_cov_trace_pc-0x4
>   11: 8b 03   mov(%rbx),%eax
>   13: 85 c0   test   %eax,%eax
>   15: 75 11   jne28 
>   17: 48 83 c4 08 add$0x8,%rsp
>   1b: 5b  pop%rbx
>   1c: 5d  pop%rbp
>   1d: e9 00 00 00 00  jmpq   22 
>   1e: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
>   22: 66 0f 1f 44 00 00   nopw   0x0(%rax,%rax,1)
>   28: e8 00 00 00 00  callq  2d 
>   29: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
>   2d: 48 8b 7b 10 mov0x10(%rbx),%rdi
>   31: e8 00 00 00 00  callq  36 
>   32: R_X86_64_PC32   ioread32-0x4
>   36: 89 05 00 00 00 00   mov%eax,0x0(%rip)# 3c 
> 
>   38: R_X86_64_PC32   snic_log_q_error_err_status-0x4
>   3c: 83 3b 01cmpl   $0x1,(%rbx)
>   3f: 76 d6   jbe17 
>   41: e8 00 00 00 00  callq  46 
>   42: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4

I opened a bug:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966

-- 
Josh

Another gcc corruption bug (was Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings)

2016-10-13 Thread Josh Poimboeuf 
On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
> On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
> > Notice how it just falls off the end of the function.  We had a similar
> > bug before:
> > 
> >   https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble
> 
> I remember that nightmare :(
> 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> > 
> > I'm not sure yet if this is the same gcc bug or a different one.  Maybe
> > it's related to the new GCC_PLUGIN_SANCOV?
> 
> I've reduced one of the test cases to this now:
> 
> /* gcc-6  -O2 -fno-strict-aliasing -fno-reorder-blocks 
> -fno-omit-frame-pointer  -Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall 
> -Werror -c snic_res.c -o snic_res.o */
> typedef int spinlock_t;
> extern unsigned int ioread32(void *);
> struct vnic_wq_ctrl {
>   unsigned int error_status;
> };
> struct vnic_wq {
>   struct vnic_wq_ctrl *ctrl;
> } mempool_t;
> struct snic {
>   unsigned int wq_count;
>   __attribute__ ((__aligned__)) struct vnic_wq wq[1];
>   spinlock_t wq_lock[1];
> };
> unsigned int snic_log_q_error_err_status;
> void snic_log_q_error(struct snic *snic)
> {
>   unsigned int i;
>   for (i = 0; i < snic->wq_count; i++)
>   snic_log_q_error_err_status =
>   ioread32(>wq[i].ctrl->error_status);
> }
> 
> which gets compiled into
> 
>  :
>0: 55  push   %rbp
>1: 48 89 e5mov%rsp,%rbp
>4: 53  push   %rbx
>5: 48 89 fbmov%rdi,%rbx
>8: 48 83 ec 08 sub$0x8,%rsp
>c: e8 00 00 00 00  callq  11 
>   d: R_X86_64_PC32__sanitizer_cov_trace_pc-0x4
>   11: 8b 03   mov(%rbx),%eax
>   13: 85 c0   test   %eax,%eax
>   15: 75 11   jne28 
>   17: 48 83 c4 08 add$0x8,%rsp
>   1b: 5b  pop%rbx
>   1c: 5d  pop%rbp
>   1d: e9 00 00 00 00  jmpq   22 
>   1e: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
>   22: 66 0f 1f 44 00 00   nopw   0x0(%rax,%rax,1)
>   28: e8 00 00 00 00  callq  2d 
>   29: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4
>   2d: 48 8b 7b 10 mov0x10(%rbx),%rdi
>   31: e8 00 00 00 00  callq  36 
>   32: R_X86_64_PC32   ioread32-0x4
>   36: 89 05 00 00 00 00   mov%eax,0x0(%rip)# 3c 
> 
>   38: R_X86_64_PC32   snic_log_q_error_err_status-0x4
>   3c: 83 3b 01cmpl   $0x1,(%rbx)
>   3f: 76 d6   jbe17 
>   41: e8 00 00 00 00  callq  46 
>   42: R_X86_64_PC32   __sanitizer_cov_trace_pc-0x4

I opened a bug:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966

-- 
Josh