linux-kernel - Re: [BUG] ARCv2: MCIP: GFRC: mcip cmd/readback concurrency

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] [day] [month] [year] [list]

Message-ID: <cb771978-00a6-2243-ba7e-8517644a3101@synopsys.com>
Date:   Thu, 22 Feb 2018 09:21:21 -0800
From:   Vineet Gupta <Vineet.Gupta1@...opsys.com>
To:     Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>,
        "linux-snps-arc@...ts.infradead.org" 
        <linux-snps-arc@...ts.infradead.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Alexey Brodkin" <Alexey.Brodkin@...opsys.com>
Subject: Re: [BUG] ARCv2: MCIP: GFRC: mcip cmd/readback concurrency

On 02/22/2018 06:26 AM, Eugeniy Paltsev wrote:
> To read any data from ARconnect we have special interface
> which includes two AUX registers: MCIP_CMD and MCIP_READBACK.
> We write command to MCIP_CMD and read data from MCIP_READBACK
> after that.
>
> We have only one instance of this registers per cluster, so we need
> to held global lock before access them. This lock is defined
> in arch/arc/kernel/mcip.c
>
>
> To read GFRC value we also use MCIP_CMD/MCIP_READBACK pair, but
> we take only local lock instead of global 'mcip_lock' lock:

This was not an omission but on purpose given the hardware is designed that way. 
We obviously want to avoid the spinlock when possible specially when everyone is 
only reading it and timer readout needs to be super fast. Thing is each core has a 
"private snapshot" of internal counter. So the READ_H* cmd read their own 
snapshots, without need for serializing globally. We do however want a consistent 
LO-HI - hence the need to disable local interrupts around.

> ---------->[drivers/clocksource/arc_timer.c]<----------
> static u64 arc_read_gfrc(struct clocksource *cs)
> {
> 	unsigned long flags;
> 	u32 l, h;
>
> 	local_irq_save(flags);
>
> 	__mcip_cmd(CMD_GFRC_READ_LO, 0);
> 	l = read_aux_reg(ARC_REG_MCIP_READBACK);
>
> 	__mcip_cmd(CMD_GFRC_READ_HI, 0);
> 	h = read_aux_reg(ARC_REG_MCIP_READBACK);
>
> 	local_irq_restore(flags);
>
> 	return (((u64)h) << 32) | l;
> }
> -------------------------->8---------------------------
>
> So we can break any command (like inter core interrupt send)
> which uses MCIP_CMD/MCIP_READBACK pair when we read time from GFRC.
>
> One possible solution is to create function like gfrc_read() in mcip.c
> which will use global 'mcip_lock' and call it from current 'arc_read_gfrc'
> function.
>
> Or we can create a wrapper like 'mcip_read' in arch/arc/kernel/mcip.c
> with next functionality:
> ------->8--------
> u32 mcip_read(u32 cmd)
> {
> 	u32 ret;
>
> 	raw_spin_lock_irqsave(&mcip_lock);
>
> 	__mcip_cmd(cmd, 0);
> 	ret = read_aux_reg(ARC_REG_MCIP_READBACK);
>
> 	raw_spin_unlock_irqrestore(&mcip_lock);
>
> 	return ret;
> }
> ------->8--------

This is a good idea in general. Mind you however that all use cases are different 
- as in we do multiple things not juct cmd + readback. e.g. for GFRC halt progrog 
we do cmd + readback + cmd. but see if you can come up with something reasonable 
to encapsulate the spin lock.