linux-kernel - Re: [PATCH] clocksource/drivers/pistachio: Fix wrong calculated clocksource read value

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: <20151216103803.GE8644@n2100.arm.linux.org.uk>
Date:	Wed, 16 Dec 2015 10:38:03 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:	Jisheng Zhang <jszhang@...vell.com>, tglx@...utronix.de,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] clocksource/drivers/pistachio: Fix wrong calculated
 clocksource read value

On Wed, Dec 16, 2015 at 11:32:17AM +0100, Daniel Lezcano wrote:
> On 12/16/2015 10:33 AM, Russell King - ARM Linux wrote:
> >On Wed, Dec 16, 2015 at 10:21:55AM +0100, Daniel Lezcano wrote:
> >>On 12/16/2015 08:36 AM, Jisheng Zhang wrote:
> >>>And in fact, clocksource_mmio_readw_down() also has similar issue, but it masks
> >>>with c->mask before return, the c->mask is less than 32 bit (because the
> >>>clocksource_mmio_init think number of valid bits > 32 or < 16 is invalid.)
> >>>the higher 32 bits are masked off, so we never saw such issue. But we'd better
> >>>to fix that, what's your opinion?
> >>
> >>I think we should have a look to this portion closely.
> >
> >There is no need to return more bits than are specified.  If you have
> >a N-bit counter, then the high (64-N)-bits can be any value, because:
> >
> >static inline cycle_t clocksource_delta(cycle_t now, cycle_t last, cycle_t mask)
> >{
> >         return (now - last) & mask;
> >}
> >
> >where 'now' is the current value returned from the clock source read
> >function, 'last' is a previously returned value, and 'mask' is the
> >bit mask.  This has the effect of ignoring the high order bits.
> 
> I think this approach is perfectly sane. When I said we should look at this
> portion closely, I meant we should double check the bitwise-nor order
> regarding the explicit cast. The clocksource's mask makes sense and must
> stay untouched.

That's not my point.  Whether you do:

	~(cycle_t)readl(...)

or

	(cycle_t)~readl(...)

is irrelevant - the result is the same as far as the core code is
concerned as it doesn't care about the higher order bits.

The only thing about which should be done is really which is faster
in the general case, since this is a fast path in the time keeping
code.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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/