linux-kernel - Re: [tip: sched/core] arm64/arch_timer: Provide noinstr sched_clock

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]

Date:   Tue, 6 Jun 2023 09:13:44 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org,
        Michael Kelley <mikelley@...rosoft.com>, x86@...nel.org
Subject: Re: [tip: sched/core] arm64/arch_timer: Provide noinstr
 sched_clock_read() functions

On Tue, Jun 06, 2023 at 10:06:14AM +0200, Peter Zijlstra wrote:
> On Mon, Jun 05, 2023 at 07:16:18PM -0000, tip-bot2 for Peter Zijlstra wrote:
> > @@ -753,14 +771,14 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
> >  	return 0;
> >  }
> >  
> > -static u64 arch_counter_get_cnt_mem(struct arch_timer *t, int offset_lo)
> > +static noinstr u64 arch_counter_get_cnt_mem(struct arch_timer *t, int offset_lo)
> >  {
> >  	u32 cnt_lo, cnt_hi, tmp_hi;
> >  
> >  	do {
> > -		cnt_hi = readl_relaxed(t->base + offset_lo + 4);
> > -		cnt_lo = readl_relaxed(t->base + offset_lo);
> > -		tmp_hi = readl_relaxed(t->base + offset_lo + 4);
> > +		cnt_hi = __raw_readl(t->base + offset_lo + 4);
> > +		cnt_lo = __raw_readl(t->base + offset_lo);
> > +		tmp_hi = __raw_readl(t->base + offset_lo + 4);
> >  	} while (cnt_hi != tmp_hi);
> >  
> >  	return ((u64) cnt_hi << 32) | cnt_lo;
> 
> Mark noted that this looses the byteswap :/
> 
> 
> ---
> Subject: arm64/arch_timer: Fix MMIO byteswap
> 
> The readl_relaxed() to __raw_readl() change meant to loose the
> instrumentation, but also (inadvertently) lost the byteswap.
> 
> Fixes: 24ee7607b286 ("arm64/arch_timer: Provide noinstr sched_clock_read() functions")
> Reported-by: Mark Rutland <mark.rutland@....com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index b23d23b033cc..e733a2a1927a 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -776,9 +776,9 @@ static noinstr u64 arch_counter_get_cnt_mem(struct arch_timer *t, int offset_lo)
>  	u32 cnt_lo, cnt_hi, tmp_hi;
>  
>  	do {
> -		cnt_hi = __raw_readl(t->base + offset_lo + 4);
> -		cnt_lo = __raw_readl(t->base + offset_lo);
> -		tmp_hi = __raw_readl(t->base + offset_lo + 4);
> +		cnt_hi = __le32_to_cpu((__le32 __force)__raw_readl(t->base + offset_lo + 4));
> +		cnt_lo = __le32_to_cpu((__le32 __force)__raw_readl(t->base + offset_lo));
> +		tmp_hi = __le32_to_cpu((__le32 __force)__raw_readl(t->base + offset_lo + 4));
>  	} while (cnt_hi != tmp_hi);

This LGTM, so FWIW:

Acked-by: Mark Rutland <mark.rutland@....com>

Longer term it'd be good to have noistr-safe forms of the MMIO accessors, but
from a quick play that's a much bigger piece of work, and I think this is fine
for now!

Thanks,
Mark.