Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751486Ab2KJB06 (ORCPT ); Fri, 9 Nov 2012 20:26:58 -0500 Received: from mail-oa0-f46.google.com ([209.85.219.46]:38591 "EHLO mail-oa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750781Ab2KJB0y (ORCPT ); Fri, 9 Nov 2012 20:26:54 -0500 MIME-Version: 1.0 In-Reply-To: <509DA63E.7070500@us.ibm.com> References: <20121105220059.GA6379@www.outflux.net> <509DA63E.7070500@us.ibm.com> Date: Fri, 9 Nov 2012 17:26:53 -0800 X-Google-Sender-Auth: AIu2anCJQ5DNptdmkg3tSwKvIbg Message-ID: Subject: Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable From: Kees Cook To: John Stultz Cc: linux-kernel@vger.kernel.org, Anton Vorontsov , Colin Cross , Tony Luck , Thomas Gleixner Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2515 Lines: 66 On Fri, Nov 9, 2012 at 4:56 PM, John Stultz wrote: > On 11/05/2012 02:00 PM, Kees Cook wrote: >> >> We must not call timekeeping functions unless they are available. If we >> dump >> before they have resumed, avoid a WARN_ON by setting the timestamp to 0. >> >> Since the "ram" pstore driver can be a module, we must have >> timekeeping_suspended exported. >> >> Reported-by: Doug Anderson >> Cc: Anton Vorontsov >> Cc: John Stultz >> Signed-off-by: Kees Cook >> --- >> v2: >> - export needed for timekeeping_suspended (thanks to Fengguang Wu). >> --- >> fs/pstore/ram.c | 8 +++++++- >> kernel/time/timekeeping.c | 1 + >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c >> index 1a4f6da..6d014e0 100644 >> --- a/fs/pstore/ram.c >> +++ b/fs/pstore/ram.c >> @@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct >> persistent_ram_zone *prz) >> struct timeval timestamp; >> size_t len; >> >> - do_gettimeofday(×tamp); >> + /* Handle dumping before timekeeping has resumed. */ >> + if (unlikely(timekeeping_suspended)) { >> + timestamp.tv_sec = 0; >> + timestamp.tv_usec = 0; >> + } else >> + do_gettimeofday(×tamp); >> + > > Would nulling out the timestamp be better done in do_gettimeofday()? That > way we don't have to export timekeeping internals and users would get > something more sane for this corner case. Well... I'm not sure. If we don't want to expose the timekeeping_suspended variable, maybe we need a function to check this? I think it's probably better to find the users of timekeeping that could call it when suspended. That's why I figured the BUG was there. Very very few things should be attempting to call gettimeofday in a place where it might be suspended. As such, it seems like those things should be able to determine how to handle it. Maybe not everything would be sensible to get back 0s. In this particular case, I'm fine with removing the BUG and returning 0 instead, since that's fine for ramoops. :) -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/