Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752330AbdGGH6F (ORCPT ); Fri, 7 Jul 2017 03:58:05 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:24239 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752156AbdGGH6D (ORCPT ); Fri, 7 Jul 2017 03:58:03 -0400 Subject: Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section To: Sergey Senozhatsky References: <1499337481-19397-1-git-send-email-matt.redfearn@imgtec.com> <20170707044537.GB7478@jagdpanzerIV.localdomain> CC: Petr Mladek , Sergey Senozhatsky , Greg Kroah-Hartman , Jiri Slaby , , Steven Rostedt , From: Matt Redfearn Message-ID: <0212d098-e307-2ea1-cb41-1e81d545da56@imgtec.com> Date: Fri, 7 Jul 2017 08:58:01 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170707044537.GB7478@jagdpanzerIV.localdomain> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.150.130.83] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3439 Lines: 80 On 07/07/17 05:45, Sergey Senozhatsky wrote: > On (07/06/17 11:38), Matt Redfearn wrote: >> Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in >> printk_late_init() if keep_bootcon") added a check on keep_bootcon to >> ensure that boot consoles were kept around until the real console is >> registered. >> This can lead to problems if the boot console data and code are in the >> init section, since it can be freed before the boot console is >> deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only >> unregister boot consoles when necessary"). >> The keep_bootcon flag prevents the unregistration of a boot console, >> even if it's data and code reside in the init section and are about to >> be freed. This can lead to crashes and weird panics when the bootconsole >> is accessed after free, especially if page poisoning is in use and the >> code / data have been overwritten with a poison value. >> To prevent this, always free the boot console if it is within the init >> section. >> >> Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary"). >> Signed-off-by: Matt Redfearn > well... hm. don't specify `keep_bootcon' if your bootcon access initdata? > keeping `early_printk' sometimes can be helpful and people even want to > use `early_printk' as a panic() console fallback (because of those nasty > deadlocks that spoil printk->call_console_drivers()). > > if someone asked to `keep_bootcon' but we actually don't keep it, then > what's the purpose of the flag and > pr_info("debug: skip boot console de-registration.\n")? Sure, but as a user, how are you supposed to know that? And if your kernel is configured with SLAB poisoning, when the earlycon resident in initdata is freed all of it's text and data is replaced with a poison value, meaning it definitely won't work anymore anyway, and will instead likely just panic the kernel, often with a completely meaningless message (I know, because that's how I tracked down the need for this patch). With this patch, the earlycon will only be unregistered if it is in the init section and will no longer be functional anyway. Perhaps it would be better to change the structure to make that more explicit, something like: if ((con->flags & CON_BOOT) && init_section_intersects(con, sizeof(*con))) { /* * Make sure to unregister boot consoles whose data * resides in the init section before the init section * is discarded. Boot consoles whose data will stick * around will automatically be unregistered when the * proper console replaces them. */ unregister_console(con); } Thanks, Matt > > >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >> index a1db38abac5b..b5411f44eed4 100644 >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -2660,7 +2660,7 @@ static int __init printk_late_init(void) >> int ret; >> >> for_each_console(con) { >> - if (!keep_bootcon && con->flags & CON_BOOT) { >> + if (con->flags & CON_BOOT) { >> /* >> * Make sure to unregister boot consoles whose data >> * resides in the init section before the init section > what about bootconsoles that are still not getting de-registered due > to keep_bootcon in register_console()? > > -ss