| Message ID | 1564670825-4050-1-git-send-email-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64/mm: fix variable 'tag' set but not used | expand |
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)); \ })
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.
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 --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); \ })
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(-)