[RFC 1/2] x86: mark target address as output in 'insb' asm

[RFC 1/2] x86: mark target address as output in 'insb' asm

[Arnd Bergmann]

The -Wmaybe-uninitialized warning triggers for one driver using the output
of the 'insb' I/O helper on x86:

drivers/net/wireless/wl3501_cs.c: In function ‘wl3501_mgmt_scan_confirm’:
drivers/net/wireless/wl3501_cs.c:665:9: error: ‘sig.status’ is used uninitialized in this function [-Werror=uninitialized]
drivers/net/wireless/wl3501_cs.c:668:12: error: ‘sig.cap_info’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

Apparently the assember constraints are slightly off here, as marking the
'addr' argument as a memory output seems appropriate here and gets rid
of the warning. For consistency I'm also adding it as input for outsb().

Unfortunately, this fix triggers another problem when CONFIG_KASAN is
set, again only in this one driver:

drivers/net/wireless/wl3501_cs.c: In function 'wl3501_rx_interrupt':
drivers/net/wireless/wl3501_cs.c:1103:1: error: the frame size of 2232 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]

I'm not an x86 person and gcc inline assembly mystifies me all the time,
so please review this carefully and suggest a better way if this is not
how it should be done.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/include/asm/io.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7afb0e2f07f4..d107251eabd9 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -328,13 +328,13 @@ static inline unsigned type in##bwl##_p(int port)			\
 static inline void outs##bwl(int port, const void *addr, unsigned long count) \
 {									\
 	asm volatile("rep; outs" #bwl					\
-		     : "+S"(addr), "+c"(count) : "d"(port));		\
+		     : "+S"(addr), "+c"(count) : "d"(port), "m" (addr));\
 }									\
 									\
 static inline void ins##bwl(int port, void *addr, unsigned long count)	\
 {									\
 	asm volatile("rep; ins" #bwl					\
-		     : "+D"(addr), "+c"(count) : "d"(port));		\
+		     : "+D"(addr), "+c"(count), "=m" (addr) : "d"(port));\
 }
 
 BUILDIO(b, b, char)
-- 
2.9.0

[RFC 2/2] wl3501_cs: reduce stack size for KASAN

[Arnd Bergmann]

Inlining functions with local variables can lead to excessive stack usage
with KASAN after a previous patch that modifies the outsb/insb helpers
on x86.

drivers/net/wireless/wl3501_cs.c: In function 'wl3501_rx_interrupt':
drivers/net/wireless/wl3501_cs.c:1103:1: error: the frame size of 2232 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]

Marking the two callers of insb/outb 'noinline' prevents the compiler
from adding up the stack usage for each of the local variables passed
into those, reducing the maximum stack frame size to 800 bytes with
KASAN again.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/wireless/wl3501_cs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/wl3501_cs.c b/drivers/net/wireless/wl3501_cs.c
index acec0d9ec422..2cce22571b4c 100644
--- a/drivers/net/wireless/wl3501_cs.c
+++ b/drivers/net/wireless/wl3501_cs.c
@@ -242,8 +242,8 @@ static int wl3501_get_flash_mac_addr(struct wl3501_card *this)
  *
  * Move 'size' bytes from PC to card. (Shouldn't be interrupted)
  */
-static void wl3501_set_to_wla(struct wl3501_card *this, u16 dest, void *src,
-			      int size)
+static noinline void wl3501_set_to_wla(struct wl3501_card *this,
+				       u16 dest, void *src, int size)
 {
 	/* switch to SRAM Page 0 */
 	wl3501_switch_page(this, (dest & 0x8000) ? WL3501_BSS_SPAGE1 :
@@ -264,8 +264,8 @@ static void wl3501_set_to_wla(struct wl3501_card *this, u16 dest, void *src,
  *
  * Move 'size' bytes from card to PC. (Shouldn't be interrupted)
  */
-static void wl3501_get_from_wla(struct wl3501_card *this, u16 src, void *dest,
-				int size)
+static noinline void wl3501_get_from_wla(struct wl3501_card *this,
+					 u16 src, void *dest, int size)
 {
 	/* switch to SRAM Page 0 */
 	wl3501_switch_page(this, (src & 0x8000) ? WL3501_BSS_SPAGE1 :
-- 
2.9.0

Re: [RFC 2/2] wl3501_cs: reduce stack size for KASAN

[Kalle Valo]

Arnd Bergmann <arnd@arndb.de> writes:

> Inlining functions with local variables can lead to excessive stack usage
> with KASAN after a previous patch that modifies the outsb/insb helpers
> on x86.
>
> drivers/net/wireless/wl3501_cs.c: In function 'wl3501_rx_interrupt':
> drivers/net/wireless/wl3501_cs.c:1103:1: error: the frame size of 2232 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]
>
> Marking the two callers of insb/outb 'noinline' prevents the compiler
> from adding up the stack usage for each of the local variables passed
> into those, reducing the maximum stack frame size to 800 bytes with
> KASAN again.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Arnd, based on the discussion I'm dropping. Please let me know if I
should take this still.

-- 
Kalle Valo

Re: [RFC 2/2] wl3501_cs: reduce stack size for KASAN

[Arnd Bergmann]

On Tue, Jul 25, 2017 at 2:52 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
>
>> Inlining functions with local variables can lead to excessive stack usage
>> with KASAN after a previous patch that modifies the outsb/insb helpers
>> on x86.
>>
>> drivers/net/wireless/wl3501_cs.c: In function 'wl3501_rx_interrupt':
>> drivers/net/wireless/wl3501_cs.c:1103:1: error: the frame size of 2232 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]
>>
>> Marking the two callers of insb/outb 'noinline' prevents the compiler
>> from adding up the stack usage for each of the local variables passed
>> into those, reducing the maximum stack frame size to 800 bytes with
>> KASAN again.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Arnd, based on the discussion I'm dropping. Please let me know if I
> should take this still.

Thanks, that's good. The problem has become unreproducible and
I assume it's gone for good with the new x86 fix. In the unlikely
case some form of the problem comes back in another randconfig,
I'll post a new patch.

       Arnd

[tip:x86/urgent] x86/io: Mark target address as output in 'insb()' asm

[tip-bot for Arnd Bergmann]

Commit-ID:  ea1b2ee6eacc4413b1dfba566480c90d0da4ec81
Gitweb:     http://git.kernel.org/tip/ea1b2ee6eacc4413b1dfba566480c90d0da4ec81
Author:     Arnd Bergmann <arnd@arndb.de>
AuthorDate: Mon, 10 Jul 2017 16:44:24 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 12 Jul 2017 10:42:32 +0200

x86/io: Mark target address as output in 'insb()' asm

The -Wmaybe-uninitialized warning triggers for one driver using the output
of the 'insb' I/O helper on x86:

  drivers/net/wireless/wl3501_cs.c: In function ‘wl3501_mgmt_scan_confirm’:
  drivers/net/wireless/wl3501_cs.c:665:9: error: ‘sig.status’ is used uninitialized in this function [-Werror=uninitialized]
  drivers/net/wireless/wl3501_cs.c:668:12: error: ‘sig.cap_info’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

Apparently the assember constraints are slightly off here, as marking the
'addr' argument as a memory output seems appropriate here and gets rid
of the warning. For consistency I'm also adding it as input for outsb().

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-wireless@vger.kernel.org
Link: http://lkml.kernel.org/r/20170710144425.2238584-1-arnd@arndb.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/io.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7afb0e2..d107251 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -328,13 +328,13 @@ static inline unsigned type in##bwl##_p(int port)			\
 static inline void outs##bwl(int port, const void *addr, unsigned long count) \
 {									\
 	asm volatile("rep; outs" #bwl					\
-		     : "+S"(addr), "+c"(count) : "d"(port));		\
+		     : "+S"(addr), "+c"(count) : "d"(port), "m" (addr));\
 }									\
 									\
 static inline void ins##bwl(int port, void *addr, unsigned long count)	\
 {									\
 	asm volatile("rep; ins" #bwl					\
-		     : "+D"(addr), "+c"(count) : "d"(port));		\
+		     : "+D"(addr), "+c"(count), "=m" (addr) : "d"(port));\
 }
 
 BUILDIO(b, b, char)

Re: [tip:x86/urgent] x86/io: Mark target address as output in 'insb()' asm

[Linus Torvalds]

On Wed, Jul 12, 2017 at 6:10 AM, tip-bot for Arnd Bergmann
<tipbot@zytor.com> wrote:
>
> Apparently the assember constraints are slightly off here, as marking the
> 'addr' argument as a memory output seems appropriate here and gets rid
> of the warning. For consistency I'm also adding it as input for outsb().

The new constraints look very questionable to me.

>         asm volatile("rep; outs" #bwl                                   \
> -                    : "+S"(addr), "+c"(count) : "d"(port));            \
> +                    : "+S"(addr), "+c"(count) : "d"(port), "m" (addr));\

>         asm volatile("rep; ins" #bwl                                    \
> -                    : "+D"(addr), "+c"(count) : "d"(port));            \
> +                    : "+D"(addr), "+c"(count), "=m" (addr) : "d"(port));\

That's not how "m" works, afaik. You're passing in an address, but "m"
takes the _value_.

So as far as I can tell, what you are really doing is say "this
reads/writes the memory that contains the _pointer_".

So not only does it not do what you think it does, it probably
actually forces "addr" to be loaded into a stack slot, in order for
the inline asm to be able to "access" that memory location to get (and
set) the value of "addr".

So if it hides warnings, it does so by virtue of confusing gcc some
more about what is actually going on, rather than by fixing the issue.

I do agree that those inline asm things do lack a memory dependency,
though. I just think that patch is *completely* wrong.

The real fix is probably to just mark them as "clobbers memory" (ie
just add "memory" to the clobber list).

If you want to be fancy, you can try to do what <asm/uaccess.h> does,
which is a disgusting hack, but has traditionally worked;

  struct __large_struct { unsigned long buf[100]; };
  #define __m(x) (*(struct __large_struct __user *)(x))

and then use your approach with "m" and "=m".

               Linus

Re: [tip:x86/urgent] x86/io: Mark target address as output in 'insb()' asm

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Jul 12, 2017 at 6:10 AM, tip-bot for Arnd Bergmann
> <tipbot@zytor.com> wrote:
> >
> > Apparently the assember constraints are slightly off here, as marking the
> > 'addr' argument as a memory output seems appropriate here and gets rid
> > of the warning. For consistency I'm also adding it as input for outsb().
> 
> The new constraints look very questionable to me.

Ok, I've removed the commit.

> The real fix is probably to just mark them as "clobbers memory" (ie
> just add "memory" to the clobber list).
> 
> If you want to be fancy, you can try to do what <asm/uaccess.h> does,
> which is a disgusting hack, but has traditionally worked;
> 
>   struct __large_struct { unsigned long buf[100]; };
>   #define __m(x) (*(struct __large_struct __user *)(x))
> 
> and then use your approach with "m" and "=m".

Arnd, could you please try Linus's suggestions?

Thanks,

	Ingo

Re: [tip:x86/urgent] x86/io: Mark target address as output in 'insb()' asm

[Arnd Bergmann]

On Wed, Jul 12, 2017 at 6:57 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Jul 12, 2017 at 6:10 AM, tip-bot for Arnd Bergmann
> <tipbot@zytor.com> wrote:
>>
>> Apparently the assember constraints are slightly off here, as marking the
>> 'addr' argument as a memory output seems appropriate here and gets rid
>> of the warning. For consistency I'm also adding it as input for outsb().
>
> The new constraints look very questionable to me.
>
>>         asm volatile("rep; outs" #bwl                                   \
>> -                    : "+S"(addr), "+c"(count) : "d"(port));            \
>> +                    : "+S"(addr), "+c"(count) : "d"(port), "m" (addr));\
>
>>         asm volatile("rep; ins" #bwl                                    \
>> -                    : "+D"(addr), "+c"(count) : "d"(port));            \
>> +                    : "+D"(addr), "+c"(count), "=m" (addr) : "d"(port));\
>
> That's not how "m" works, afaik. You're passing in an address, but "m"
> takes the _value_.
>
> So as far as I can tell, what you are really doing is say "this
> reads/writes the memory that contains the _pointer_".
>
> So not only does it not do what you think it does, it probably
> actually forces "addr" to be loaded into a stack slot, in order for
> the inline asm to be able to "access" that memory location to get (and
> set) the value of "addr".

Ok, got it, thanks for taking a look!

> So if it hides warnings, it does so by virtue of confusing gcc some
> more about what is actually going on, rather than by fixing the issue.

Right, as far as gcc is concerned, 'addr' might now point to something
that was initialized, so it no longer warns even though it still thinks
that *addr did not get touched.

> I do agree that those inline asm things do lack a memory dependency,
> though. I just think that patch is *completely* wrong.
>
> The real fix is probably to just mark them as "clobbers memory" (ie
> just add "memory" to the clobber list).
>
> If you want to be fancy, you can try to do what <asm/uaccess.h> does,
> which is a disgusting hack, but has traditionally worked;
>
>   struct __large_struct { unsigned long buf[100]; };
>   #define __m(x) (*(struct __large_struct __user *)(x))
>
> and then use your approach with "m" and "=m".

Ok, I'll try both tomorrow and see where I end up.

I guess it's not urgent to fix it since that code has literally
been there since linux-0.1 (first in hd.c, and moved to
asm/io.h in 0.99.17k).

Would you expect that the missing clobber causes actual
runtime bugs and the fix needs to be backported to stable
kernels?

     Arnd

Re: [tip:x86/urgent] x86/io: Mark target address as output in 'insb()' asm

[Linus Torvalds]

On Wed, Jul 12, 2017 at 2:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> The real fix is probably to just mark them as "clobbers memory" (ie
>> just add "memory" to the clobber list).
>>
>> If you want to be fancy, you can try to do what <asm/uaccess.h> does,
>> which is a disgusting hack, but has traditionally worked;
>>
>>   struct __large_struct { unsigned long buf[100]; };
>>   #define __m(x) (*(struct __large_struct __user *)(x))
>>
>> and then use your approach with "m" and "=m".
>
> Ok, I'll try both tomorrow and see where I end up.

Note that just adding the "memory" thing to the clobbers likely causes
slightly worse code generation (it basically says that the asm can
clobber anything at all, so cause re-loads etc that are entirely
unrelated to the asm).

But for something like "rep in/out", that really doesn't much matter.
PIO is very slow due to being fully serialized, and "rep ins/outs" is
just about the slowest thing you can do on a machine. So nobody really
does it, the main traditional user was the legacy PIO data transfer
for ST-506 disks. And, as you noticed a few *really* old network card
drivers.

So I suspect the big hammer memory clobber is the right thing to do.

> Would you expect that the missing clobber causes actual
> runtime bugs and the fix needs to be backported to stable
> kernels?

Probably not. "asm volatile" is already pretty serialized. It's not
entirely obvious what gcc will move around it, but almost certainly no
operations that matter for the network buffer.

And honestly, the wt3501 is basically an ISA card in PCMCIA format. I
don't think it's even cardbus (Cardbus aka Yenta is basically "hotplug
PCI"). So I don't think anybody even has that hardware.

It was a good card for its time. But its time was basically two decades ago.

                  Linus