* [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16()
@ 2015-09-08 12:12 Xishi Qiu
2015-09-09 6:40 ` long.wanglong
2015-09-09 10:01 ` Andrey Ryabinin
0 siblings, 2 replies; 6+ messages in thread
From: Xishi Qiu @ 2015-09-08 12:12 UTC (permalink / raw)
To: Andrew Morton, Andrey Konovalov, Rusty Russell, Michal Marek,
zhongjiang
Cc: Linux MM, LKML
The shadow which correspond 16 bytes memory may span 2 or 3 bytes. If the
memory is aligned on 8, then the shadow takes only 2 bytes. So we check
"shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);".
But the code "if (likely(!last_byte))" is wrong judgement.
e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will
continue to call "memory_is_poisoned_1(addr + 15);"
Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
---
mm/kasan/kasan.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 7b28e9c..8da2114 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
if (unlikely(*shadow_addr)) {
u16 shadow_first_bytes = *(u16 *)shadow_addr;
- s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
if (unlikely(shadow_first_bytes))
return true;
- if (likely(!last_byte))
+ if (likely(IS_ALIGNED(addr, 8)))
return false;
return memory_is_poisoned_1(addr + 15);
--
1.7.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16()
2015-09-08 12:12 [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() Xishi Qiu
@ 2015-09-09 6:40 ` long.wanglong
2015-09-09 7:43 ` Xishi Qiu
2015-09-09 8:30 ` Andrey Ryabinin
2015-09-09 10:01 ` Andrey Ryabinin
1 sibling, 2 replies; 6+ messages in thread
From: long.wanglong @ 2015-09-09 6:40 UTC (permalink / raw)
To: Xishi Qiu, ryabinin.a.a
Cc: Andrew Morton, Andrey Konovalov, Rusty Russell, Michal Marek,
zhongjiang, Linux MM, LKML, Wang Long
On 2015/9/8 20:12, Xishi Qiu wrote:
> The shadow which correspond 16 bytes memory may span 2 or 3 bytes. If the
> memory is aligned on 8, then the shadow takes only 2 bytes. So we check
> "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);".
> But the code "if (likely(!last_byte))" is wrong judgement.
>
> e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will
> continue to call "memory_is_poisoned_1(addr + 15);"
>
> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
> ---
> mm/kasan/kasan.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 7b28e9c..8da2114 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>
> if (unlikely(*shadow_addr)) {
> u16 shadow_first_bytes = *(u16 *)shadow_addr;
> - s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
>
> if (unlikely(shadow_first_bytes))
> return true;
>
> - if (likely(!last_byte))
> + if (likely(IS_ALIGNED(addr, 8)))
> return false;
>
> return memory_is_poisoned_1(addr + 15);
>
Hi,
I also notice this problem, how about another method to fix it:
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 5d65d06..6a20dda 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -140,7 +140,7 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
if (unlikely(shadow_first_bytes))
return true;
- if (likely(!last_byte))
+ if (likely(last_byte >= 7))
return false;
return memory_is_poisoned_1(addr + 15);
This method can ensure consistency of code, for example, in memory_is_poisoned_8:
static __always_inline bool memory_is_poisoned_8(unsigned long addr)
{
u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
if (unlikely(*shadow_addr)) {
if (memory_is_poisoned_1(addr + 7))
return true;
if (likely(((addr + 7) & KASAN_SHADOW_MASK) >= 7))
return false;
return unlikely(*(u8 *)shadow_addr);
}
return false;
}
Otherwise, we also should use IS_ALIGNED macro in memory_is_poisoned_8!
Best Regards
Wang Long
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16()
2015-09-09 6:40 ` long.wanglong
@ 2015-09-09 7:43 ` Xishi Qiu
2015-09-09 8:10 ` long.wanglong
2015-09-09 8:30 ` Andrey Ryabinin
1 sibling, 1 reply; 6+ messages in thread
From: Xishi Qiu @ 2015-09-09 7:43 UTC (permalink / raw)
To: long.wanglong
Cc: ryabinin.a.a, Andrew Morton, Andrey Konovalov, Rusty Russell,
Michal Marek, zhongjiang, Linux MM, LKML
On 2015/9/9 14:40, long.wanglong wrote:
> On 2015/9/8 20:12, Xishi Qiu wrote:
>> The shadow which correspond 16 bytes memory may span 2 or 3 bytes. If the
>> memory is aligned on 8, then the shadow takes only 2 bytes. So we check
>> "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);".
>> But the code "if (likely(!last_byte))" is wrong judgement.
>>
>> e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will
>> continue to call "memory_is_poisoned_1(addr + 15);"
>>
>> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
>> ---
>> mm/kasan/kasan.c | 3 +--
>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 7b28e9c..8da2114 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>
>> if (unlikely(*shadow_addr)) {
>> u16 shadow_first_bytes = *(u16 *)shadow_addr;
>> - s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
>>
>> if (unlikely(shadow_first_bytes))
>> return true;
>>
>> - if (likely(!last_byte))
>> + if (likely(IS_ALIGNED(addr, 8)))
>> return false;
>>
>> return memory_is_poisoned_1(addr + 15);
>>
>
> Hi,
> I also notice this problem, how about another method to fix it:
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 5d65d06..6a20dda 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -140,7 +140,7 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
> if (unlikely(shadow_first_bytes))
> return true;
>
> - if (likely(!last_byte))
> + if (likely(last_byte >= 7))
> return false;
>
> return memory_is_poisoned_1(addr + 15);
>
> This method can ensure consistency of code, for example, in memory_is_poisoned_8:
>
> static __always_inline bool memory_is_poisoned_8(unsigned long addr)
> {
> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>
> if (unlikely(*shadow_addr)) {
> if (memory_is_poisoned_1(addr + 7))
> return true;
>
> if (likely(((addr + 7) & KASAN_SHADOW_MASK) >= 7))
> return false;
>
> return unlikely(*(u8 *)shadow_addr);
> }
>
> return false;
> }
>
> Otherwise, we also should use IS_ALIGNED macro in memory_is_poisoned_8!
>
Hi Wanglong,
How about use IS_ALIGNED instead of those code in memory_is_poisoned_8()
and other functions? I think the current code looks a bit ugly.
Thanks,
Xishi Qiu
>
> Best Regards
> Wang Long
>
>
>
>
>
> .
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16()
2015-09-09 7:43 ` Xishi Qiu
@ 2015-09-09 8:10 ` long.wanglong
0 siblings, 0 replies; 6+ messages in thread
From: long.wanglong @ 2015-09-09 8:10 UTC (permalink / raw)
To: Xishi Qiu
Cc: ryabinin.a.a, Andrew Morton, Andrey Konovalov, Rusty Russell,
Michal Marek, zhongjiang, Linux MM, LKML
On 2015/9/9 15:43, Xishi Qiu wrote:
> On 2015/9/9 14:40, long.wanglong wrote:
>
>> On 2015/9/8 20:12, Xishi Qiu wrote:
>>> The shadow which correspond 16 bytes memory may span 2 or 3 bytes. If the
>>> memory is aligned on 8, then the shadow takes only 2 bytes. So we check
>>> "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);".
>>> But the code "if (likely(!last_byte))" is wrong judgement.
>>>
>>> e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will
>>> continue to call "memory_is_poisoned_1(addr + 15);"
>>>
>>> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
>>> ---
>>> mm/kasan/kasan.c | 3 +--
>>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>>> index 7b28e9c..8da2114 100644
>>> --- a/mm/kasan/kasan.c
>>> +++ b/mm/kasan/kasan.c
>>> @@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>>
>>> if (unlikely(*shadow_addr)) {
>>> u16 shadow_first_bytes = *(u16 *)shadow_addr;
>>> - s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
>>>
>>> if (unlikely(shadow_first_bytes))
>>> return true;
>>>
>>> - if (likely(!last_byte))
>>> + if (likely(IS_ALIGNED(addr, 8)))
>>> return false;
>>>
>>> return memory_is_poisoned_1(addr + 15);
>>>
>>
>> Hi,
>> I also notice this problem, how about another method to fix it:
>>
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 5d65d06..6a20dda 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -140,7 +140,7 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> if (unlikely(shadow_first_bytes))
>> return true;
>>
>> - if (likely(!last_byte))
>> + if (likely(last_byte >= 7))
>> return false;
>>
>> return memory_is_poisoned_1(addr + 15);
>>
>> This method can ensure consistency of code, for example, in memory_is_poisoned_8:
>>
>> static __always_inline bool memory_is_poisoned_8(unsigned long addr)
>> {
>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>
>> if (unlikely(*shadow_addr)) {
>> if (memory_is_poisoned_1(addr + 7))
>> return true;
>>
>> if (likely(((addr + 7) & KASAN_SHADOW_MASK) >= 7))
>> return false;
>>
>> return unlikely(*(u8 *)shadow_addr);
>> }
>>
>> return false;
>> }
>>
>> Otherwise, we also should use IS_ALIGNED macro in memory_is_poisoned_8!
>>
>
> Hi Wanglong,
>
> How about use IS_ALIGNED instead of those code in memory_is_poisoned_8()
> and other functions? I think the current code looks a bit ugly.
>
I think we can use IS_ALIGNED macro in memory_is_poisoned_8 and memory_is_poisoned_16.
but for functions memory_is_poisoned_2 and memory_is_poisoned_4, we can't.
Wang Long
> Thanks,
> Xishi Qiu
>
>>
>> Best Regards
>> Wang Long
>>
>>
>>
>>
>>
>> .
>>
>
>
>
>
> .
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16()
2015-09-09 6:40 ` long.wanglong
2015-09-09 7:43 ` Xishi Qiu
@ 2015-09-09 8:30 ` Andrey Ryabinin
1 sibling, 0 replies; 6+ messages in thread
From: Andrey Ryabinin @ 2015-09-09 8:30 UTC (permalink / raw)
To: long.wanglong
Cc: Xishi Qiu, Andrew Morton, Andrey Konovalov, Rusty Russell,
Michal Marek, zhongjiang, Linux MM, LKML
2015-09-09 9:40 GMT+03:00 long.wanglong <long.wanglong@huawei.com>:
> On 2015/9/8 20:12, Xishi Qiu wrote:
>> The shadow which correspond 16 bytes memory may span 2 or 3 bytes. If the
>> memory is aligned on 8, then the shadow takes only 2 bytes. So we check
>> "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);".
>> But the code "if (likely(!last_byte))" is wrong judgement.
>>
>> e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will
>> continue to call "memory_is_poisoned_1(addr + 15);"
>>
>> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
>> ---
>> mm/kasan/kasan.c | 3 +--
>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 7b28e9c..8da2114 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>
>> if (unlikely(*shadow_addr)) {
>> u16 shadow_first_bytes = *(u16 *)shadow_addr;
>> - s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
>>
>> if (unlikely(shadow_first_bytes))
>> return true;
>>
>> - if (likely(!last_byte))
>> + if (likely(IS_ALIGNED(addr, 8)))
>> return false;
>>
>> return memory_is_poisoned_1(addr + 15);
>>
>
> Hi,
> I also notice this problem, how about another method to fix it:
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 5d65d06..6a20dda 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -140,7 +140,7 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
> if (unlikely(shadow_first_bytes))
> return true;
>
> - if (likely(!last_byte))
> + if (likely(last_byte >= 7))
I suggested to use IS_ALIGNED instead of this because it generates
less code and it also more readable.
./scripts/bloat-o-meter kasan_aligned.o kasan_last_byte.o
add/remove: 0/0 grow/shrink: 4/0 up/down: 20/0 (20)
function old new delta
__asan_store16_noabort 106 111 +5
__asan_store16 106 111 +5
__asan_load16_noabort 103 108 +5
__asan_load16 103 108 +5
> }
>
> return false;
> }
>
> Otherwise, we also should use IS_ALIGNED macro in memory_is_poisoned_8!
>
I believe this would be a beneficial micro optimization.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16()
2015-09-08 12:12 [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() Xishi Qiu
2015-09-09 6:40 ` long.wanglong
@ 2015-09-09 10:01 ` Andrey Ryabinin
1 sibling, 0 replies; 6+ messages in thread
From: Andrey Ryabinin @ 2015-09-09 10:01 UTC (permalink / raw)
To: Xishi Qiu, Andrew Morton, Andrey Konovalov, Rusty Russell,
Michal Marek, zhongjiang
Cc: Linux MM, LKML, ryabinin.a.a
On 09/08/2015 03:12 PM, Xishi Qiu wrote:
> The shadow which correspond 16 bytes memory may span 2 or 3 bytes. If the
> memory is aligned on 8, then the shadow takes only 2 bytes. So we check
> "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);".
> But the code "if (likely(!last_byte))" is wrong judgement.
>
> e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will
> continue to call "memory_is_poisoned_1(addr + 15);"
>
> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
Acked-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-09 10:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-08 12:12 [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() Xishi Qiu
2015-09-09 6:40 ` long.wanglong
2015-09-09 7:43 ` Xishi Qiu
2015-09-09 8:10 ` long.wanglong
2015-09-09 8:30 ` Andrey Ryabinin
2015-09-09 10:01 ` Andrey Ryabinin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).