linux-kernel - Re: [PATCH 1/2] clocksource: replace cycle

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: <alpine.DEB.2.11.1510301533310.4032@nanos>
Date:	Fri, 30 Oct 2015 15:56:46 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Yang Yingliang <yangyingliang@...wei.com>
cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] clocksource: replace cycle_last validation with an
 equal way

Yang,

On Tue, 27 Oct 2015, Yang Yingliang wrote:

> Mask the cycle values before subtraction. So we can use this
> validation while the clocksource mask is not 64-bits.
> 
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> ---
>  kernel/time/timekeeping_internal.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/timekeeping_internal.h b/kernel/time/timekeeping_internal.h
> index 4ea005a..984f02e 100644
> --- a/kernel/time/timekeeping_internal.h
> +++ b/kernel/time/timekeeping_internal.h
> @@ -15,7 +15,7 @@ extern void tk_debug_account_sleep_time(struct timespec64 *t);
>  #ifdef CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE
>  static inline cycle_t clocksource_delta(cycle_t now, cycle_t last, cycle_t mask)
>  {
> -	cycle_t ret = (now - last) & mask;
> +	cycle_t ret = (now & mask) - (last & mask);

I agree the original code is broken for all masks which are !=
CLOCKSOURCE_MASK(64).

But your change does not work for actual wraparounds. You probably
cannot trigger it for the 56bits of the arm architected timer, but
that does not make it more correct.

Assume a CLOCKSOURCE_MASK(32) and that the timer wrapped around since
we last read it.

last = 0xffffffff
now = 0x01

So:

	ret = (0x01 & 0xffffffff) - (0xffffffff & 0xffffffff);
-->	ret = 0x01 - 0xffffffff;
-->	ret = ffffffff00000002;

-->	(s64) ret is < 0 !!!

This is wrong as the clocksource legitimately wrapped around since we
accessed it last.

The correct solution to this is:

    	ret = (now - last) & mask;
	
	negative = ret & ~(mask >> 1);

	return negative ? 0 : ret;

So in the above case this will be:

-->	ret = (0x01 - 0xffffffff) & 0xffffffff;
-->	ret = 0x02;

-->	negative = 0x02 & ~(0x0fffffff);
-->	negative = 0;

But for 

last = 0x1000
now = 0x01

-->	ret = (0x01 - 0x1000) & 0xffffffff;
-->	ret = 0xfffff001;

-->	negative = 0xfffff001 & ~(0x0fffffff);
-->	negative = 0x80000000;

And this is the case which we actually need to catch.

Thanks,

	tglx
--
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/