diff mbox series

[v2] arm64/mm: fix variable 'tag' set but not used

Message ID 1564670825-4050-1-git-send-email-cai@lca.pw (mailing list archive)
State Mainlined
Commit 7732d20a160c76006c7fe7bca5178aea6af1d2e8
Headers show
Series [v2] arm64/mm: fix variable 'tag' set but not used | expand

Commit Message

Qian Cai Aug. 1, 2019, 2:47 p.m. UTC
When CONFIG_KASAN_SW_TAGS=n, set_tag() is compiled away. GCC throws a
warning,

mm/kasan/common.c: In function '__kasan_kmalloc':
mm/kasan/common.c:464:5: warning: variable 'tag' set but not used
[-Wunused-but-set-variable]
  u8 tag = 0xff;
     ^~~

Fix it by making __tag_set() a static inline function the same as
arch_kasan_set_tag() in mm/kasan/kasan.h for consistency because there
is a macro in arch/arm64/include/asm/kasan.h,

 #define arch_kasan_set_tag(addr, tag) __tag_set(addr, tag)

However, when CONFIG_DEBUG_VIRTUAL=n and CONFIG_SPARSEMEM_VMEMMAP=y,
page_to_virt() will call __tag_set() with incorrect type of a
parameter, so fix that as well. Also, still let page_to_virt() return
"void *" instead of "const void *", so will not need to add a similar
cast in lowmem_page_address().

Signed-off-by: Qian Cai <cai@lca.pw>
---

v2: Fix compilation warnings of CONFIG_DEBUG_VIRTUAL=n spotted by Will.

 arch/arm64/include/asm/memory.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox Aug. 1, 2019, 4 p.m. UTC | #1
On Thu, Aug 01, 2019 at 10:47:05AM -0400, Qian Cai wrote:

Given this:

> -#define __tag_set(addr, tag)	(addr)
> +static inline const void *__tag_set(const void *addr, u8 tag)
> +{
> +	return addr;
> +}
> +
>  #define __tag_reset(addr)	(addr)
>  #define __tag_get(addr)		0
>  #endif
> @@ -301,8 +305,8 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #define page_to_virt(page)	({					\
>  	unsigned long __addr =						\
>  		((__page_to_voff(page)) | PAGE_OFFSET);			\
> -	unsigned long __addr_tag =					\
> -		 __tag_set(__addr, page_kasan_tag(page));		\
> +	const void *__addr_tag =					\
> +		__tag_set((void *)__addr, page_kasan_tag(page));	\
>  	((void *)__addr_tag);						\
>  })

Can't you simplify that macro to:

 #define page_to_virt(page)	({					\
 	unsigned long __addr =						\
 		((__page_to_voff(page)) | PAGE_OFFSET);			\
-	unsigned long __addr_tag =					\
-		 __tag_set(__addr, page_kasan_tag(page));		\
-	((void *)__addr_tag);						\
+	__tag_set((void *)__addr, page_kasan_tag(page));		\
 })
Qian Cai Aug. 1, 2019, 4:20 p.m. UTC | #2
On Thu, 2019-08-01 at 09:00 -0700, Matthew Wilcox wrote:
> On Thu, Aug 01, 2019 at 10:47:05AM -0400, Qian Cai wrote:
> 
> Given this:
> 
> > -#define __tag_set(addr, tag)	(addr)
> > +static inline const void *__tag_set(const void *addr, u8 tag)
> > +{
> > +	return addr;
> > +}
> > +
> >  #define __tag_reset(addr)	(addr)
> >  #define __tag_get(addr)		0
> >  #endif
> > @@ -301,8 +305,8 @@ static inline void *phys_to_virt(phys_addr_t x)
> >  #define page_to_virt(page)	({					
> > \
> >  	unsigned long __addr =						
> > \
> >  		((__page_to_voff(page)) | PAGE_OFFSET);			
> > \
> > -	unsigned long __addr_tag =					\
> > -		 __tag_set(__addr, page_kasan_tag(page));		\
> > +	const void *__addr_tag =					\
> > +		__tag_set((void *)__addr, page_kasan_tag(page));	\
> >  	((void *)__addr_tag);						
> > \
> >  })
> 
> Can't you simplify that macro to:
> 
>  #define page_to_virt(page)	({					\
>  	unsigned long __addr =						
> \
>  		((__page_to_voff(page)) | PAGE_OFFSET);			
> \
> -	unsigned long __addr_tag =					\
> -		 __tag_set(__addr, page_kasan_tag(page));		\
> -	((void *)__addr_tag);						
> \
> +	__tag_set((void *)__addr, page_kasan_tag(page));		\
>  })

It still need a cast or lowmem_page_address() will complain of a discarded
"const". It might be a bit harder to read when adding a cast as in,

((void *)__tag_set((void *)__addr, page_kasan_tag(page)));

But, that feel like more of a followup patch for me if ever needed.
Andrey Konovalov Aug. 5, 2019, 3:37 p.m. UTC | #3
On Thu, Aug 1, 2019 at 4:47 PM Qian Cai <cai@lca.pw> wrote:
>
> When CONFIG_KASAN_SW_TAGS=n, set_tag() is compiled away. GCC throws a
> warning,
>
> mm/kasan/common.c: In function '__kasan_kmalloc':
> mm/kasan/common.c:464:5: warning: variable 'tag' set but not used
> [-Wunused-but-set-variable]
>   u8 tag = 0xff;
>      ^~~
>
> Fix it by making __tag_set() a static inline function the same as
> arch_kasan_set_tag() in mm/kasan/kasan.h for consistency because there
> is a macro in arch/arm64/include/asm/kasan.h,
>
>  #define arch_kasan_set_tag(addr, tag) __tag_set(addr, tag)
>
> However, when CONFIG_DEBUG_VIRTUAL=n and CONFIG_SPARSEMEM_VMEMMAP=y,
> page_to_virt() will call __tag_set() with incorrect type of a
> parameter, so fix that as well. Also, still let page_to_virt() return
> "void *" instead of "const void *", so will not need to add a similar
> cast in lowmem_page_address().
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>
> v2: Fix compilation warnings of CONFIG_DEBUG_VIRTUAL=n spotted by Will.
>
>  arch/arm64/include/asm/memory.h | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index b7ba75809751..fb04f10a78ab 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -210,7 +210,11 @@ static inline unsigned long kaslr_offset(void)
>  #define __tag_reset(addr)      untagged_addr(addr)
>  #define __tag_get(addr)                (__u8)((u64)(addr) >> 56)
>  #else
> -#define __tag_set(addr, tag)   (addr)
> +static inline const void *__tag_set(const void *addr, u8 tag)
> +{
> +       return addr;
> +}
> +
>  #define __tag_reset(addr)      (addr)
>  #define __tag_get(addr)                0
>  #endif
> @@ -301,8 +305,8 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #define page_to_virt(page)     ({                                      \
>         unsigned long __addr =                                          \
>                 ((__page_to_voff(page)) | PAGE_OFFSET);                 \
> -       unsigned long __addr_tag =                                      \
> -                __tag_set(__addr, page_kasan_tag(page));               \
> +       const void *__addr_tag =                                        \
> +               __tag_set((void *)__addr, page_kasan_tag(page));        \
>         ((void *)__addr_tag);                                           \
>  })
>
> --
> 1.8.3.1
>

Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index b7ba75809751..fb04f10a78ab 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -210,7 +210,11 @@  static inline unsigned long kaslr_offset(void)
 #define __tag_reset(addr)	untagged_addr(addr)
 #define __tag_get(addr)		(__u8)((u64)(addr) >> 56)
 #else
-#define __tag_set(addr, tag)	(addr)
+static inline const void *__tag_set(const void *addr, u8 tag)
+{
+	return addr;
+}
+
 #define __tag_reset(addr)	(addr)
 #define __tag_get(addr)		0
 #endif
@@ -301,8 +305,8 @@  static inline void *phys_to_virt(phys_addr_t x)
 #define page_to_virt(page)	({					\
 	unsigned long __addr =						\
 		((__page_to_voff(page)) | PAGE_OFFSET);			\
-	unsigned long __addr_tag =					\
-		 __tag_set(__addr, page_kasan_tag(page));		\
+	const void *__addr_tag =					\
+		__tag_set((void *)__addr, page_kasan_tag(page));	\
 	((void *)__addr_tag);						\
 })