Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Ingo Molnar' <mingo@kernel.org>,
        Alexander Potapenko <glider@google.com>
CC:     "paulmck@linux.ibm.com" <paulmck@linux.ibm.com>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dvyukov@google.com" <dvyukov@google.com>,
        "jyknight@google.com" <jyknight@google.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Borislav Petkov <bp@alien8.de>
Subject: RE: [PATCH v2] x86/asm: fix assembly constraints in bitops
Thread-Topic: [PATCH v2] x86/asm: fix assembly constraints in bitops
Thread-Index: AQHU65N45ims1sqDDECRt8+l+UbnY6YtaHaA
Date:   Fri, 5 Apr 2019 11:12:03 +0000
Message-ID: <00ac2060e69c4e06915ca51c1308a73e@AcuMS.aculab.com>
References: <20190402112813.193378-1-glider@google.com>
 <20190405093931.GA28890@gmail.com>
In-Reply-To: <20190405093931.GA28890@gmail.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

From: Ingo Molnar
> Sent: 05 April 2019 10:40
> 
> * Alexander Potapenko <glider@google.com> wrote:
> 
> > 1. Use memory clobber in bitops that touch arbitrary memory
> >
> > Certain bit operations that read/write bits take a base pointer and an
> > arbitrarily large offset to address the bit relative to that base.
> > Inline assembly constraints aren't expressive enough to tell the
> > compiler that the assembly directive is going to touch a specific memory
> > location of unknown size, therefore we have to use the "memory" clobber
> > to indicate that the assembly is going to access memory locations other
> > than those listed in the inputs/outputs.
> > To indicate that BTR/BTS instructions don't necessarily touch the first
> > sizeof(long) bytes of the argument, we also move the address to assembly
> > inputs.
> >
> > This particular change leads to size increase of 124 kernel functions in
> > a defconfig build. For some of them the diff is in NOP operations, other
> > end up re-reading values from memory and may potentially slow down the
> > execution. But without these clobbers the compiler is free to cache
> > the contents of the bitmaps and use them as if they weren't changed by
> > the inline assembly.
> >
> > 2. Use byte-sized arguments for operations touching single bytes.
> >
> > Passing a long value to ANDB/ORB/XORB instructions makes the compiler
> > treat sizeof(long) bytes as being clobbered, which isn't the case. This
> > may theoretically lead to worse code in the case of heavy optimization.
> >
...
> 
> I'm wondering what the primary motivation for the patch is:
> 
>  - Does it fix an actual miscompilation, or only a theoretical miscompilation?
> 
>  - If it fixes an existing miscompilation:
> 
>    - Does it fix a miscompilation triggered by current/future versions of GCC?
>    - Does it fix a miscompilation triggered by current/future versions of Clang?
> 
>  - Also, is the miscompilation triggered by 'usual' kernel configs, or
>    does it require exotics such as weird debug options or GCC plugins,
>    etc?
> 
> I.e. a bit more context would be useful.

The missing memory clobber (change 1) can cause very difficult to debug bugs.
Simple things like gcc deciding to inline a function can change the order
of memory accesses.
Having the wrong just isn't worth the trouble it can cause.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)