| Message ID | 20210303121157.3430807-1-elver@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [mm] kfence: fix printk format for ptrdiff_t | expand |
On Wed, Mar 3, 2021 at 1:12 PM Marco Elver <elver@google.com> wrote: > > Use %td for ptrdiff_t. > > Link: https://lkml.kernel.org/r/3abbe4c9-16ad-c168-a90f-087978ccd8f7@csgroup.eu > Reported-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Signed-off-by: Marco Elver <elver@google.com> Reviewed-by: Alexander Potapenko <glider@google.com>
+segher Le 03/03/2021 à 13:27, Alexander Potapenko a écrit : > On Wed, Mar 3, 2021 at 1:12 PM Marco Elver <elver@google.com> wrote: >> >> Use %td for ptrdiff_t. >> >> Link: https://lkml.kernel.org/r/3abbe4c9-16ad-c168-a90f-087978ccd8f7@csgroup.eu >> Reported-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> Signed-off-by: Marco Elver <elver@google.com> > Reviewed-by: Alexander Potapenko <glider@google.com> > Still a problem. I don't understand, gcc bug ? The offending argument is 'const ptrdiff_t object_index' We have: arch/powerpc/include/uapi/asm/posix_types.h:typedef long __kernel_ptrdiff_t; include/linux/types.h:typedef __kernel_ptrdiff_t ptrdiff_t; And get: CC mm/kfence/report.o In file included from ./include/linux/printk.h:7, from ./include/linux/kernel.h:16, from mm/kfence/report.c:10: mm/kfence/report.c: In function 'kfence_report_error': ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument of type 'ptrdiff_t', but argument 6 has type 'long int' [-Wformat=] 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ | ^~~~~~ ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH' 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */ | ^~~~~~~~ ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR' 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~ mm/kfence/report.c:213:3: note: in expansion of macro 'pr_err' 213 | pr_err("Out-of-bounds %s at 0x%p (%luB %s of kfence-#%td):\n", | ^~~~~~ ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument of type 'ptrdiff_t', but argument 4 has type 'long int' [-Wformat=] 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ | ^~~~~~ ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH' 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */ | ^~~~~~~~ ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR' 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~ mm/kfence/report.c:222:3: note: in expansion of macro 'pr_err' 222 | pr_err("Use-after-free %s at 0x%p (in kfence-#%td):\n", | ^~~~~~ ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument of type 'ptrdiff_t', but argument 2 has type 'long int' [-Wformat=] 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ | ^~~~~~ ./include/linux/kern_levels.h:24:19: note: in expansion of macro 'KERN_SOH' 24 | #define KERN_CONT KERN_SOH "c" | ^~~~~~~~ ./include/linux/printk.h:385:9: note: in expansion of macro 'KERN_CONT' 385 | printk(KERN_CONT fmt, ##__VA_ARGS__) | ^~~~~~~~~ mm/kfence/report.c:229:3: note: in expansion of macro 'pr_cont' 229 | pr_cont(" (in kfence-#%td):\n", object_index); | ^~~~~~~ ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument of type 'ptrdiff_t', but argument 3 has type 'long int' [-Wformat=] 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ | ^~~~~~ ./include/linux/kern_levels.h:11:18: note: in expansion of macro 'KERN_SOH' 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */ | ^~~~~~~~ ./include/linux/printk.h:343:9: note: in expansion of macro 'KERN_ERR' 343 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~ mm/kfence/report.c:239:3: note: in expansion of macro 'pr_err' 239 | pr_err("Invalid free of 0x%p (in kfence-#%td):\n", (void *)address, | ^~~~~~ Christophe
On Tue, Mar 16, 2021 at 09:32:32AM +0100, Christophe Leroy wrote: > +segher I cannot see through the wood of #defines here, sorry. > Still a problem. > > I don't understand, gcc bug ? Rule #1: If you do not understand what is happening, it is not a compiler bug. I'm not saying that it isn't, just that it is much more likely something else. > The offending argument is 'const ptrdiff_t object_index' > > We have: > > arch/powerpc/include/uapi/asm/posix_types.h:typedef long > __kernel_ptrdiff_t; So this is a 64-bit build. > include/linux/types.h:typedef __kernel_ptrdiff_t ptrdiff_t; > > And get: > > CC mm/kfence/report.o > In file included from ./include/linux/printk.h:7, > from ./include/linux/kernel.h:16, > from mm/kfence/report.c:10: > mm/kfence/report.c: In function 'kfence_report_error': > ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument > of type 'ptrdiff_t', but argument 6 has type 'long int' [-Wformat=] This is declared as const ptrdiff_t object_index = meta ? meta - kfence_metadata : -1; so maybe something with that goes wrong? What happens if you delete the (useless) "const" here? Segher
Le 16/03/2021 à 16:33, Segher Boessenkool a écrit : > On Tue, Mar 16, 2021 at 09:32:32AM +0100, Christophe Leroy wrote: >> +segher > > I cannot see through the wood of #defines here, sorry. > >> Still a problem. >> >> I don't understand, gcc bug ? > > Rule #1: If you do not understand what is happening, it is not a > compiler bug. I'm not saying that it isn't, just that it is much more > likely something else. > >> The offending argument is 'const ptrdiff_t object_index' >> >> We have: >> >> arch/powerpc/include/uapi/asm/posix_types.h:typedef long >> __kernel_ptrdiff_t; > > So this is a 64-bit build. No it's 32 bits. The code in posix-types.h is #ifdef __powerpc64__ ... #else ... typedef long __kernel_ptrdiff_t; > >> include/linux/types.h:typedef __kernel_ptrdiff_t ptrdiff_t; >> >> And get: >> >> CC mm/kfence/report.o >> In file included from ./include/linux/printk.h:7, >> from ./include/linux/kernel.h:16, >> from mm/kfence/report.c:10: >> mm/kfence/report.c: In function 'kfence_report_error': >> ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument >> of type 'ptrdiff_t', but argument 6 has type 'long int' [-Wformat=] > > This is declared as > const ptrdiff_t object_index = meta ? meta - kfence_metadata : -1; > so maybe something with that goes wrong? What happens if you delete the > (useless) "const" here? No change. Christophe
From: Christophe Leroy > Sent: 16 March 2021 15:41 ... > >> include/linux/types.h:typedef __kernel_ptrdiff_t ptrdiff_t; > >> > >> And get: > >> > >> CC mm/kfence/report.o > >> In file included from ./include/linux/printk.h:7, > >> from ./include/linux/kernel.h:16, > >> from mm/kfence/report.c:10: > >> mm/kfence/report.c: In function 'kfence_report_error': > >> ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument > >> of type 'ptrdiff_t', but argument 6 has type 'long int' [-Wformat=] > > > > This is declared as > > const ptrdiff_t object_index = meta ? meta - kfence_metadata : -1; > > so maybe something with that goes wrong? What happens if you delete the > > (useless) "const" here? The obvious thing to try is changing it to 'int'. That will break 64bit builds, but if it fixes the 32bit one it will tell you what type gcc is expecting. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Le 17/03/2021 à 13:51, David Laight a écrit : > From: Christophe Leroy >> Sent: 16 March 2021 15:41 > ... >>>> include/linux/types.h:typedef __kernel_ptrdiff_t ptrdiff_t; >>>> >>>> And get: >>>> >>>> CC mm/kfence/report.o >>>> In file included from ./include/linux/printk.h:7, >>>> from ./include/linux/kernel.h:16, >>>> from mm/kfence/report.c:10: >>>> mm/kfence/report.c: In function 'kfence_report_error': >>>> ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument >>>> of type 'ptrdiff_t', but argument 6 has type 'long int' [-Wformat=] >>> >>> This is declared as >>> const ptrdiff_t object_index = meta ? meta - kfence_metadata : -1; >>> so maybe something with that goes wrong? What happens if you delete the >>> (useless) "const" here? > > The obvious thing to try is changing it to 'int'. > That will break 64bit builds, but if it fixes the 32bit one > it will tell you what type gcc is expecting. > Yes, if defining 'object_index' as int, gcc is happy. If removing the powerpc re-definition of ptrdiff_t typedef in https://elixir.bootlin.com/linux/v5.12-rc3/source/arch/powerpc/include/uapi/asm/posix_types.h , it works great as well. So seems like gcc doesn't take into account the typedef behind ptrdiff_t, it just expects it to be int on 32 bits ? Christophe
From: Christophe Leroy > Sent: 17 March 2021 17:35 > > Le 17/03/2021 à 13:51, David Laight a écrit : > > From: Christophe Leroy > >> Sent: 16 March 2021 15:41 > > ... > >>>> include/linux/types.h:typedef __kernel_ptrdiff_t ptrdiff_t; > >>>> > >>>> And get: > >>>> > >>>> CC mm/kfence/report.o > >>>> In file included from ./include/linux/printk.h:7, > >>>> from ./include/linux/kernel.h:16, > >>>> from mm/kfence/report.c:10: > >>>> mm/kfence/report.c: In function 'kfence_report_error': > >>>> ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument > >>>> of type 'ptrdiff_t', but argument 6 has type 'long int' [-Wformat=] > >>> > >>> This is declared as > >>> const ptrdiff_t object_index = meta ? meta - kfence_metadata : -1; > >>> so maybe something with that goes wrong? What happens if you delete the > >>> (useless) "const" here? > > > > The obvious thing to try is changing it to 'int'. > > That will break 64bit builds, but if it fixes the 32bit one > > it will tell you what type gcc is expecting. > > > > Yes, if defining 'object_index' as int, gcc is happy. > If removing the powerpc re-definition of ptrdiff_t typedef in > https://elixir.bootlin.com/linux/v5.12-rc3/source/arch/powerpc/include/uapi/asm/posix_types.h , it > works great as well. > > So seems like gcc doesn't take into account the typedef behind ptrdiff_t, it just expects it to be > int on 32 bits ? gcc never cares how ptrdiff_t (or any of the related types) is defined it requires int or long for the format depending on the architecture. The error message will say ptrdiff_t or size_t (etc) - but that is just in the error message. So the ppc32 uapi definition of __kernel_ptrdiff_t is wrong. However it is probably set in stone. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Le 18/03/2021 à 10:14, David Laight a écrit : > From: Christophe Leroy >> Sent: 17 March 2021 17:35 >> >> Le 17/03/2021 à 13:51, David Laight a écrit : >>> From: Christophe Leroy >>>> Sent: 16 March 2021 15:41 >>> ... >>>>>> include/linux/types.h:typedef __kernel_ptrdiff_t ptrdiff_t; >>>>>> >>>>>> And get: >>>>>> >>>>>> CC mm/kfence/report.o >>>>>> In file included from ./include/linux/printk.h:7, >>>>>> from ./include/linux/kernel.h:16, >>>>>> from mm/kfence/report.c:10: >>>>>> mm/kfence/report.c: In function 'kfence_report_error': >>>>>> ./include/linux/kern_levels.h:5:18: warning: format '%td' expects argument >>>>>> of type 'ptrdiff_t', but argument 6 has type 'long int' [-Wformat=] >>>>> >>>>> This is declared as >>>>> const ptrdiff_t object_index = meta ? meta - kfence_metadata : -1; >>>>> so maybe something with that goes wrong? What happens if you delete the >>>>> (useless) "const" here? >>> >>> The obvious thing to try is changing it to 'int'. >>> That will break 64bit builds, but if it fixes the 32bit one >>> it will tell you what type gcc is expecting. >>> >> >> Yes, if defining 'object_index' as int, gcc is happy. >> If removing the powerpc re-definition of ptrdiff_t typedef in >> https://elixir.bootlin.com/linux/v5.12-rc3/source/arch/powerpc/include/uapi/asm/posix_types.h , it >> works great as well. >> >> So seems like gcc doesn't take into account the typedef behind ptrdiff_t, it just expects it to be >> int on 32 bits ? > > gcc never cares how ptrdiff_t (or any of the related types) is defined > it requires int or long for the format depending on the architecture. > The error message will say ptrdiff_t or size_t (etc) - but that is just > in the error message. > > So the ppc32 uapi definition of __kernel_ptrdiff_t is wrong. > However it is probably set in stone. > Yes it seems to be wrong. It was changed by commit d27dfd3887 ("Import pre2.0.8"), so that's long time ago. Before that it was an 'int' for ppc32. gcc provides ptrdiff_t in stddef.h via __PTRDIFF_TYPE__ gcc defined __PTRDIFF_TYPE__ as 'int' at build time. Should we fix it in arch/powerpc/include/uapi/asm/posix_types.h ? Anyway 'long' and 'int' makes no functionnal difference on 32 bits so there should be no impact for users if any. Christophe
On Thu, Mar 18, 2021 at 10:38:43AM +0100, Christophe Leroy wrote: > Yes it seems to be wrong. It was changed by commit d27dfd3887 ("Import > pre2.0.8"), so that's long time ago. Before that it was an 'int' for ppc32. > > gcc provides ptrdiff_t in stddef.h via __PTRDIFF_TYPE__ > gcc defined __PTRDIFF_TYPE__ as 'int' at build time. (On 32-bit PowerPC Linux.) > Should we fix it in arch/powerpc/include/uapi/asm/posix_types.h ? I think so, yes. > Anyway > 'long' and 'int' makes no functionnal difference on 32 bits so there should > be no impact for users if any. Except that long and int are different types, which causes errors like what you have here. There may be similar fallout from changing it back. Segher
diff --git a/mm/kfence/report.c b/mm/kfence/report.c index ab83d5a59bb1..519f037720f5 100644 --- a/mm/kfence/report.c +++ b/mm/kfence/report.c @@ -116,12 +116,12 @@ void kfence_print_object(struct seq_file *seq, const struct kfence_metadata *met lockdep_assert_held(&meta->lock); if (meta->state == KFENCE_OBJECT_UNUSED) { - seq_con_printf(seq, "kfence-#%zd unused\n", meta - kfence_metadata); + seq_con_printf(seq, "kfence-#%td unused\n", meta - kfence_metadata); return; } seq_con_printf(seq, - "kfence-#%zd [0x%p-0x%p" + "kfence-#%td [0x%p-0x%p" ", size=%d, cache=%s] allocated by task %d:\n", meta - kfence_metadata, (void *)start, (void *)(start + size - 1), size, (cache && cache->name) ? cache->name : "<destroyed>", meta->alloc_track.pid); @@ -204,7 +204,7 @@ void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *r pr_err("BUG: KFENCE: out-of-bounds %s in %pS\n\n", get_access_type(is_write), (void *)stack_entries[skipnr]); - pr_err("Out-of-bounds %s at 0x%p (%luB %s of kfence-#%zd):\n", + pr_err("Out-of-bounds %s at 0x%p (%luB %s of kfence-#%td):\n", get_access_type(is_write), (void *)address, left_of_object ? meta->addr - address : address - meta->addr, left_of_object ? "left" : "right", object_index); @@ -213,14 +213,14 @@ void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *r case KFENCE_ERROR_UAF: pr_err("BUG: KFENCE: use-after-free %s in %pS\n\n", get_access_type(is_write), (void *)stack_entries[skipnr]); - pr_err("Use-after-free %s at 0x%p (in kfence-#%zd):\n", + pr_err("Use-after-free %s at 0x%p (in kfence-#%td):\n", get_access_type(is_write), (void *)address, object_index); break; case KFENCE_ERROR_CORRUPTION: pr_err("BUG: KFENCE: memory corruption in %pS\n\n", (void *)stack_entries[skipnr]); pr_err("Corrupted memory at 0x%p ", (void *)address); print_diff_canary(address, 16, meta); - pr_cont(" (in kfence-#%zd):\n", object_index); + pr_cont(" (in kfence-#%td):\n", object_index); break; case KFENCE_ERROR_INVALID: pr_err("BUG: KFENCE: invalid %s in %pS\n\n", get_access_type(is_write), @@ -230,7 +230,7 @@ void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *r break; case KFENCE_ERROR_INVALID_FREE: pr_err("BUG: KFENCE: invalid free in %pS\n\n", (void *)stack_entries[skipnr]); - pr_err("Invalid free of 0x%p (in kfence-#%zd):\n", (void *)address, + pr_err("Invalid free of 0x%p (in kfence-#%td):\n", (void *)address, object_index); break; }
Use %td for ptrdiff_t. Link: https://lkml.kernel.org/r/3abbe4c9-16ad-c168-a90f-087978ccd8f7@csgroup.eu Reported-by: Christophe Leroy <christophe.leroy@csgroup.eu> Signed-off-by: Marco Elver <elver@google.com> --- mm/kfence/report.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)