Re: [PATCH v7 2/5] x86/asm: add volatile, clobbers and zero-length check in inline memcmp

[Brian Gerst <brgerst@xxxxxxxxx>]

On Thu, Jul 23, 2026 at 7:13 PM H. Peter Anvin <hpa@xxxxxxxxx> wrote:
>
> On 2026-07-23 12:37, Brian Gerst wrote:
> > On Wed, Jul 22, 2026 at 2:53 PM H. Peter Anvin <hpa@xxxxxxxxx> wrote:
> >>
> >> On July 22, 2026 10:03:34 AM PDT, Borislav Petkov <bp@xxxxxxxxx> wrote:
> >>> hpa in To:.
> >>>
> >>> On Tue, Jul 21, 2026 at 12:56:43PM -0300, Mauricio Faria de Oliveira 
> >>> wrote:
> >>>> Add the volatile qualifier and clobbers parameter to prevent bugs with
> >>>> instruction reordering and optimization.
> >>>>
> >>>> Also check the zero-length case, as the 'repe' prefix does not run the
> >>>> 'cmpsb' instruction if the 'count' register is zero, which doesn't set
> >>>
> >>> Please use capital letters for insns: REPE, CMPSB and you don't need to 
> >>> put
> >>> words in '' - it reads fine without them.
> >>>
> >>>> the condition-code/zero flag, so the result is based on a stale flag.
> >>>>
> >>>> Those are pre-existing issues found by Sashiko.
> >>>>
> >>>> Link: 
> >>>> https://sashiko.dev/#/patchset/20260701-pvh-kasan-inline-v6-0-ba99045dfa9f%40igalia.com
> >>>> Signed-off-by: Mauricio Faria de Oliveira <mfo@xxxxxxxxxx>
> >>>> ---
> >>>>  arch/x86/include/asm/shared/string.h | 8 ++++++--
> >>>>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/include/asm/shared/string.h 
> >>>> b/arch/x86/include/asm/shared/string.h
> >>>> index 
> >>>> 02b92927553f7b8e1c87e6122bbaa70439e57ea7..166274e44f3cb49e3dccab3cdac281d67aef5d44
> >>>>  100644
> >>>> --- a/arch/x86/include/asm/shared/string.h
> >>>> +++ b/arch/x86/include/asm/shared/string.h
> >>>> @@ -11,8 +11,12 @@ static __always_inline int __inline_memcmp(const void 
> >>>> *s1, const void *s2, size_
> >>>>  {
> >>>>      bool diff;
> >>>>
> >>>> -    asm("repe cmpsb"
> >>>> -        : "=@ccnz" (diff), "+D" (s1), "+S" (s2), "+c" (len));
> >>>> +    if (len == 0)
> >>>> +            return 0;
> >>>> +
> >>>> +    asm volatile("repe cmpsb"
> >>>> +                 : "=@ccnz" (diff), "+D" (s1), "+S" (s2), "+c" (len)
> >>>> +                 : : "cc", "memory");
> >>>>
> >>>>      return diff;
> >>>>  }
> >>>
> >>> So this is a fix which should probably go to stable, I think. Going back 
> >>> into
> >>> git history, it points to
> >>>
> >>>  62bd0337d0c4 ("Top header file for new x86 setup code")
> >>>
> >> >from 2007. And we have carried it this way through the years and who knows
> >>> what hit this or not. So please make this the first patch in your set - 
> >>> you
> >>> can even send it separately so that I can get route it through stable.
> >>>
> >>> Then you can base the rest ontop.
> >>>
> >>> Thx.
> >>>
> >>
> >> Also, this is silly. Instead of adding a whole separate test, just do 
> >> "test %3,%3" before the repe to set ZF and let the REPE skip.
> >
> > REPE does not check ZF before the first iteration.
> >
> > From the Intel SDM:
> > "When the REPE/REPZ and REPNE/REPNZ prefixes are used, the ZF flag
> > does not require initialization because both
> > the CMPS and SCAS instructions affect the ZF flag according to the
> > results of the comparisons they make."
> >
>
> The point was to ensure we have a valid result in ZF (which should be ZF=1)
> even if len = 0.

I had originally thought that the test for RCX==0 was also at the end
of the loop, after the decrement.  But a closer look at the
pseudo-code in the SDM does show that the test is at the start of the
loop.  Starting with RCX==0 does indeed skip the comparison and does
not touch ZF.  Your suggestion is good, but should have a comment
explaining why it's there to someone reading the code in the future.