linux-kernel - Re: [RFC patch 08/18] cnt32_to_63 should use smp

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: <20081107170902.GD22134@Krystal>
Date:	Fri, 7 Nov 2008 12:09:02 -0500
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	David Howells <dhowells@...hat.com>
Cc:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	akpm@...ux-foundation.org, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org, Nicolas Pitre <nico@....org>,
	Ralf Baechle <ralf@...ux-mips.org>, benh@...nel.crashing.org,
	paulus@...ba.org, David Miller <davem@...emloft.net>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-arch@...r.kernel.org
Subject: Re: [RFC patch 08/18] cnt32_to_63 should use smp_rmb()

* David Howells (dhowells@...hat.com) wrote:
> Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca> wrote:
> 
> > Assume the time source is a global clock which insures that time will never
> > *ever* go backward. Use a smp_rmb() to make sure the cnt_lo value is read before
> > __m_cnt_hi.
> 
> If you have an smp_rmb(), then don't you need an smp_wmb()/smp_mb() to match
> it to make it work?  And is your assumption valid that smp_rmb() will affect
> memory vs the I/O access to read the clock?  There's no requirement that
> cnt_lo will have been read from an MMIO location at all.
> 
> David

I want to make sure

  __m_cnt_hi
 is read before
  mmio cnt_lo read

for the detailed reasons explained in my previous discussion with
Nicolas here :
http://lkml.org/lkml/2008/10/21/1

I use smp_rmb() to do this on SMP systems (hrm, actually, a rmb() could
be required so it works also on UP systems safely wrt interrupts).

The write side is between the hardware counter, which is assumed to
increment monotonically between each read, and the value __m_cnt_hi
updated by the CPU. I don't see where we could put a wmb() there.

Without barrier, the smp race looks as follow :


CPU    A                                    B
                                            read hw cnt low (0xFFFFFFFA)
       read __m_cnt_hi (0x80000000)
       read hw cnt low (0x00000001)
       (wrap detected :
        (s32)(0x80000000 ^ 0x1) < 0)
       write __m_cnt_hi = 0x00000001
                                            read __m_cnt_hi (0x00000001)
       return 0x0000000100000001
                                            (wrap detected :
                                             (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
                                            write __m_cnt_hi = 0x80000001
                                            return 0x80000001FFFFFFFA
                                                                  (time jumps)

And UP interrupt race :

       Thread context                       Interrupts
       read hw cnt low (0xFFFFFFFA)
                                            read __m_cnt_hi (0x80000000)
                                            read hw cnt low (0x00000001)
                                            (wrap detected :
                                             (s32)(0x80000000 ^ 0x1) < 0)
                                            write __m_cnt_hi = 0x00000001
       read __m_cnt_hi (0x00000001)
                                            return 0x0000000100000001
       (wrap detected :
        (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
       write __m_cnt_hi = 0x80000001
       return 0x80000001FFFFFFFA
                (time jumps)

New code to fix it here with full rmb() :

static inline u64 cnt32_to_63(u32 io_addr, u32 *__m_cnt_hi)
{
        union cnt32_to_63 __x;
        __x.hi = *__m_cnt_hi;   /* memory read for high bits internal state */
        rmb();                  /*
                                 * read high bits before low bits insures time
                                 * does not go backward. Sync across
                                 * CPUs and for interrupts.
                                 */
        __x.lo = readl(cnt_lo); /* mmio read */
        if (unlikely((s32)(__x.hi ^ __x.lo) < 0))
                *__m_cnt_hi =
                        __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31);
        return __x.val;
}

Mathieu

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
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/