[PATCH] arm: memset: zero out upper bytes in r1

[PATCH] arm: memset: zero out upper bytes in r1

[Andrey Ryabinin]

memset doesn't work right for following example:

	signed char c = 0xF0;
	memset(addr, c, size);

Variable c is signed, so after typcasting to int the value will be 0xFFFFFFF0.
This value will be passed through r1 regitster to memset function.
memset doesn't zero out upper bytes in r1, so memory will be filled
with 0xFFFFFFF0 instead of expected 0xF0F0F0F0.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 arch/arm/lib/memset.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
index 94b0650..a010f76 100644
--- a/arch/arm/lib/memset.S
+++ b/arch/arm/lib/memset.S
@@ -22,7 +22,8 @@ ENTRY(memset)
 /*
  * we know that the pointer in ip is aligned to a word boundary.
  */
-1:	orr	r1, r1, r1, lsl #8
+1:	and	r1, r1, #0xff
+	orr	r1, r1, r1, lsl #8
 	orr	r1, r1, r1, lsl #16
 	mov	r3, r1
 	cmp	r2, #16
-- 
1.8.3.2

[PATCH] arm: memset: zero out upper bytes in r1

[Afzal Mohammed]

Hi Andrey,

On Mon, May 05, 2014 at 11:11:13AM +0400, Andrey Ryabinin wrote:

> memset doesn't work right for following example:
> 
> 	signed char c = 0xF0;
> 	memset(addr, c, size);
> 
> Variable c is signed, so after typcasting to int the value will be 0xFFFFFFF0.
> This value will be passed through r1 regitster to memset function.
> memset doesn't zero out upper bytes in r1, so memory will be filled
> with 0xFFFFFFF0 instead of expected 0xF0F0F0F0.

> --- a/arch/arm/lib/memset.S
> +++ b/arch/arm/lib/memset.S
> @@ -22,7 +22,8 @@ ENTRY(memset)
>  /*
>   * we know that the pointer in ip is aligned to a word boundary.
>   */
> -1:	orr	r1, r1, r1, lsl #8
> +1:	and	r1, r1, #0xff
> +	orr	r1, r1, r1, lsl #8

int is to be converted to unsigned char in memset, would having above
change immediately upon entry to memset rather than at a place where it
won't always execute make intention clearer ? (although it doesn't make
difference)

ubfx r1, r1, #0, #8 would have given the needed typecasting, but seems
it is available only on ARMv6T2 & above.

Regards
Afzal

[PATCH] arm: memset: zero out upper bytes in r1

[Andrey Ryabinin]

On 05/08/14 03:42, Afzal Mohammed wrote:
> 
> int is to be converted to unsigned char in memset, would having above
> change immediately upon entry to memset rather than at a place where it
> won't always execute make intention clearer ? (although it doesn't make
> difference)
> 

I think it's better to keep it near other manipulations with r1.
Plus this will save us from executing extra instruction on 'memset -> 6 -> 5 -> return'
path (memset for size <= 3).


> ubfx r1, r1, #0, #8 would have given the needed typecasting, but seems
> it is available only on ARMv6T2 & above.
> 

Indeed. It might be wrapped it with #if #else, but it will be a bit ugly,
probably not worth to do.

[PATCH] arm: memset: zero out upper bytes in r1

[Andrey Ryabinin]

On 05/08/14 11:59, Andrey Ryabinin wrote:
> On 05/08/14 03:42, Afzal Mohammed wrote:
>>
>> int is to be converted to unsigned char in memset, would having above
>> change immediately upon entry to memset rather than at a place where it
>> won't always execute make intention clearer ? (although it doesn't make
>> difference)
>>
> 
> I think it's better to keep it near other manipulations with r1.
> Plus this will save us from executing extra instruction on 'memset -> 6 -> 5 -> return'
> path (memset for size <= 3).
> 
> 
>> ubfx r1, r1, #0, #8 would have given the needed typecasting, but seems
>> it is available only on ARMv6T2 & above.
>>
> 
> Indeed. It might be wrapped it with #if #else, but it will be a bit ugly,
> probably not worth to do.
> 
> 

Err, there is no reason to use ubfx, it just the same as and r1,r1, #0xff.
On ARMv6T2 & above one instruction could be saved by using BFI instead of ORR, like this:

diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
index 94b0650..9a7f714 100644
--- a/arch/arm/lib/memset.S
+++ b/arch/arm/lib/memset.S
@@ -22,8 +22,8 @@ ENTRY(memset)
 /*
  * we know that the pointer in ip is aligned to a word boundary.
  */
-1:     orr     r1, r1, r1, lsl #8
-       orr     r1, r1, r1, lsl #16
+1:     bfi     r1, r1, #8, #8
+       bfi     r1, r1, #16, #16
        mov     r3, r1
        cmp     r2, #16
        blt     4f

[PATCH] arm: memset: zero out upper bytes in r1

[Andrey Ryabinin]

On 05/08/14 12:38, Vladimir Murzin wrote:
> Vladimir Murzin <murzin.v <at> gmail.com> writes:
> 
>>
>> Andrey Ryabinin <a.ryabinin <at> samsung.com> writes:
>>
>>>
>>> memset doesn't work right for following example:
>>>
>>> 	signed char c = 0xF0;
>>> 	memset(addr, c, size);
>>>
>>> Variable c is signed, so after typcasting to int the value will be
> 0xFFFFFFF0.
>>> This value will be passed through r1 regitster to memset function.
>>> memset doesn't zero out upper bytes in r1, so memory will be filled
>>> with 0xFFFFFFF0 instead of expected 0xF0F0F0F0.
>>
>> It behaves the same as a generic implementation in lib/string.c, moreover
>> gcc built-in behaves the same. So it looks like expected behavior and POSIX
>> Programmer's Manual (man 3posix memset) explicitly says that "c" is
>> converted to unsigned char.
>>
>> Cheers
>> Vladimir
>>
>>
> 

Hi Vladimir.
Please, use reply to all next time, otherwise I might miss your mails.

> Sorry, had a bad coffee when posted it ;) It behaves /different/, but it is
> here for many years... 

Your are right, it's here for many years and wasn't noticed, probably because, there are no
such users, that call memset with not zero upper bytes in second argument. Though, I can't tell this
for sure, there are might be a few of such calls.
So this fix probably doesn't change anything for current kernel, however it might help to avoid problems in future.
Also, I think would be good to have the same behavior on different architectures (arm64/x86 works correctly
for example above).

> doesn't this change break something? iow, it possible
> that some user rely on this behavior...
> 

I don't think that such user exist, cause such user must know implementation details of memset
on ARM and rely on them.
If such user exist that's insane user and should be fixed.


> Vladimir
>