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

[Mauricio Faria de Oliveira <mfo@igalia.com>]

On 2026-07-22 15:45, H. Peter Anvin wrote:
> On July 22, 2026 10:03:34 AM PDT, Borislav Petkov <bp@alien8.de> 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@igalia.com>
>>> ---
>>>  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.

This is elegant; done. Thanks!

-- 
Mauricio