linux-kernel - Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50A70199.9030109@us.ibm.com>
Date:	Fri, 16 Nov 2012 19:16:41 -0800
From:	John Stultz <johnstul@...ibm.com>
To:	Anton Vorontsov <anton.vorontsov@...aro.org>
CC:	Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org,
	Colin Cross <ccross@...roid.com>,
	Tony Luck <tony.luck@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable

On 11/16/2012 06:53 PM, Anton Vorontsov wrote:
> On Fri, Nov 09, 2012 at 05:26:53PM -0800, Kees Cook wrote:
> [....]
>>>> @@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
>>>> persistent_ram_zone *prz)
>>>>          struct timeval timestamp;
>>>>          size_t len;
>>>>
>>>> -       do_gettimeofday(&timestamp);
>>>> +       /* Handle dumping before timekeeping has resumed. */
>>>> +       if (unlikely(timekeeping_suspended)) {
>>>> +               timestamp.tv_sec = 0;
>>>> +               timestamp.tv_usec = 0;
>>>> +       } else
>>>> +               do_gettimeofday(&timestamp);
>>>> +
>>> 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. :)
> In the lack of agreement on kernel/time/timekeeping.c change, I can't
> apply the patch. And personally I tend to agree that doing this workaround
> in the pstore code is odd. How about introducing ___do_gettimeofday() that
> is safe to call when suspened, and the func would have good kernel doc
> comments explaining the purpose of it?
Yea, I wanted to revisit this, because it is an odd case.

We don't want to call getnstimeofday() while the timekeeping code is 
suspended, since the clocksource cycle_last value may be invalid if the 
hardware was reset during suspend.  Kees is correct,  the WARN_ONs were 
there to make sure no one tries to use the timekeeping core before its 
resumed, so removing them is problematic.

Your sugggestion of having the __do_gettimeofday() internal accessor 
that maybe returns an error if timekeeping has been suspended could work.

The other possibility is depending on the needs for accuracy with the 
timestamp, current_kernel_time() might be a better interface to use, 
since it will return the time at the last tick, and doesn't require 
accessing the clocksource hardware.  Might that be a simpler solution? 
Or is sub-tick granularity necessary?

thanks
-john







--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/