| Message ID | 20190503175041.7949-4-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mips: Post-bootmem-memblock transition fixes | expand |
Hi Serge, On Fri, May 03, 2019 at 08:50:39PM +0300, Serge Semin wrote: > It is useful at least for debugging to have the kernel virtual > memory layout printed at boot time so to have the full information > about the booted kernel. Make the printing optional and available > only when DEBUG_KERNEL config is enabled so not to leak a sensitive > kernel information. > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > --- > arch/mips/mm/init.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) FYI the rest of the series is in mips-next, but I left this one out because it gives me compile errors for 64r6el_defconfig: In file included from ./include/linux/printk.h:7, from ./include/linux/kernel.h:15, from ./include/asm-generic/bug.h:18, from ./arch/mips/include/asm/bug.h:42, from ./include/linux/bug.h:5, from arch/mips/mm/init.c:11: arch/mips/mm/init.c: In function ‘mem_print_kmap_info’: ./include/linux/kern_levels.h:5:18: error: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘long long unsigned int’ [-Werror=format=] #define KERN_SOH "\001" /* ASCII Start Of Header */ ^~~~~~ ./include/linux/kern_levels.h:13:21: note: in expansion of macro ‘KERN_SOH’ #define KERN_NOTICE KERN_SOH "5" /* normal but significant condition */ ^~~~~~~~ ./include/linux/printk.h:307:9: note: in expansion of macro ‘KERN_NOTICE’ printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__) ^~~~~~~~~~~ arch/mips/mm/init.c:69:2: note: in expansion of macro ‘pr_notice’ pr_notice("Kernel virtual memory layout:\n" ^~~~~~~~~ arch/mips/mm/init.c:70:39: note: format string is defined here " lowmem : 0x%px - 0x%px (%4ld MB)\n" ~~~^ %4lld In file included from ./arch/mips/include/asm/bug.h:5, from ./include/linux/bug.h:5, from arch/mips/mm/init.c:11: In function ‘mem_print_kmap_info’, inlined from ‘mem_init’ at arch/mips/mm/init.c:530:2: ./include/linux/compiler.h:344:38: error: call to ‘__compiletime_assert_99’ declared with attribute error: BUILD_BUG_ON failed: FIXADDR_TOP < PAGE_OFFSET _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) ^ ./include/linux/compiler.h:325:4: note: in definition of macro ‘__compiletime_assert’ prefix ## suffix(); \ ^~~~~~ ./include/linux/compiler.h:344:2: note: in expansion of macro ‘_compiletime_assert’ _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) ^~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) ^~~~~~~~~~~~~~~~ arch/mips/mm/init.c:99:2: note: in expansion of macro ‘BUILD_BUG_ON’ BUILD_BUG_ON(FIXADDR_TOP < PAGE_OFFSET); ^~~~~~~~~~~~ cc1: all warnings being treated as errors make[3]: *** [scripts/Makefile.build:278: arch/mips/mm/init.o] Error 1 make[2]: *** [scripts/Makefile.build:489: arch/mips/mm] Error 2 Thanks, Paul
Hello Paul On Mon, May 06, 2019 at 07:14:21PM +0000, Paul Burton wrote: > Hi Serge, > > On Fri, May 03, 2019 at 08:50:39PM +0300, Serge Semin wrote: > > It is useful at least for debugging to have the kernel virtual > > memory layout printed at boot time so to have the full information > > about the booted kernel. Make the printing optional and available > > only when DEBUG_KERNEL config is enabled so not to leak a sensitive > > kernel information. > > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > arch/mips/mm/init.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > FYI the rest of the series is in mips-next, but I left this one out > because it gives me compile errors for 64r6el_defconfig: > > In file included from ./include/linux/printk.h:7, > from ./include/linux/kernel.h:15, > from ./include/asm-generic/bug.h:18, > from ./arch/mips/include/asm/bug.h:42, > from ./include/linux/bug.h:5, > from arch/mips/mm/init.c:11: > arch/mips/mm/init.c: In function ‘mem_print_kmap_info’: > ./include/linux/kern_levels.h:5:18: error: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘long long unsigned int’ [-Werror=format=] > #define KERN_SOH "\001" /* ASCII Start Of Header */ > ^~~~~~ > ./include/linux/kern_levels.h:13:21: note: in expansion of macro ‘KERN_SOH’ > #define KERN_NOTICE KERN_SOH "5" /* normal but significant condition */ > ^~~~~~~~ > ./include/linux/printk.h:307:9: note: in expansion of macro ‘KERN_NOTICE’ > printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__) > ^~~~~~~~~~~ > arch/mips/mm/init.c:69:2: note: in expansion of macro ‘pr_notice’ > pr_notice("Kernel virtual memory layout:\n" > ^~~~~~~~~ > arch/mips/mm/init.c:70:39: note: format string is defined here > " lowmem : 0x%px - 0x%px (%4ld MB)\n" > ~~~^ > %4lld > In file included from ./arch/mips/include/asm/bug.h:5, > from ./include/linux/bug.h:5, > from arch/mips/mm/init.c:11: > In function ‘mem_print_kmap_info’, > inlined from ‘mem_init’ at arch/mips/mm/init.c:530:2: > ./include/linux/compiler.h:344:38: error: call to ‘__compiletime_assert_99’ declared with attribute error: BUILD_BUG_ON failed: FIXADDR_TOP < PAGE_OFFSET > _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) > ^ > ./include/linux/compiler.h:325:4: note: in definition of macro ‘__compiletime_assert’ > prefix ## suffix(); \ > ^~~~~~ > ./include/linux/compiler.h:344:2: note: in expansion of macro ‘_compiletime_assert’ > _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) > ^~~~~~~~~~~~~~~~~~~ > ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ > #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > ^~~~~~~~~~~~~~~~~~ > ./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ > BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) > ^~~~~~~~~~~~~~~~ > arch/mips/mm/init.c:99:2: note: in expansion of macro ‘BUILD_BUG_ON’ > BUILD_BUG_ON(FIXADDR_TOP < PAGE_OFFSET); > ^~~~~~~~~~~~ > cc1: all warnings being treated as errors > make[3]: *** [scripts/Makefile.build:278: arch/mips/mm/init.o] Error 1 > make[2]: *** [scripts/Makefile.build:489: arch/mips/mm] Error 2 > > Thanks, > Paul Thanks for the report regarding this issue. I actually thought I tested the patch being buildable for 64bit systems. It turns out I didn't.( Should I resend the fixed patch as a separate v3 one In-Reply-to this v2 patch or resubmit the patchset with cover-letter and only the fixed patch being there? Cheers, -Sergey
Hi Serge, On Wed, May 08, 2019 at 01:36:07AM +0300, Serge Semin wrote: > Thanks for the report regarding this issue. I actually thought I > tested the patch being buildable for 64bit systems. It turns out I > didn't.( Easily done :) > Should I resend the fixed patch as a separate v3 one In-Reply-to this > v2 patch or resubmit the patchset with cover-letter and only the fixed > patch being there? Replying with just v3 of this patch will be fine, no need to resend the cover letter. I currently plan to submit a pull request for mips-next as-is, without this patch, in the next day or two. There are a few last minute submissions this time round that I'll then queue up & send a second pull request next week, which this can be part of. Thanks, Paul
On Tue, May 07, 2019 at 10:41:10PM +0000, Paul Burton wrote: > Hi Serge, > > On Wed, May 08, 2019 at 01:36:07AM +0300, Serge Semin wrote: > > Thanks for the report regarding this issue. I actually thought I > > tested the patch being buildable for 64bit systems. It turns out I > > didn't.( > > Easily done :) > > > Should I resend the fixed patch as a separate v3 one In-Reply-to this > > v2 patch or resubmit the patchset with cover-letter and only the fixed > > patch being there? > > Replying with just v3 of this patch will be fine, no need to resend the > cover letter. > Ok. I've just submitted the v3 version with fixed buildability problem. > I currently plan to submit a pull request for mips-next as-is, without > this patch, in the next day or two. There are a few last minute > submissions this time round that I'll then queue up & send a second pull > request next week, which this can be part of. > > Thanks, > Paul Regarding this patch being part of the mips mm init code. I've just found out that 32-bit arm subsystem maintainers removed the same functionality from the kernel 5.1. This also was removed from arm64 in kernel 4.15: commit 1c31d4e96b8c ("ARM: 8820/1: mm: Stop printing the virtual memory layout") commit 071929dbdd86 ("arm64: Stop printing the virtual memory layout") Maintainer of m68k and unicore32 discarded the printing as well: commit 1476ea250cf0 ("unicore32: stop printing the virtual memory layout") commit 31833332f798 ("m68k/mm: Stop printing the virtual memory layout") The reasoning of these removal was that since commit ad67b74d2469 ("printk: hash addresses printed with %p") the kernel virtual addresses weren't printed to the system log anyway. So instead of replacing the format string with "%px" they decided not to leak a virtual memory layout information and completely removed the printing. I don't really know why they didn't closed the printing for debug kernel only as we did, since the info might be useful in this case. Since I see a tendency of this functionality removal, we might need to reconsider this patch integration into the MIPS arch code. What do you think? Although some architectures still perform the virtual memory layout printing at boot-time: x86_32, parisc, xtensa, sh, nds32 (might be others). Cheers, -Sergey
Hello Paul On Wed, May 08, 2019 at 02:38:49AM +0300, Serge Semin wrote: > On Tue, May 07, 2019 at 10:41:10PM +0000, Paul Burton wrote: > > Hi Serge, > > > > On Wed, May 08, 2019 at 01:36:07AM +0300, Serge Semin wrote: > > > Thanks for the report regarding this issue. I actually thought I > > > tested the patch being buildable for 64bit systems. It turns out I > > > didn't.( > > > > Easily done :) > > > > > Should I resend the fixed patch as a separate v3 one In-Reply-to this > > > v2 patch or resubmit the patchset with cover-letter and only the fixed > > > patch being there? > > > > Replying with just v3 of this patch will be fine, no need to resend the > > cover letter. > > > > Ok. I've just submitted the v3 version with fixed buildability problem. > > > I currently plan to submit a pull request for mips-next as-is, without > > this patch, in the next day or two. There are a few last minute > > submissions this time round that I'll then queue up & send a second pull > > request next week, which this can be part of. > > > > Thanks, > > Paul > > Regarding this patch being part of the mips mm init code. I've just found out > that 32-bit arm subsystem maintainers removed the same functionality from the > kernel 5.1. This also was removed from arm64 in kernel 4.15: > commit 1c31d4e96b8c ("ARM: 8820/1: mm: Stop printing the virtual memory layout") > commit 071929dbdd86 ("arm64: Stop printing the virtual memory layout") > > Maintainer of m68k and unicore32 discarded the printing as well: > commit 1476ea250cf0 ("unicore32: stop printing the virtual memory layout") > commit 31833332f798 ("m68k/mm: Stop printing the virtual memory layout") > > The reasoning of these removal was that since commit ad67b74d2469 ("printk: > hash addresses printed with %p") the kernel virtual addresses weren't > printed to the system log anyway. So instead of replacing the format string with > "%px" they decided not to leak a virtual memory layout information and completely > removed the printing. I don't really know why they didn't closed the printing for > debug kernel only as we did, since the info might be useful in this case. > > Since I see a tendency of this functionality removal, we might need to > reconsider this patch integration into the MIPS arch code. What do you think? > > Although some architectures still perform the virtual memory layout printing > at boot-time: x86_32, parisc, xtensa, sh, nds32 (might be others). > > Cheers, > -Sergey So any update on this patch status? Regards, -Sergey
diff --git a/arch/mips/mm/init.c b/arch/mips/mm/init.c index bbb196ad5f26..c338bbd03b2a 100644 --- a/arch/mips/mm/init.c +++ b/arch/mips/mm/init.c @@ -31,6 +31,7 @@ #include <linux/gfp.h> #include <linux/kcore.h> #include <linux/initrd.h> +#include <linux/sizes.h> #include <asm/bootinfo.h> #include <asm/cachectl.h> @@ -56,6 +57,53 @@ unsigned long empty_zero_page, zero_page_mask; EXPORT_SYMBOL_GPL(empty_zero_page); EXPORT_SYMBOL(zero_page_mask); +/* + * Print out the kernel virtual memory layout + */ +#define MLK(b, t) (void *)b, (void *)t, ((t) - (b)) >> 10 +#define MLM(b, t) (void *)b, (void *)t, ((t) - (b)) >> 20 +#define MLK_ROUNDUP(b, t) (void *)b, (void *)t, DIV_ROUND_UP(((t) - (b)), SZ_1K) +static void __init mem_print_kmap_info(void) +{ +#ifdef CONFIG_DEBUG_KERNEL + pr_notice("Kernel virtual memory layout:\n" + " lowmem : 0x%px - 0x%px (%4ld MB)\n" + " .text : 0x%px - 0x%px (%4td kB)\n" + " .data : 0x%px - 0x%px (%4td kB)\n" + " .init : 0x%px - 0x%px (%4td kB)\n" + " .bss : 0x%px - 0x%px (%4td kB)\n" + " vmalloc : 0x%px - 0x%px (%4ld MB)\n" +#ifdef CONFIG_HIGHMEM + " pkmap : 0x%px - 0x%px (%4ld MB)\n" +#endif + " fixmap : 0x%px - 0x%px (%4ld kB)\n", + MLM(PAGE_OFFSET, (unsigned long)high_memory), + MLK_ROUNDUP(_text, _etext), + MLK_ROUNDUP(_sdata, _edata), + MLK_ROUNDUP(__init_begin, __init_end), + MLK_ROUNDUP(__bss_start, __bss_stop), + MLM(VMALLOC_START, VMALLOC_END), +#ifdef CONFIG_HIGHMEM + MLM(PKMAP_BASE, (PKMAP_BASE) + (LAST_PKMAP)*(PAGE_SIZE)), +#endif + MLK(FIXADDR_START, FIXADDR_TOP)); + + /* Check some fundamental inconsistencies. May add something else? */ +#ifdef CONFIG_HIGHMEM + BUILD_BUG_ON(VMALLOC_END < PAGE_OFFSET); + BUG_ON(VMALLOC_END < (unsigned long)high_memory); + BUILD_BUG_ON((PKMAP_BASE) + (LAST_PKMAP)*(PAGE_SIZE) < PAGE_OFFSET); + BUG_ON((PKMAP_BASE) + (LAST_PKMAP)*(PAGE_SIZE) < + (unsigned long)high_memory); +#endif + BUILD_BUG_ON(FIXADDR_TOP < PAGE_OFFSET); + BUG_ON(FIXADDR_TOP < (unsigned long)high_memory); +#endif /* CONFIG_DEBUG_KERNEL */ +} +#undef MLK +#undef MLM +#undef MLK_ROUNDUP + /* * Not static inline because used by IP27 special magic initialization code */ @@ -479,6 +527,7 @@ void __init mem_init(void) setup_zero_pages(); /* Setup zeroed pages. */ mem_init_free_highmem(); mem_init_print_info(NULL); + mem_print_kmap_info(); #ifdef CONFIG_64BIT if ((unsigned long) &_text > (unsigned long) CKSEG0)
It is useful at least for debugging to have the kernel virtual memory layout printed at boot time so to have the full information about the booted kernel. Make the printing optional and available only when DEBUG_KERNEL config is enabled so not to leak a sensitive kernel information. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- arch/mips/mm/init.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)