Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution

[Josh Poimboeuf]

On Tue, Jan 09, 2018 at 11:44:05AM -0800, Dan Williams wrote:
> On Tue, Jan 9, 2018 at 11:34 AM, Jiri Kosina  wrote:
> > On Fri, 5 Jan 2018, Dan Williams wrote:
> >
> > [ ... snip ... ]
> >> Andi Kleen (1):
> >>   x86, barrier: stop speculation for failed access_ok
> >>
> >> Dan Williams (13):
> >>   x86: implement nospec_barrier()
> >>   [media] uvcvideo: prevent bounds-check bypass via speculative 
> >> execution
> >>   carl9170: prevent bounds-check bypass via speculative execution
> >>   p54: prevent bounds-check bypass via speculative execution
> >>   qla2xxx: prevent bounds-check bypass via speculative execution
> >>   cw1200: prevent bounds-check bypass via speculative execution
> >>   Thermal/int340x: prevent bounds-check bypass via speculative 
> >> execution
> >>   ipv6: prevent bounds-check bypass via speculative execution
> >>   ipv4: prevent bounds-check bypass via speculative execution
> >>   vfs, fdtable: prevent bounds-check bypass via speculative execution
> >>   net: mpls: prevent bounds-check bypass via speculative execution
> >>   udf: prevent bounds-check bypass via speculative execution
> >>   userns: prevent bounds-check bypass via speculative execution
> >>
> >> Mark Rutland (4):
> >>   asm-generic/barrier: add generic nospec helpers
> >>   Documentation: document nospec helpers
> >>   arm64: implement nospec_ptr()
> >>   arm: implement nospec_ptr()
> >
> > So considering the recent publication of [1], how come we all of a sudden
> > don't need the barriers in ___bpf_prog_run(), namely for LD_IMM_DW and
> > LDX_MEM_##SIZEOP, and something comparable for eBPF JIT?
> >
> > Is this going to be handled in eBPF in some other way?
> >
> > Without that in place, and considering Jann Horn's paper, it would seem
> > like PTI doesn't really lock it down fully, right?
> 
> Here is the latest (v3) bpf fix:
> 
> https://patchwork.ozlabs.org/patch/856645/
> 
> I currently have v2 on my 'nospec' branch and will move that to v3 for
> the next update, unless it goes upstream before then.

That patch seems specific to CONFIG_BPF_SYSCALL.  Is the bpf() syscall
the only attack vector?  Or are there other ways to run bpf programs
that we should be worried about?

-- 
Josh

Re: [PATCH, RFT] byteswap: try to avoid __builtin_constant_p gcc bug

[Josh Poimboeuf]

On Thu, Apr 28, 2016 at 12:00:36AM +0200, Arnd Bergmann wrote:
> This is another attempt to avoid a regression in wwn_to_u64()
> after that started using get_unaligned_be64(), which in turn
> ran into a bug on gcc-4.9 through 6.1.
> 
> As part of the problem is how __builtin_constant_p gets evaluated
> on an argument passed by reference into an inline function, this
> avoids the use of __builtin_constant_p() for all architectures
> that set CONFIG_ARCH_USE_BUILTIN_BSWAP. Most architectures do not
> set ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably
> do not suffer from the problem in the qla2xxx driver, but they
> might still run into it elsewhere.
> 
> I have not been able to reproduce the original problem, so I don't
> know if this patch solves it, but at least it leads to simpler
> code doing the same thing, so at least there should be no downsides.
> 
> Please test.
> 
> Signed-off-by: Arnd Bergmann <a...@arndb.de>

Nice patch.  I can confirm it fixes the issue with gcc 5.3.1.

Tested-by: Josh Poimboeuf <jpoim...@redhat.com>
Reviewed-by: Josh Poimboeuf <jpoim...@redhat.com>

> diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
> index 3f10e5317b46..de56fd54428d 100644
> --- a/include/uapi/linux/swab.h
> +++ b/include/uapi/linux/swab.h
> @@ -45,9 +45,7 @@
>  
>  static inline __attribute_const__ __u16 __fswab16(__u16 val)
>  {
> -#ifdef __HAVE_BUILTIN_BSWAP16__
> - return __builtin_bswap16(val);
> -#elif defined (__arch_swab16)
> +#if defined (__arch_swab16)
>   return __arch_swab16(val);
>  #else
>   return ___constant_swab16(val);
> @@ -56,9 +54,7 @@ static inline __attribute_const__ __u16 __fswab16(__u16 val)
>  
>  static inline __attribute_const__ __u32 __fswab32(__u32 val)
>  {
> -#ifdef __HAVE_BUILTIN_BSWAP32__
> - return __builtin_bswap32(val);
> -#elif defined(__arch_swab32)
> +#if defined(__arch_swab32)
>   return __arch_swab32(val);
>  #else
>   return ___constant_swab32(val);
> @@ -67,9 +63,7 @@ static inline __attribute_const__ __u32 __fswab32(__u32 val)
>  
>  static inline __attribute_const__ __u64 __fswab64(__u64 val)
>  {
> -#ifdef __HAVE_BUILTIN_BSWAP64__
> - return __builtin_bswap64(val);
> -#elif defined (__arch_swab64)
> +#if defined (__arch_swab64)
>   return __arch_swab64(val);
>  #elif defined(__SWAB_64_THRU_32__)
>   __u32 h = val >> 32;
> @@ -102,28 +96,40 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 
> val)
>   * __swab16 - return a byteswapped 16-bit value
>   * @x: value to byteswap
>   */
> +#ifdef __HAVE_BUILTIN_BSWAP16__
> +#define __swab16(x) __builtin_bswap16((__u16)(x))
> +#else
>  #define __swab16(x)  \
>   (__builtin_constant_p((__u16)(x)) ? \
>   ___constant_swab16(x) : \
>   __fswab16(x))
> +#endif
>  
>  /**
>   * __swab32 - return a byteswapped 32-bit value
>   * @x: value to byteswap
>   */
> +#ifdef __HAVE_BUILTIN_BSWAP32__
> +#define __swab32(x) __builtin_bswap32((__u32)(x))
> +#else
>  #define __swab32(x)  \
>   (__builtin_constant_p((__u32)(x)) ? \
>   ___constant_swab32(x) : \
>   __fswab32(x))
> +#endif
>  
>  /**
>   * __swab64 - return a byteswapped 64-bit value
>   * @x: value to byteswap
>   */
> +#ifdef __HAVE_BUILTIN_BSWAP64__
> +#define __swab64(x) __builtin_bswap64((__u64)(x))
> +#else
>  #define __swab64(x)  \
>   (__builtin_constant_p((__u64)(x)) ? \
>   ___constant_swab64(x) : \
>   __fswab64(x))
> +#endif
>  
>  /**
>   * __swahw32 - return a word-swapped 32-bit value
> 

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

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:
> 
>   2f53 :
>   2f53:   55  push   %rbp
>   2f54:   48 89 e5mov%rsp,%rbp
> 
>   2f57 :
>   2f57:   55  push   %rbp
>   2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
>   2f5d:   48 89 e5mov%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...@intel.com>
> Signed-off-by: Josh Poimboeuf <jpoim...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

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:

  2f53 :
  2f53:   55  push   %rbp
  2f54:   48 89 e5mov%rsp,%rbp

  2f57 :
  2f57:   55  push   %rbp
  2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
  2f5d:   48 89 e5mov%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...@intel.com>
Signed-off-by: Josh Poimboeuf <jpoim...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

On Sat, Apr 16, 2016 at 11:03:32AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoim...@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 qla2 code:
> > > 
> > >   
> > > https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

On Sat, Apr 16, 2016 at 09:42:37AM +0200, Arnd Bergmann wrote:
> On Friday 15 April 2016 07:45:19 Ingo Molnar wrote:
> > 
> > * Denys Vlasenko  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?
> > 
> > Which, by the way, is pretty cool!
> 
> I have done a patch for the asm-generic/unaligned handling recently that
> reworks the implementation to avoid an ARM specific bug (gcc uses certain
> CPU instructions that require aligned data when we tell it that unaligned
> data is not).
> 
> It changes the code enough that the gcc bug might not be triggered any more,
> aside from generating far superior code in some cases.

I tried this patch, but unfortunately it doesn't make the gcc bug go
away.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

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 <dvlas...@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 qla2 code:
> 
>   
> https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] qla2xxx: rewrite code to avoid hitting gcc bug 70646

[Josh Poimboeuf]

On Fri, Apr 15, 2016 at 12:05:26PM -0700, James Bottomley wrote:
> On Fri, 2016-04-15 at 20:56 +0200, Denys Vlasenko wrote:
> > and now *many* users of qla2x00 and new-ish gcc are going to
> > very much notice it, as their kernels will start crashing reliably.
> > 
> > The commits can be reverted, sure, but they per se do not contain
> > anything unusual. They, together with not very typical construct
> > in qla2x00_get_host_fabric_name, one
> > which boils down to "swab64p(constant_array_of_8_bytes)",
> > just happen to nudge gcc in a right way to finally trigger the bug.
> > 
> > So I came with another idea how to forestall the imminent deluge of
> > qla2x00 oops reports - this patch.
> 
> There are actually a raft of checkers that run the upstream code which
> aren't seeing any problem; likely because the code is harder to trigger
> than you think.  So, lets wait until the resolution of the other thread
> before we panic, especially since we're only at -rc3.

Regardless of the outcome of the gcc bug, it seems kind of silly to
byteswap a constant value of 0x.

uint8_t node_name[WWN_SIZE] = { 0xFF, 0xFF, 0xFF, 0xFF, \
0xFF, 0xFF, 0xFF, 0xFF};
u64 fabric_name = wwn_to_u64(node_name);

Similar to what Denys suggested, it can just be:

u64 fabric_name = -1;
or
u64 fabric_name = 0x;

Wouldn't that be an improvement to the code regardless?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

On Fri, Apr 15, 2016 at 07:45:19AM +0200, Ingo Molnar wrote:
> 
> * Denys Vlasenko  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 qla2 code:

  
https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@redhat.com


-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Poimboeuf]

On Thu, Apr 14, 2016 at 05:29:06PM +0200, Denys Vlasenko wrote:
> On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
> >>>>>> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> >>>>>>
> >>>>>> 2f53 :
> >>>>>> 2f53:   55  push   %rbp
> >>>>>> 2f54:   48 89 e5mov%rsp,%rbp
> >>>>>>
> >>>>>> 2f57 :
> >>>>>> 2f57:   55  push   %rbp
> >>>>>> 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
> >>>>>> 2f5d:   48 89 e5mov%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 wrong.
> >>>>>
> >>>>> Wow, that's ... interesting.
> >>>>>
> >>>>>
> >>>>>> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> >>>>>> linus/master with
> >>>>>> the .config from the above link.
> >>>>>>
> >>>>>> 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() <- changed to __always_inline by this
> >>>>>> patch
> >>>>>>
> >>>>>> 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")
> >>>>>>
> >>>>>> I can confirm that reverting either patch makes the problem go
> >>>>>> away.
> >>>>>> I'm planning on opening a gcc bug tomorrow.
> >>>>>
> >>>>>
> >>>>> Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> >>>>> keywords are in fact __always_inline, so the bug must be
> >>>>> triggering
> >>>>> even without the patch.
> >>>>
> >>>> Makes sense in theory, but the bug doesn't actually trigger when I
> >>>> revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> >>>>
> >>>> Perhaps even more surprising, it doesn't trigger *with* the patch
> >>>> and
> >>>> CONFIG_OPTIMIZE_INLINING=n.
> >>>
> >>> [ Adding James to CC since this bug affects scsi. ]
> >>>
> >>> Here's the gcc bug:
> >>>
> >>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> >>>
> >>
> >>
> >> Actually, adding me doesn't help, I've added linux-scsi.  The summary
> >> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
> >> this means we're going to have to ask the compiler version of reported
> >> crashes.
> > 
> > The bug isn't specific to a compiler version.  I've seen it with gcc
> > 5.3.1 and gcc 6.0.  I haven't tried any older versions.  And the gcc bug
> > hasn't been resolved (or even investigated) yet.
> > 
> > The bug is triggered by a combination of the above two commits from the
> > 4.6 merge window, so presumably we'd need to revert one of them to avoid
> > crashes in 4.6.
> 
> The bug is indeed in the compiler. 4.9 and all later versions are affected.
> gcc bugzilla now has a reproducer. In abridged form:
> 
> 
> static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
> {
>  return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & 
> (u64)0x00ffULL) << 56) | (((u64)(*p) & 
> (u64)0xff00ULL) << 40) | (((u64)(*p) & 
> (u64)0x00ffULL) << 24) | (((u64)(*p) & 
> (u64)0xff00ULL) << 8) | (((u64)(*p) & (u64)0x00ff

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

[Josh Poimboeuf]

On Wed, Apr 13, 2016 at 09:55:09AM -0700, James Bottomley wrote:
> On Wed, 2016-04-13 at 10:15 -0500, Josh Poimboeuf wrote:
> > On Wed, Apr 13, 2016 at 07:36:07AM -0500, Josh Poimboeuf wrote:
> > > On Wed, Apr 13, 2016 at 02:12:25PM +0200, Denys Vlasenko wrote:
> > > > On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> > > > > On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> > > > > > Sometimes gcc mysteriously doesn't inline
> > > > > > very small functions we expect to be inlined. See
> > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> > > > > > 
> > > > > > With this .config:
> > > > > > http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_O
> > > > > > s,
> > > > > > the following functions get deinlined many times.
> > > > > > Examples of disassembly:
> > > > > > 
> > > > > >  (12 copies, 51 calls):
> > > > > >66 8b 07mov(%rdi),%ax
> > > > > >55  push   %rbp
> > > > > >48 89 e5mov%rsp,%rbp
> > > > > >86 e0   xchg   %ah,%al
> > > > > >5d  pop%rbp
> > > > > >c3  retq
> > > > ...
> > > > > > This patch fixes this via s/inline/__always_inline/.
> > > > > > Code size decrease after the patch is ~4.5k:
> > > > > > 
> > > > > > text data  bss   dec hex filename
> > > > > > 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> > > > > > 92197848 20826112 36417536 149441496 8e84bd8
> > > > > > vmlinux5_swap_after
> > > > > > 
> > > > > > Signed-off-by: Denys Vlasenko <dvlas...@redhat.com>
> > > > > > Cc: Ingo Molnar <mi...@kernel.org>
> > > > > > Cc: Thomas Graf <tg...@suug.ch>
> > > > > > Cc: Peter Zijlstra <pet...@infradead.org>
> > > > > > Cc: David Rientjes <rient...@google.com>
> > > > > > Cc: Andrew Morton <a...@linux-foundation.org>
> > > > > > Cc: linux-ker...@vger.kernel.org
> > > > > > ---
> > > > > >  include/uapi/linux/byteorder/big_endian.h| 24
> > > > > > 
> > > > > >  include/uapi/linux/byteorder/little_endian.h | 24
> > > > > > 
> > > > > >  include/uapi/linux/swab.h| 10 +-
> > > > > >  3 files changed, 29 insertions(+), 29 deletions(-)
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > This patch seems to trigger a gcc bug which can produce corrupt
> > > > > code.  I
> > > > > discovered it when investigating an objtool warning reported by
> > > > > kbuild
> > > > > bot:
> > > > > 
> > > > >   http://www.spinics.net/lists/linux-scsi/msg95481.html
> > > > > 
> > > > > From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> > > > > 
> > > > > 2f53 :
> > > > > 2f53:   55  push   %rbp
> > > > > 2f54:   48 89 e5mov%rsp,%rbp
> > > > > 
> > > > > 2f57 :
> > > > > 2f57:   55  push   %rbp
> > > > > 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
> > > > > 2f5d:   48 89 e5mov%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 wrong.
> > > > 
> > > > Wow, that's ... interesting.
> > > > 
> > > > 
> > > > > I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> > > > > linus/master with
> > > > > the .config from the above link.
> > > > > 
> > > > > The call chain which appears to trigger the problem is:
> > > > > 
> > > > > ql

Re: [PATCH v2 6/8] tools rpmb: add RPBM access tool

[Josh Poimboeuf]

Hi Tomas,

On Mon, Apr 04, 2016 at 02:11:22PM +0300, Tomas Winkler wrote:
> Add simple RPMB host testing tool. It can be used
> to program key, write and read data block, and retrieve
> write counter.
> 
> Signed-off-by: Tomas Winkler 
> ---
> V2: resend
>  MAINTAINERS   |   1 +
>  tools/Makefile|  16 +-
>  tools/rpmb/.gitignore |   2 +
>  tools/rpmb/Makefile   |  32 ++
>  tools/rpmb/rpmb.c | 807 
> ++
>  5 files changed, 853 insertions(+), 5 deletions(-)
>  create mode 100644 tools/rpmb/.gitignore
>  create mode 100644 tools/rpmb/Makefile
>  create mode 100644 tools/rpmb/rpmb.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 07bd6f380460..b7966b95c265 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9425,6 +9425,7 @@ F:  drivers/char/rpmb/*
>  F:   include/uapi/linux/rpmb.h
>  F:   include/linux/rpmb.h
>  F:   Documentation/ABI/testing/sysfs-class-rpmb
> +F:   tools/rpmb/
>  
>  RTL2830 MEDIA DRIVER
>  M:   Antti Palosaari 
> diff --git a/tools/Makefile b/tools/Makefile
> index 60c7e6c8ff17..5b37ccd95cab 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -19,6 +19,7 @@ help:
>   @echo '  lguest - a minimal 32-bit x86 hypervisor'
>   @echo '  net- misc networking tools'
>   @echo '  perf   - Linux performance measurement and 
> analysis tool'
> + @echo '  rpmb   - Replay protected memory block access 
> tool'
>   @echo '  selftests  - various kernel selftests'
>   @echo '  spi- spi tools'
>   @echo '  objtool- an ELF object analysis tool'
> @@ -55,7 +56,8 @@ acpi: FORCE
>  cpupower: FORCE
>   $(call descend,power/$@)
>  
> -cgroup firewire hv guest spi usb virtio vm net iio gpio objtool: FORCE
> +cgroup firewire hv guest rpmb spi usb virtio vm net iio gpio objtool: FORCE
> +cgroup firewire hv guest rpmb spi usb virtio vm net iio: FORCE

This looks like an artifact from a bad merge resolution.  The second
line needs to be deleted.  This probably explains the kbuild robot build
errors, since CONFIG_STACK_VALIDATION requires objtool to be built.

>   $(call descend,$@)
>  
>  liblockdep: FORCE
> @@ -85,7 +87,7 @@ freefall: FORCE
>   $(call descend,laptop/$@)
>  
>  all: acpi cgroup cpupower hv firewire lguest \
> - perf selftests turbostat usb \
> + perf rpmb selftests turbostat usb \
>   virtio vm net x86_energy_perf_policy \
>   tmon freefall objtool
>  
> @@ -95,7 +97,8 @@ acpi_install:
>  cpupower_install:
>   $(call descend,power/$(@:_install=),install)
>  
> -cgroup_install firewire_install hv_install lguest_install perf_install 
> usb_install virtio_install vm_install net_install objtool_install:
> +cgroup_install firewire_install hv_install lguest_install perf_install 
> rpmb_install usb_install virtio_install vm_install net_install 
> objtool_install:
> +cgroup_install firewire_install hv_install lguest_install perf_install 
> rpmb_install usb_install virtio_install vm_install net_install:

Same here.


-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html