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

Groups > linux.kernel > #1685914

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC 1/2] x86: mark target address as output in 'insb' asm Arnd Bergmann <arnd@arndb.de> - 2017-07-10 16:50 +0200
  [RFC 2/2] wl3501_cs: reduce stack size for KASAN Arnd Bergmann <arnd@arndb.de> - 2017-07-10 16:50 +0200
  [tip:x86/urgent] x86/io: Mark target address as output in 'insb()'  asm tip-bot for Arnd Bergmann <tipbot@zytor.com> - 2017-07-12 15:20 +0200
    Re: [tip:x86/urgent] x86/io: Mark target address as output in  'insb()' asm Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-12 19:00 +0200
      Re: [tip:x86/urgent] x86/io: Mark target address as output in  'insb()' asm Ingo Molnar <mingo@kernel.org> - 2017-07-12 21:30 +0200
      Re: [tip:x86/urgent] x86/io: Mark target address as output in  'insb()' asm Arnd Bergmann <arnd@arndb.de> - 2017-07-12 23:50 +0200
        Re: [tip:x86/urgent] x86/io: Mark target address as output in  'insb()' asm Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-13 00:50 +0200

csiph-web