From patchwork Fri May 18 22:26:39 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anton Vorontsov X-Patchwork-Id: 8830 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 5733A23EB5 for ; Fri, 18 May 2012 22:28:11 +0000 (UTC) Received: from mail-gg0-f180.google.com (mail-gg0-f180.google.com [209.85.161.180]) by fiordland.canonical.com (Postfix) with ESMTP id 10E2AA18340 for ; Fri, 18 May 2012 22:28:10 +0000 (UTC) Received: by ggnf1 with SMTP id f1so4151954ggn.11 for ; Fri, 18 May 2012 15:28:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf:date:from :to:cc:subject:message-id:references:mime-version:content-type :content-disposition:in-reply-to:user-agent:x-gm-message-state; bh=ckYJqDjITqov/2/mIj0mSNme5qPIJm5UF1lsOHwcK3w=; b=pLw2VHjkWCFS2rirfVUQ6eGQWUdkwnyBsPF4WCQmYF/6iufBZXYvJ88Mt8PHx712fM gq0i3Xp4m+1YttA+yds3/aE3xMTkISbMAR2Jp59xJgbJVq1CWfgprHKf+dcCGCQYzyKu jBKXE1ttTDFY5ZUzfADDH7OESbLDXmv1S+lnQ5SMYDAmet6uz2Am6SudaO7Am45Kdz1l /tHx6QGdMrTYyXXoEKStxBvnejEd31RE3TMbjcOjEB/jsYvwk4NxBetREiJvzD4G2Ooz JjY3YMvBpc0w+ipNAqc+SYmSuKgZPJW7KVTbsVi1aa3nBvIHj3dOqampqt7WVXPJTN1d 8V9A== Received: by 10.50.40.193 with SMTP id z1mr2110695igk.0.1337380090415; Fri, 18 May 2012 15:28:10 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.231.35.72 with SMTP id o8csp135346ibd; Fri, 18 May 2012 15:28:09 -0700 (PDT) Received: by 10.68.242.197 with SMTP id ws5mr2973421pbc.12.1337380089778; Fri, 18 May 2012 15:28:09 -0700 (PDT) Received: from mail-pz0-f50.google.com (mail-pz0-f50.google.com [209.85.210.50]) by mx.google.com with ESMTPS id pm1si9712546pbc.68.2012.05.18.15.28.09 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 18 May 2012 15:28:09 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.210.50 is neither permitted nor denied by best guess record for domain of anton.vorontsov@linaro.org) client-ip=209.85.210.50; Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.210.50 is neither permitted nor denied by best guess record for domain of anton.vorontsov@linaro.org) smtp.mail=anton.vorontsov@linaro.org Received: by mail-pz0-f50.google.com with SMTP id h15so5258947dan.37 for ; Fri, 18 May 2012 15:28:09 -0700 (PDT) Received: by 10.68.136.167 with SMTP id qb7mr42907297pbb.82.1337380088892; Fri, 18 May 2012 15:28:08 -0700 (PDT) Received: from localhost (c-71-204-165-222.hsd1.ca.comcast.net. [71.204.165.222]) by mx.google.com with ESMTPS id ou5sm14125374pbb.54.2012.05.18.15.28.06 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 18 May 2012 15:28:08 -0700 (PDT) Date: Fri, 18 May 2012 15:26:39 -0700 From: Anton Vorontsov To: Greg Kroah-Hartman , Kees Cook , Colin Cross , Tony Luck Cc: Arnd Bergmann , John Stultz , Shuah Khan , arve@android.com, Rebecca Schultz Zavin , Jesper Juhl , Randy Dunlap , Stephen Boyd , Thomas Meyer , Andrew Morton , Marco Stornelli , WANG Cong , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, linaro-kernel@lists.linaro.org, patches@linaro.org, kernel-team@android.com Subject: [PATCH 14/14] pstore/platform: Remove automatic updates Message-ID: <20120518222639.GN23089@lizard> References: <20120518222314.GA9425@lizard> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20120518222314.GA9425@lizard> User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQkXcLW02XIxX5y4bO7ceJmiRwz4DVjkhw64FVde7cEGZyG02m43aNKuWubiSZrrEUvzbVnb Having automatic updates seems pointless, and even dangerous and thus counter-productive: 1. If we can mount pstore, or read files, we can as well read /proc/kmsg. So, there's little point in duplicating the functionality and present the same information but via another userland ABI; 2. Expecting the kernel to behave sanely after oops/panic is naive. It might work, but you'd rather not try it. Screwed up kernel can do rather bad things, like recursive faults[1]; and pstore rather provoking bad things to happen. It uses: 1. Timers (assumes sane interrupts state); 2. Workqueues and mutexes (assumes scheduler in a sane state); 3. kzalloc (a working slab allocator); That's too much for a dead kernel, so the debugging facility itself might just make debugging harder, which is not what we want. So, let's remove the automatic updates, this keeps things simple and safe. (Maybe for non-oops message types it would make sense to add automatic updates, but so far I don't see any use case for this. Even for tracing, it has its own run-time/normal ABI, so we're only interested in pstore upon next boot, to retrieve what has gone wrong with HW or SW.) [1] BUG: unable to handle kernel paging request at fffffffffffffff8 IP: [] kthread_data+0xb/0x20 [...] Process kworker/0:1 (pid: 14, threadinfo ffff8800072c0000, task ffff88000725b100) [... Call Trace: [] wq_worker_sleeping+0x10/0xa0 [] __schedule+0x568/0x7d0 [] ? trace_hardirqs_on+0xd/0x10 [] ? call_rcu_sched+0x12/0x20 [] ? release_task+0x156/0x2d0 [] ? release_task+0x1e/0x2d0 [] ? trace_hardirqs_on+0xd/0x10 [] schedule+0x24/0x70 [] do_exit+0x1f8/0x370 [] oops_end+0x77/0xb0 [] no_context+0x1a6/0x1b5 [] __bad_area_nosemaphore+0x1ce/0x1ed [] ? ttwu_queue+0xc6/0xe0 [] bad_area_nosemaphore+0xe/0x10 [] do_page_fault+0x2c7/0x450 [] ? __lock_release+0x6b/0xe0 [] ? mark_held_locks+0x61/0x140 [] ? __wake_up+0x4e/0x70 [] ? trace_hardirqs_off_thunk+0x3a/0x3c [] ? pstore_register+0x120/0x120 [] page_fault+0x1f/0x30 [] ? pstore_register+0x120/0x120 [] ? memcpy+0x68/0x110 [] ? pstore_get_records+0x3a/0x130 [] ? persistent_ram_copy_old+0x64/0x90 [] ramoops_pstore_read+0x84/0x130 [] pstore_get_records+0x79/0x130 [] ? process_one_work+0x116/0x450 [] ? pstore_register+0x120/0x120 [] pstore_dowork+0xe/0x10 [] process_one_work+0x174/0x450 [] ? process_one_work+0x116/0x450 [] worker_thread+0x123/0x2d0 [] ? manage_workers.isra.28+0x120/0x120 [] kthread+0x8e/0xa0 [] kernel_thread_helper+0x4/0x10 [] ? retint_restore_args+0xe/0xe [] ? __init_kthread_worker+0x70/0x70 [] ? gs_change+0xb/0xb Code: be e2 00 00 00 48 c7 c7 d1 2a 4e 81 e8 bf fb fd ff 48 8b 5d f0 4c 8b 65 f8 c9 c3 0f 1f 44 00 00 48 8b 87 08 02 00 00 55 48 89 e5 <48> 8b 40 f8 5d c3 66 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 RIP [] kthread_data+0xb/0x20 RSP CR2: fffffffffffffff8 ---[ end trace 996a332dc399111d ]--- Fixing recursive fault but reboot is needed! Signed-off-by: Anton Vorontsov --- fs/pstore/platform.c | 37 ------------------------------------- 1 file changed, 37 deletions(-) diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index a3f6d96..3c7ac9b 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -27,30 +27,13 @@ #include #include #include -#include #include #include #include -#include #include "internal.h" /* - * We defer making "oops" entries appear in pstore - see - * whether the system is actually still running well enough - * to let someone see the entry - */ -#define PSTORE_INTERVAL (60 * HZ) - -static int pstore_new_entry; - -static void pstore_timefunc(unsigned long); -static DEFINE_TIMER(pstore_timer, pstore_timefunc, 0, 0); - -static void pstore_dowork(struct work_struct *); -static DECLARE_WORK(pstore_work, pstore_dowork); - -/* * pstore_lock just protects "psinfo" during * calls to pstore_register() */ @@ -140,8 +123,6 @@ static void pstore_dump(struct kmsg_dumper *dumper, ret = psinfo->write(PSTORE_TYPE_DMESG, reason, &id, part, hsize + l1_cpy + l2_cpy, psinfo); - if (ret == 0 && reason == KMSG_DUMP_OOPS && pstore_is_mounted()) - pstore_new_entry = 1; l1 -= l1_cpy; l2 -= l2_cpy; total += l1_cpy + l2_cpy; @@ -227,9 +208,6 @@ int pstore_register(struct pstore_info *psi) kmsg_dump_register(&pstore_dumper); pstore_register_console(); - pstore_timer.expires = jiffies + PSTORE_INTERVAL; - add_timer(&pstore_timer); - return 0; } EXPORT_SYMBOL_GPL(pstore_register); @@ -275,20 +253,5 @@ out: failed, psi->name); } -static void pstore_dowork(struct work_struct *work) -{ - pstore_get_records(1); -} - -static void pstore_timefunc(unsigned long dummy) -{ - if (pstore_new_entry) { - pstore_new_entry = 0; - schedule_work(&pstore_work); - } - - mod_timer(&pstore_timer, jiffies + PSTORE_INTERVAL); -} - module_param(backend, charp, 0444); MODULE_PARM_DESC(backend, "Pstore backend to use");