Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1220729 > unrolled thread
| Started by | Xishi Qiu <qiuxishi@huawei.com> |
|---|---|
| First post | 2015-09-08 14:20 +0200 |
| Last post | 2015-09-09 12:10 +0200 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() Xishi Qiu <qiuxishi@huawei.com> - 2015-09-08 14:20 +0200
Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() "long.wanglong" <long.wanglong@huawei.com> - 2015-09-09 08:50 +0200
Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() Xishi Qiu <qiuxishi@huawei.com> - 2015-09-09 09:50 +0200
Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() "long.wanglong" <long.wanglong@huawei.com> - 2015-09-09 10:20 +0200
Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-09-09 10:40 +0200
Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-09-09 12:10 +0200
| From | Xishi Qiu <qiuxishi@huawei.com> |
|---|---|
| Date | 2015-09-08 14:20 +0200 |
| Subject | [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() |
| Message-ID | <q6q2t-3WT-7@gated-at.bofh.it> |
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | "long.wanglong" <long.wanglong@huawei.com> |
|---|---|
| Date | 2015-09-09 08:50 +0200 |
| Message-ID | <q6HmF-3Ay-9@gated-at.bofh.it> |
| In reply to | #1220729 |
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Xishi Qiu <qiuxishi@huawei.com> |
|---|---|
| Date | 2015-09-09 09:50 +0200 |
| Message-ID | <q6IiK-529-17@gated-at.bofh.it> |
| In reply to | #1221276 |
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | "long.wanglong" <long.wanglong@huawei.com> |
|---|---|
| Date | 2015-09-09 10:20 +0200 |
| Message-ID | <q6ILL-5Pm-1@gated-at.bofh.it> |
| In reply to | #1221286 |
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andrey Ryabinin <ryabinin.a.a@gmail.com> |
|---|---|
| Date | 2015-09-09 10:40 +0200 |
| Message-ID | <q6J58-6bW-7@gated-at.bofh.it> |
| In reply to | #1221276 |
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andrey Ryabinin <aryabinin@virtuozzo.com> |
|---|---|
| Date | 2015-09-09 12:10 +0200 |
| Subject | Re: [PATCH V2] kasan: fix last shadow judgement in memory_is_poisoned_16() |
| Message-ID | <q6Kue-8mm-27@gated-at.bofh.it> |
| In reply to | #1220729 |
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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web