Skip to contentSkip to conversation

kasan: fix last shadow judgement in memory_is_poisoned_16()

PATCH In review Xishi Qiu 4 participants 6 messages Sep 8, 2015 – Sep 9, 2015

Subsystems (2)
Memory management KASAN
lore
Share & tools
lore

Xishi Qiu

author
#0 lore raw

PATCH v2 1/1 kasan: fix last shadow judgement in memory_is_poisoned_16()

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 <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +1 -2
index 7b28e9c..8da2114 100644
@@ -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);
Signature · 1 line
1.7.1
#1 lore raw
Quoted 22 lines · Xishi Qiu

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 <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +0 -0
index 7b28e9c..8da2114 100644
@@ -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;

mm/kasan/kasan.c static __always_inline bool memory_is_poisoned_16(unsigned long addr) · line 145

Quoted 10 lines
mm/kasan/kasan.c +1 -2
-		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:

mm/kasan/kasan.c +1 -1
index 5d65d06..6a20dda 100644
@@ -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

author
#2 lore raw
Quoted 70 lines · long.wanglong
Quoted 22 lines · Xishi Qiu

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 <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +0 -0
index 7b28e9c..8da2114 100644
@@ -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;

mm/kasan/kasan.c static __always_inline bool memory_is_poisoned_16(unsigned long addr) · line 145

Quoted 10 lines
mm/kasan/kasan.c +1 -2
-		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:

mm/kasan/kasan.c +1 -1
index 5d65d06..6a20dda 100644
@@ -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;
}
Quoted 2 lines

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

Quoted 10 lines

Best Regards
Wang Long

.

#3 lore raw
Quoted 72 lines · Xishi Qiu
Quoted 70 lines · long.wanglong
Quoted 22 lines · Xishi Qiu

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 <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +0 -0
index 7b28e9c..8da2114 100644
@@ -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;

mm/kasan/kasan.c static __always_inline bool memory_is_poisoned_16(unsigned long addr) · line 145

Quoted 10 lines
mm/kasan/kasan.c +1 -2
-		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:

mm/kasan/kasan.c +1 -1
index 5d65d06..6a20dda 100644
@@ -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;
}
Quoted 8 lines
Quoted 2 lines

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

Quoted 19 lines

Thanks,
Xishi Qiu

Quoted 10 lines

Best Regards
Wang Long

.

.

maintainer
#4 lore raw
In reply to long.wanglong

2015-09-09 9:40 GMT+03:00 long.wanglong <[email protected]>:

Quoted 42 lines · Xishi Qiu
Quoted 22 lines

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 <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +0 -0
index 7b28e9c..8da2114 100644
@@ -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;

mm/kasan/kasan.c static __always_inline bool memory_is_poisoned_16(unsigned long addr) · line 145

Quoted 10 lines
mm/kasan/kasan.c +1 -2
-             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:

mm/kasan/kasan.c +0 -0
index 5d65d06..6a20dda 100644

mm/kasan/kasan.c static __always_inline bool memory_is_poisoned_16(unsigned long addr) · line 143

Quoted 5 lines
mm/kasan/kasan.c +1 -1
@@ -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
Quoted 7 lines
mm/kasan/kasan.c +0 -0
        }
 
        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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Acked-by on v2 1/1
#5 lore raw
In reply to Xishi Qiu
Quoted 9 lines · Xishi Qiu

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 <[email protected]>
Acked-by Andrey Ryabinin <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>