Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380339 > unrolled thread

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: This patch triggers a bad gcc bug (was Re: [PATCH] force  inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-16 00:30 +0200
    Re: This patch triggers a bad gcc bug (was Re: [PATCH] force  inlining of some byteswap operations) Ingo Molnar <mingo@kernel.org> - 2016-04-16 11:10 +0200
      Re: This patch triggers a bad gcc bug (was Re: [PATCH] force  inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-18 15:40 +0200
        Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Arnd Bergmann <arnd@arndb.de> - 2016-04-18 16:10 +0200
          Re: This patch triggers a bad gcc bug (was Re: [PATCH] force  inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-18 16:20 +0200
            Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Arnd Bergmann <arnd@arndb.de> - 2016-04-18 16:30 +0200
        Re: This patch triggers a bad gcc bug (was Re: [PATCH] force  inlining of some byteswap operations) Ingo Molnar <mingo@kernel.org> - 2016-04-19 11:00 +0200
          [PATCH] scsi: fc: force inlining of wwn conversion functions Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-19 16:00 +0200
            Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Quinn Tran <quinn.tran@qlogic.com> - 2016-04-23 01:20 +0200
            Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-25 18:10 +0200
              Re: [PATCH] scsi: fc: force inlining of wwn conversion functions "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-04-26 04:50 +0200
                Re: [PATCH] scsi: fc: force inlining of wwn conversion functions James Bottomley <James.Bottomley@HansenPartnership.com> - 2016-04-26 05:40 +0200
                  Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Arnd Bergmann <arnd@arndb.de> - 2016-04-26 09:30 +0200
                    Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Christoph Hellwig <hch@infradead.org> - 2016-04-26 10:40 +0200
                      Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Arnd Bergmann <arnd@arndb.de> - 2016-04-26 12:10 +0200
                    Re: [PATCH] scsi: fc: force inlining of wwn conversion functions "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-04-26 15:10 +0200
                      Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Arnd Bergmann <arnd@arndb.de> - 2016-04-26 18:00 +0200
                        Re: [PATCH] scsi: fc: force inlining of wwn conversion functions James Bottomley <James.Bottomley@HansenPartnership.com> - 2016-04-27 00:40 +0200
                          Re: [PATCH] scsi: fc: force inlining of wwn conversion functions "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-04-27 02:50 +0200
                        Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Martin Jambor <mjambor@suse.cz> - 2016-04-27 13:10 +0200
                          Re: [PATCH] scsi: fc: force inlining of wwn conversion functions Arnd Bergmann <arnd@arndb.de> - 2016-04-27 23:40 +0200
                          [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann <arnd@arndb.de> - 2016-04-28 00:10 +0200
                            Re: [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-28 00:20 +0200

Page 1 of 2  [1] 2  Next page →

#1380339 — Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

On Fri, Apr 15, 2016 at 08:47:45AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 15, 2016 at 07:45:19AM +0200, Ingo Molnar wrote:
> > 
> > * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> > 
> > > > In fact, the following patch seems to fix it:
> > > > 
> > > > diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> > > > index bf66ea6..56b9e81 100644
> > > > --- a/include/scsi/scsi_transport_fc.h
> > > > +++ b/include/scsi/scsi_transport_fc.h
> > > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > > >  	return result;
> > > >  }
> > > >  
> > > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > > >  {
> > > >  	return get_unaligned_be64(wwn);
> > > >  }
> > > 
> > > It is not a guarantee.
> > 
> > Of course it's a workaround - but is there any deterministic way to turn off this 
> > GCC bug (by activating some GCC command line switch), or do we have to live with 
> > objtool warning about this GCC?
> 
> I don't think we know yet if there's a reliable way to turn the bug off.
> 
> Also, according to the gcc guys, this bug won't always result in a
> truncated function, and may sometimes just make some inline function
> call sites disappear:
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> 
> though I haven't been able to confirm that experimentally.  But if it's
> true, that means that objtool won't be able to detect all cases of the
> bug and some function calls may just silently disappear!
> 
> There's a lot of activity in the bug now, so hopefully they'll be able
> to tell us soon if there's a reliable way to avoid it and/or detect it.
> 
> BTW, Denys posted a workaround patch for the qla2xxxx code:
> 
>   https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com

Martin Jambor wrote a succinct summary of the conditions needed for this
bug:

  "This bug can occur when an inlineable function containing a call to
  __builtin_constant_p, which checks a parameter or a value it
  references and a (possibly indirect) caller of the function actually
  passes a constant, but stores it using a type of a different size."

So to prevent it from happening elsewhere in the kernel, it sounds like
we'd have to either remove all uses of __builtin_constant_p() or disable
inlining completely.

There's also no reliable way to detect the bug has occurred, though
objtool will detect it in cases when the function gets truncated.

-- 
Josh

[toc] | [next] | [standalone]

#1380514

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > I don't think we know yet if there's a reliable way to turn the bug off.
> > 
> > Also, according to the gcc guys, this bug won't always result in a
> > truncated function, and may sometimes just make some inline function
> > call sites disappear:
> > 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > 
> > though I haven't been able to confirm that experimentally.  But if it's
> > true, that means that objtool won't be able to detect all cases of the
> > bug and some function calls may just silently disappear!
> > 
> > There's a lot of activity in the bug now, so hopefully they'll be able
> > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > 
> > BTW, Denys posted a workaround patch for the qla2xxxx code:
> > 
> >   https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
> 
> Martin Jambor wrote a succinct summary of the conditions needed for this
> bug:
> 
>   "This bug can occur when an inlineable function containing a call to
>   __builtin_constant_p, which checks a parameter or a value it
>   references and a (possibly indirect) caller of the function actually
>   passes a constant, but stores it using a type of a different size."
> 
> So to prevent it from happening elsewhere in the kernel, it sounds like
> we'd have to either remove all uses of __builtin_constant_p() or disable
> inlining completely.
> 
> There's also no reliable way to detect the bug has occurred, though
> objtool will detect it in cases when the function gets truncated.

So it appears to me that due to the hard to detect nature of the GCC bug the fix 
will probably be backported by them, so I think we should be fine with relying on 
objtool to detect weird code sequences in the kernel, and should work around 
specific instances of the bug.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1381709

On Sat, Apr 16, 2016 at 11:03:32AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > > I don't think we know yet if there's a reliable way to turn the bug off.
> > > 
> > > Also, according to the gcc guys, this bug won't always result in a
> > > truncated function, and may sometimes just make some inline function
> > > call sites disappear:
> > > 
> > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > > 
> > > though I haven't been able to confirm that experimentally.  But if it's
> > > true, that means that objtool won't be able to detect all cases of the
> > > bug and some function calls may just silently disappear!
> > > 
> > > There's a lot of activity in the bug now, so hopefully they'll be able
> > > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > > 
> > > BTW, Denys posted a workaround patch for the qla2xxxx code:
> > > 
> > >   https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
> > 
> > Martin Jambor wrote a succinct summary of the conditions needed for this
> > bug:
> > 
> >   "This bug can occur when an inlineable function containing a call to
> >   __builtin_constant_p, which checks a parameter or a value it
> >   references and a (possibly indirect) caller of the function actually
> >   passes a constant, but stores it using a type of a different size."
> > 
> > So to prevent it from happening elsewhere in the kernel, it sounds like
> > we'd have to either remove all uses of __builtin_constant_p() or disable
> > inlining completely.
> > 
> > There's also no reliable way to detect the bug has occurred, though
> > objtool will detect it in cases when the function gets truncated.
> 
> So it appears to me that due to the hard to detect nature of the GCC bug the fix 
> will probably be backported by them, so I think we should be fine with relying on 
> objtool to detect weird code sequences in the kernel, and should work around 
> specific instances of the bug.

I agree.  So how should we work around the bug in this case?  There have
been several suggestions:

- change wwn_to_u64() to __always_inline

- change qla2x00_get_host_fabric_name() to skip the unnecessary call to
  wwn_to_u64()

- revert one of the two commits:
  bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
  ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")


-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1381734 — Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> 
> I agree.  So how should we work around the bug in this case?  There have
> been several suggestions:
> 
> - change wwn_to_u64() to __always_inline
> 
> - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
>   wwn_to_u64()
> 
> - revert one of the two commits:
>   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
>   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")

What about the patch to change get_unaligned_be64() that I posted?

I think we want to merge that anyway, I just don't know if that helps
with this particular problem as well.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1381738

On Mon, Apr 18, 2016 at 04:07:51PM +0200, Arnd Bergmann wrote:
> On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> > 
> > I agree.  So how should we work around the bug in this case?  There have
> > been several suggestions:
> > 
> > - change wwn_to_u64() to __always_inline
> > 
> > - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> >   wwn_to_u64()
> > 
> > - revert one of the two commits:
> >   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> >   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> 
> What about the patch to change get_unaligned_be64() that I posted?
> 
> I think we want to merge that anyway, I just don't know if that helps
> with this particular problem as well.

I replied to your other email about that -- it doesn't seem to help this
issue.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1381748 — Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

On Monday 18 April 2016 09:12:41 Josh Poimboeuf wrote:
> On Mon, Apr 18, 2016 at 04:07:51PM +0200, Arnd Bergmann wrote:
> > On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> > > 
> > > I agree.  So how should we work around the bug in this case?  There have
> > > been several suggestions:
> > > 
> > > - change wwn_to_u64() to __always_inline
> > > 
> > > - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> > >   wwn_to_u64()
> > > 
> > > - revert one of the two commits:
> > >   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
> > >   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> > 
> > What about the patch to change get_unaligned_be64() that I posted?
> > 
> > I think we want to merge that anyway, I just don't know if that helps
> > with this particular problem as well.
> 
> I replied to your other email about that -- it doesn't seem to help this
> issue.
> 

Ok, I see. I had problems with my mail server last week, your reply
must have been a victim of that as I never saw it (found it on the
web archive now).

I'd vote for the wwn_to_u64 change then as it should prevent the
same thing from happining in other drivers. I would prefer not to
see ef3fb2422ffe reverted, as that works around another gcc-6 bug
on ARM.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1382297

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Sat, Apr 16, 2016 at 11:03:32AM +0200, Ingo Molnar wrote:
> > 
> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > > I don't think we know yet if there's a reliable way to turn the bug off.
> > > > 
> > > > Also, according to the gcc guys, this bug won't always result in a
> > > > truncated function, and may sometimes just make some inline function
> > > > call sites disappear:
> > > > 
> > > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > > > 
> > > > though I haven't been able to confirm that experimentally.  But if it's
> > > > true, that means that objtool won't be able to detect all cases of the
> > > > bug and some function calls may just silently disappear!
> > > > 
> > > > There's a lot of activity in the bug now, so hopefully they'll be able
> > > > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > > > 
> > > > BTW, Denys posted a workaround patch for the qla2xxxx code:
> > > > 
> > > >   https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
> > > 
> > > Martin Jambor wrote a succinct summary of the conditions needed for this
> > > bug:
> > > 
> > >   "This bug can occur when an inlineable function containing a call to
> > >   __builtin_constant_p, which checks a parameter or a value it
> > >   references and a (possibly indirect) caller of the function actually
> > >   passes a constant, but stores it using a type of a different size."
> > > 
> > > So to prevent it from happening elsewhere in the kernel, it sounds like
> > > we'd have to either remove all uses of __builtin_constant_p() or disable
> > > inlining completely.
> > > 
> > > There's also no reliable way to detect the bug has occurred, though
> > > objtool will detect it in cases when the function gets truncated.
> > 
> > So it appears to me that due to the hard to detect nature of the GCC bug the fix 
> > will probably be backported by them, so I think we should be fine with relying on 
> > objtool to detect weird code sequences in the kernel, and should work around 
> > specific instances of the bug.
> 
> I agree.  So how should we work around the bug in this case?  There have
> been several suggestions:
> 
> - change wwn_to_u64() to __always_inline
> 
> - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
>   wwn_to_u64()
> 
> - revert one of the two commits:
>   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
>   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")

The first option sounds like the best one by far: it does a change that is related 
to the GCC bug (tweaks inlining), has near zero impact and does not revert other 
useful progress.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1382538 — [PATCH] scsi: fc: force inlining of wwn conversion functions

objtool reports [1] the following warning:

  drivers/scsi/qla2xxx/qla_attr.o: warning: objtool: qla2x00_get_host_fabric_name() falls through to next function qla2x00_get_starget_port_name()

This warning is due to a gcc bug [2] which causes corrupt code:

  0000000000002f53 <qla2x00_get_host_fabric_name>:
      2f53:       55                      push   %rbp
      2f54:       48 89 e5                mov    %rsp,%rbp

  0000000000002f57 <qla2x00_get_fc_host_stats>:
      2f57:       55                      push   %rbp
      2f58:       b9 e8 00 00 00          mov    $0xe8,%ecx
      2f5d:       48 89 e5                mov    %rsp,%rbp
  ...

Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
setting up the frame pointer.  It falls through to the next function,
which is very bad.

It occurs with the combination of the following two recent commits:

  bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
  ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")

The call chain which appears to trigger the problem is:

  qla2x00_get_host_fabric_name()
    wwn_to_u64()
      get_unaligned_be64()
        be64_to_cpup()
          __be64_to_cpup()

The bug requires very specific conditions to trigger.  According to Martin
Jambor (from the gcc bugzilla):

  "This bug can occur when an inlineable function containing a call to
  __builtin_constant_p, which checks a parameter or a value it
  references and a (possibly indirect) caller of the function actually
  passes a constant, but stores it using a type of a different size."

There's no reliable way to avoid (or even detect) the bug.  Until it
gets fixed in released versions of gcc, the least intrusive workaround
for this particular issue is to force the wwn conversion functions to be
inlined.

[1] https://lists.01.org/pipermail/kbuild-all/2016-April/019579.html
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 include/scsi/scsi_transport_fc.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index bf66ea6..1919cd4 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -796,12 +796,12 @@ fc_remote_port_chkready(struct fc_rport *rport)
 	return result;
 }
 
-static inline u64 wwn_to_u64(u8 *wwn)
+static __always_inline u64 wwn_to_u64(u8 *wwn)
 {
 	return get_unaligned_be64(wwn);
 }
 
-static inline void u64_to_wwn(u64 inm, u8 *wwn)
+static __always_inline void u64_to_wwn(u64 inm, u8 *wwn)
 {
 	put_unaligned_be64(inm, wwn);
 }
-- 
2.4.11

[toc] | [prev] | [next] | [standalone]

#1385531 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

Current kernel (4.6.0-rc4+) + GCC 5.3.0 definitely truncated qla2x00_get_host_fabric_name() routine.  Just like Josh indicated, we’re dropping down to the next routine.


root@mars:/sys/class/fc_host/host3  2016-04-22 16:07:30
> cat fabric_name
Killed
——
static void
qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
{
    32d0:       e8 00 00 00 00          callq  32d5 <qla2x00_get_host_fabric_name+0x5>
    32d5:       55                      push   %rbp
    32d6:       48 89 e5                mov    %rsp,%rbp
    32d9:       0f 1f 80 00 00 00 00    nopl   0x0(%rax)

00000000000032e0 <qla2x00_get_starget_node_name>:
qla2x00_get_starget_node_name():
/root/qt/linux.git/drivers/scsi/qla2xxx/qla_attr.c:1756
        fc_host_port_type(shost) = port_type;
}


----

Apr 22 16:07:50 mars kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
Apr 22 16:07:50 mars kernel: IP: [<ffffffff813f72d7>] scsi_is_host_device+0x7/0x20
Apr 22 16:07:50 mars kernel: PGD 7fe1c8067 PUD 7f5c72067 PMD 0 
Apr 22 16:07:50 mars kernel: Oops: 0000 [#1] SMP 
Apr 22 16:07:50 mars kernel: Modules linked in: qla2xxx scsi_transport_fc ebtable_nat ebtables ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 
...
dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E) [last unloaded: qla2xxx]
Apr 22 16:07:50 mars kernel: CPU: 8 PID: 10452 Comm: cat Tainted: G            E   4.6.0-rc4+ #2
Apr 22 16:07:50 mars kernel: Hardware name: HP ProLiant DL380 G7, BIOS P67 05/05/2011
Apr 22 16:07:50 mars kernel: task: ffff8807fcd1a880 ti: ffff8807ff128000 task.ti: ffff8807ff128000
Apr 22 16:07:50 mars kernel: RIP: 0010:[<ffffffff813f72d7>]  [<ffffffff813f72d7>] scsi_is_host_device+0x7/0x20
Apr 22 16:07:50 mars kernel: RSP: 0018:ffff8807ff12bcf0  EFLAGS: 00010246
Apr 22 16:07:50 mars kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff880ffe8ade88
Apr 22 16:07:50 mars kernel: RDX: ffff8807f5db9000 RSI: ffff880ffed43340 RDI: 0000000000000000
Apr 22 16:07:50 mars kernel: RBP: ffff8807ff12bd08 R08: ffff88101f45ac38 R09: ffff8807fccef280
Apr 22 16:07:50 mars kernel: R10: 0000000000000000 R11: 0000000000000000 R12: ffff88101b7e6000
Apr 22 16:07:50 mars kernel: R13: ffff8807fdaf1f00 R14: ffff8800dad379c0 R15: 0000000000000001
Apr 22 16:07:50 mars kernel: FS:  00007fd569512700(0000) GS:ffff88081fc80000(0000) knlGS:0000000000000000
Apr 22 16:07:50 mars kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Apr 22 16:07:50 mars kernel: CR2: 0000000000000058 CR3: 00000007f23f4000 CR4: 00000000000006e0
Apr 22 16:07:50 mars kernel: Stack:
Apr 22 16:07:50 mars kernel: ffffffffa0759db5 ffff88101b7e6000 ffff8807f5db9000 ffff8807ff12bd10
Apr 22 16:07:50 mars kernel: ffff8807ff12bd30 ffffffffa065b1bb ffff880ffed43340 ffffffff8166d950
Apr 22 16:07:50 mars kernel: ffff8807ff12bd50 ffffffff813c9e30 ffffffff815ccfb2 ffff8800dad379c0
Apr 22 16:07:50 mars kernel: Call Trace:
Apr 22 16:07:50 mars kernel: [<ffffffffa0759db5>] ? qla2x00_get_starget_node_name+0x25/0x90 [qla2xxx]
Apr 22 16:07:50 mars kernel: [<ffffffffa065b1bb>] ? show_fc_host_fabric_name+0x4b/0x80 [scsi_transport_fc]
Apr 22 16:07:50 mars kernel: [<ffffffff813c9e30>] ? dev_attr_show+0x20/0x50




Regards,
Quinn Tran






-----Original Message-----
From: <linux-scsi-owner@vger.kernel.org> on behalf of Josh Poimboeuf <jpoimboe@redhat.com>
Date: Tuesday, April 19, 2016 at 6:56 AM
To: James Bottomley <James.Bottomley@HansenPartnership.com>, "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi <linux-scsi@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Denys Vlasenko <dvlasenk@redhat.com>, Thomas Graf <tgraf@suug.ch>, Peter Zijlstra <peterz@infradead.org>, David Rientjes <rientjes@google.com>, Andrew Morton <akpm@linux-foundation.org>, Arnd Bergmann <arnd@arndb.de>, "jamborm@gcc.gnu.org" <jamborm@gcc.gnu.org>, Ingo Molnar <mingo@kernel.org>, Himanshu Madhani <himanshu.madhani@qlogic.com>, Dept-Eng QLA2xxx Upstream <qla2xxx-upstream@qlogic.com>
Subject: [PATCH] scsi: fc: force inlining of wwn conversion functions

>objtool reports [1] the following warning:
>
>  drivers/scsi/qla2xxx/qla_attr.o: warning: objtool: qla2x00_get_host_fabric_name() falls through to next function qla2x00_get_starget_port_name()
>
>This warning is due to a gcc bug [2] which causes corrupt code:
>
>  0000000000002f53 <qla2x00_get_host_fabric_name>:
>      2f53:       55                      push   %rbp
>      2f54:       48 89 e5                mov    %rsp,%rbp
>
>  0000000000002f57 <qla2x00_get_fc_host_stats>:
>      2f57:       55                      push   %rbp
>      2f58:       b9 e8 00 00 00          mov    $0xe8,%ecx
>      2f5d:       48 89 e5                mov    %rsp,%rbp
>  ...
>
>Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
>setting up the frame pointer.  It falls through to the next function,
>which is very bad.
>
>It occurs with the combination of the following two recent commits:
>
>  bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
>  ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
>
>The call chain which appears to trigger the problem is:
>
>  qla2x00_get_host_fabric_name()
>    wwn_to_u64()
>      get_unaligned_be64()
>        be64_to_cpup()
>          __be64_to_cpup()
>
>The bug requires very specific conditions to trigger.  According to Martin
>Jambor (from the gcc bugzilla):
>
>  "This bug can occur when an inlineable function containing a call to
>  __builtin_constant_p, which checks a parameter or a value it
>  references and a (possibly indirect) caller of the function actually
>  passes a constant, but stores it using a type of a different size."
>
>There's no reliable way to avoid (or even detect) the bug.  Until it
>gets fixed in released versions of gcc, the least intrusive workaround
>for this particular issue is to force the wwn conversion functions to be
>inlined.
>
>[1] https://lists.01.org/pipermail/kbuild-all/2016-April/019579.html
>[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>
>Reported-by: kbuild test robot <fengguang.wu@intel.com>
>Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>---
> include/scsi/scsi_transport_fc.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
>index bf66ea6..1919cd4 100644
>--- a/include/scsi/scsi_transport_fc.h
>+++ b/include/scsi/scsi_transport_fc.h
>@@ -796,12 +796,12 @@ fc_remote_port_chkready(struct fc_rport *rport)
> 	return result;
> }
> 
>-static inline u64 wwn_to_u64(u8 *wwn)
>+static __always_inline u64 wwn_to_u64(u8 *wwn)
> {
> 	return get_unaligned_be64(wwn);
> }
> 
>-static inline void u64_to_wwn(u64 inm, u8 *wwn)
>+static __always_inline void u64_to_wwn(u64 inm, u8 *wwn)
> {
> 	put_unaligned_be64(inm, wwn);
> }
>-- 
>2.4.11
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1386630 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

James, 

Can you merge this patch for 4.6?

On Tue, Apr 19, 2016 at 08:56:00AM -0500, Josh Poimboeuf wrote:
> objtool reports [1] the following warning:
> 
>   drivers/scsi/qla2xxx/qla_attr.o: warning: objtool: qla2x00_get_host_fabric_name() falls through to next function qla2x00_get_starget_port_name()
> 
> This warning is due to a gcc bug [2] which causes corrupt code:
> 
>   0000000000002f53 <qla2x00_get_host_fabric_name>:
>       2f53:       55                      push   %rbp
>       2f54:       48 89 e5                mov    %rsp,%rbp
> 
>   0000000000002f57 <qla2x00_get_fc_host_stats>:
>       2f57:       55                      push   %rbp
>       2f58:       b9 e8 00 00 00          mov    $0xe8,%ecx
>       2f5d:       48 89 e5                mov    %rsp,%rbp
>   ...
> 
> Note that qla2x00_get_host_fabric_name() is inexplicably truncated after
> setting up the frame pointer.  It falls through to the next function,
> which is very bad.
> 
> It occurs with the combination of the following two recent commits:
> 
>   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some byteswap operations")
>   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> 
> The call chain which appears to trigger the problem is:
> 
>   qla2x00_get_host_fabric_name()
>     wwn_to_u64()
>       get_unaligned_be64()
>         be64_to_cpup()
>           __be64_to_cpup()
> 
> The bug requires very specific conditions to trigger.  According to Martin
> Jambor (from the gcc bugzilla):
> 
>   "This bug can occur when an inlineable function containing a call to
>   __builtin_constant_p, which checks a parameter or a value it
>   references and a (possibly indirect) caller of the function actually
>   passes a constant, but stores it using a type of a different size."
> 
> There's no reliable way to avoid (or even detect) the bug.  Until it
> gets fixed in released versions of gcc, the least intrusive workaround
> for this particular issue is to force the wwn conversion functions to be
> inlined.
> 
> [1] https://lists.01.org/pipermail/kbuild-all/2016-April/019579.html
> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  include/scsi/scsi_transport_fc.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> index bf66ea6..1919cd4 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -796,12 +796,12 @@ fc_remote_port_chkready(struct fc_rport *rport)
>  	return result;
>  }
>  
> -static inline u64 wwn_to_u64(u8 *wwn)
> +static __always_inline u64 wwn_to_u64(u8 *wwn)
>  {
>  	return get_unaligned_be64(wwn);
>  }
>  
> -static inline void u64_to_wwn(u64 inm, u8 *wwn)
> +static __always_inline void u64_to_wwn(u64 inm, u8 *wwn)
>  {
>  	put_unaligned_be64(inm, wwn);
>  }
> -- 
> 2.4.11
> 

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1387102 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

>>>>> "Josh" == Josh Poimboeuf <jpoimboe@redhat.com> writes:

Josh> Can you merge this patch for 4.6?

I am really not a big fan of working around compiler bugs in a device
driver.

Are we sure there are no other get_unaligned_be64() calls in the kernel
that suffer the same fate?

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1387124 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

On Mon, 2016-04-25 at 22:40 -0400, Martin K. Petersen wrote:
> > > > > > "Josh" == Josh Poimboeuf <jpoimboe@redhat.com> writes:
> 
> Josh> Can you merge this patch for 4.6?
> 
> I am really not a big fan of working around compiler bugs in a device
> driver.

Me neither

> Are we sure there are no other get_unaligned_be64() calls in the
> kernel that suffer the same fate?

Agree, plus, as I've said before, we have 3-4 weeks before we go final,
so we still have some time before a decision has to be made.  It looks
like the gcc people already have a patch for the compiler, so the
distributions could just push that out through channels.

James

[toc] | [prev] | [next] | [standalone]

#1387183 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

On Monday 25 April 2016 20:37:31 James Bottomley wrote:
> On Mon, 2016-04-25 at 22:40 -0400, Martin K. Petersen wrote:
> > > > > > > "Josh" == Josh Poimboeuf <jpoimboe@redhat.com> writes:
> > 
> > Josh> Can you merge this patch for 4.6?
> > 
> > I am really not a big fan of working around compiler bugs in a device
> > driver.
> 
> Me neither
> 
> > Are we sure there are no other get_unaligned_be64() calls in the
> > kernel that suffer the same fate?
> 
> Agree, plus, as I've said before, we have 3-4 weeks before we go final,
> so we still have some time before a decision has to be made.  It looks
> like the gcc people already have a patch for the compiler, so the
> distributions could just push that out through channels.

I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to compilers
that have not been released yet in order to build a linux-4.6 kernel.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1387229 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

On Tue, Apr 26, 2016 at 09:22:46AM +0200, Arnd Bergmann wrote:
> > Agree, plus, as I've said before, we have 3-4 weeks before we go final,
> > so we still have some time before a decision has to be made.  It looks
> > like the gcc people already have a patch for the compiler, so the
> > distributions could just push that out through channels.
> 
> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to compilers
> that have not been released yet in order to build a linux-4.6 kernel.

Agreed.  What about just removing the wrappers?  They seem fairly
pointless to start with.

[toc] | [prev] | [next] | [standalone]

#1387323 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

On Tuesday 26 April 2016 01:35:16 Christoph Hellwig wrote:
> On Tue, Apr 26, 2016 at 09:22:46AM +0200, Arnd Bergmann wrote:
> > > Agree, plus, as I've said before, we have 3-4 weeks before we go final,
> > > so we still have some time before a decision has to be made.  It looks
> > > like the gcc people already have a patch for the compiler, so the
> > > distributions could just push that out through channels.
> > 
> > I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> > gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to compilers
> > that have not been released yet in order to build a linux-4.6 kernel.
> 
> Agreed.  What about just removing the wrappers?  They seem fairly
> pointless to start with.

I think at this point it's mainly a question of whether we want such a
big (however trivial) patch in v4.6. We can certainly do that for 4.7,
but as a fixup for the existing problem, either the __always_inline
hack or using a macro should be sufficient:

diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index bf66ea6bed2b..51a98b182a67 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -796,15 +796,8 @@ fc_remote_port_chkready(struct fc_rport *rport)
 	return result;
 }
 
-static inline u64 wwn_to_u64(u8 *wwn)
-{
-	return get_unaligned_be64(wwn);
-}
-
-static inline void u64_to_wwn(u64 inm, u8 *wwn)
-{
-	put_unaligned_be64(inm, wwn);
-}
+#define wwn_to_u64(wwn) get_unaligned_be64(wwn)
+#define u64_to_wwn(inm, wwn) put_unaligned_be64(inm, wwn)
 
 /**
  * fc_vport_set_state() - called to set a vport's state. Saves the old state,

	Arnd

[toc] | [prev] | [next] | [standalone]

#1387450 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

>>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:

Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to
Arnd> compilers that have not been released yet in order to build a
Arnd> linux-4.6 kernel.

I agree that compiler blacklisting is problematic and I'd like to avoid
it. The question is how far we go in the kernel to accommodate various
levels of brokenness.

In any case. Sticking compiler workarounds in device driver code is akin
to putting demolition orders on display on Alpha Centauri. At the very
minimum the patch should put a fat comment in the code stating that
these wrapper functions or #defines should not be changed in the future
because that'll break builds using gcc XYZ. But that does not solve the
problem for anybody else that might be doing something similar.
Converting between u64 and $RANDOM_TYPE in an inline wrapper does not
seem like a rare and unusual programming pattern.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1387646 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> >>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> 
> Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to
> Arnd> compilers that have not been released yet in order to build a
> Arnd> linux-4.6 kernel.
> 
> I agree that compiler blacklisting is problematic and I'd like to avoid
> it. The question is how far we go in the kernel to accommodate various
> levels of brokenness.
> 
> In any case. Sticking compiler workarounds in device driver code is akin
> to putting demolition orders on display on Alpha Centauri. At the very
> minimum the patch should put a fat comment in the code stating that
> these wrapper functions or #defines should not be changed in the future
> because that'll break builds using gcc XYZ. But that does not solve the
> problem for anybody else that might be doing something similar.
> Converting between u64 and $RANDOM_TYPE in an inline wrapper does not
> seem like a rare and unusual programming pattern.

It's not the driver really, it's the core scsi/fc layer, which makes
it a little dangerous that a random driver.

I agree that putting a comment in would also help. What I understand
from the bug report is that to trigger this bug you need these elements:

1. an inline function marked __always_inline
2. another inline function that is automatically inlined (not __always_inline)
3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2
4. __builtin_compatible_p inside that inline function

The last point is what Denys introduced in the kernel with
bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some
byteswap operations"). So maybe it's better after all to revert that
patch, to have a higher confidence in the same bug not appearing
elsewhere. It's also really a workaround for another quirk of the
compiler, but that one only results in duplicated functions in object
code rather than functions that end in the middle.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1388179 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

On Tue, 2016-04-26 at 17:58 +0200, Arnd Bergmann wrote:
> On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> > > > > > > "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> > 
> > Arnd> I don't think we can realistically blacklist gcc
> > -4.9.{0,1,2,3},
> > Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade
> > to
> > Arnd> compilers that have not been released yet in order to build a
> > Arnd> linux-4.6 kernel.
> > 
> > I agree that compiler blacklisting is problematic and I'd like to 
> > avoid it. The question is how far we go in the kernel to 
> > accommodate various levels of brokenness.
> > 
> > In any case. Sticking compiler workarounds in device driver code is 
> > akin to putting demolition orders on display on Alpha Centauri. At 
> > the very minimum the patch should put a fat comment in the code 
> > stating that these wrapper functions or #defines should not be 
> > changed in the future because that'll break builds using gcc XYZ. 
> > But that does not solve the problem for anybody else that might be 
> > doing something similar. Converting between u64 and $RANDOM_TYPE in 
> > an inline wrapper does not seem like a rare and unusual programming
> > pattern.
> 
> It's not the driver really, it's the core scsi/fc layer, which makes
> it a little dangerous that a random driver.

Well, it's libfc; that's a fibre channel transport class mostly used by
FCoE drivers ... there's few enough of those to call it driver only.

> I agree that putting a comment in would also help. What I understand
> from the bug report is that to trigger this bug you need these
> elements:
> 
> 1. an inline function marked __always_inline
> 2. another inline function that is automatically inlined (not
> __always_inline)
> 3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2
> 4. __builtin_compatible_p inside that inline function
> 
> The last point is what Denys introduced in the kernel with
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of 
> some byteswap operations"). So maybe it's better after all to revert 
> that patch, to have a higher confidence in the same bug not appearing
> elsewhere. It's also really a workaround for another quirk of the
> compiler, but that one only results in duplicated functions in object
> code rather than functions that end in the middle.

Yes, I think this is my preferred option.  That patch is nothing more
than an attempt to force the compiler to do something it didn't do but
should have.  If we apply the general rule that we shouldn't work
around compiler problems in the kernel code, then that should have been
disallowed first.  Plus, as the root cause of all of this, reverting
that patch will ensure that nothing else picks up this problem (at
least from the route we got it).

James

[toc] | [prev] | [next] | [standalone]

#1388546 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

>> The last point is what Denys introduced in the kernel with
>> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of
>> some byteswap operations"). So maybe it's better after all to revert
>> that patch, to have a higher confidence in the same bug not appearing
>> elsewhere. It's also really a workaround for another quirk of the
>> compiler, but that one only results in duplicated functions in object
>> code rather than functions that end in the middle.

James> Yes, I think this is my preferred option.

Same here.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1388898 — Re: [PATCH] scsi: fc: force inlining of wwn conversion functions

Hi,

On Tue, Apr 26, 2016 at 05:58:20PM +0200, Arnd Bergmann wrote:
> On Tuesday 26 April 2016 09:06:54 Martin K. Petersen wrote:
> > >>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> > 
> > Arnd> I don't think we can realistically blacklist gcc-4.9.{0,1,2,3},
> > Arnd> gcc-5.{0,1,2,3}.* and gcc-6.0 and require everyone to upgrade to
> > Arnd> compilers that have not been released yet in order to build a
> > Arnd> linux-4.6 kernel.
> > 
> > I agree that compiler blacklisting is problematic and I'd like to avoid
> > it. The question is how far we go in the kernel to accommodate various
> > levels of brokenness.
> > 
> > In any case. Sticking compiler workarounds in device driver code is akin
> > to putting demolition orders on display on Alpha Centauri. At the very
> > minimum the patch should put a fat comment in the code stating that
> > these wrapper functions or #defines should not be changed in the future
> > because that'll break builds using gcc XYZ. But that does not solve the
> > problem for anybody else that might be doing something similar.
> > Converting between u64 and $RANDOM_TYPE in an inline wrapper does not
> > seem like a rare and unusual programming pattern.
> 
> It's not the driver really, it's the core scsi/fc layer, which makes
> it a little dangerous that a random driver.
> 
> I agree that putting a comment in would also help. What I understand
> from the bug report is that to trigger this bug you need these elements:
> 
> 1. an inline function marked __always_inline
> 2. another inline function that is automatically inlined (not __always_inline)
> 3. CONFIG_OPTIMIZE_INLINING=y to guarantee 2
> 4. __builtin_compatible_p inside that inline function

The __always_inline requirement is not true.  In fact, if you look at
the example testcase filed in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c7 you'll see it
uses __builtin_compatible_p in an __always inline function that is
called from one that is not tagged with that attribute.

And generally speaking, always inline is never a requirement, any call
or chain of calls that the inliner can decide to inline can lead to
the bug (if it complies with the condition below).

What is a requirement, though, is that __builtin_compatible_p is
called on something passed in an argument by reference or in an
aggregate (i.e. struct or array) argument.

So,

  int foo1 (unsigned long *ref)
  {
    if (__builtin_constant (*ref))
      ...
    else
      /* wrongly unreachable code */
  }

can lead to this issue, as can

  int foo2 (struct S s)
  {
    if ((__builtin_constant (s.l))
      ...
    else
      /* wrongly unreachable code */
  }

but

  int foo3 (unsigned long val)
  {
    if (__builtin_constant (val))
      ...
    else
      /* all OK */
  }

cannot, and is fine.  But please note that wrapping a foo[12]-like
function into a dereferencing wrapper might not help if foo[12] would
be early-inlined into such wrapper (GCC has two inliners, a very
simple early-inliner that only handles simple cases and a full-blown
IPA inliner that contains the bug).  I believe this can be ensured by
making the wrapper always_inline and never calling it indirectly (via
a pointer).  Honza (CCed), you know inlining heuristics better, please
correct me if my last statement is somehow inaccurate (or indeed if
you have a better idea how kernel developers can make sure they do not
hit the bug).

Thanks,

Martin

> 
> The last point is what Denys introduced in the kernel with
> bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some
> byteswap operations"). So maybe it's better after all to revert that
> patch, to have a higher confidence in the same bug not appearing
> elsewhere. It's also really a workaround for another quirk of the
> compiler, but that one only results in duplicated functions in object
> code rather than functions that end in the middle.
> 
> 	Arnd
> 

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web