linux-kernel - Re: [PATCH v2] clarify usage expectations for cnt32_to

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:	Sun, 09 Nov 2008 08:34:23 -0500 (EST)
From:	Nicolas Pitre <nico@....org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Russell King <rmk+lkml@....linux.org.uk>,
	David Howells <dhowells@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	lkml <linux-kernel@...r.kernel.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: [PATCH v2] clarify usage expectations for cnt32_to_63()

On Sun, 9 Nov 2008, Mathieu Desnoyers wrote:

> * Nicolas Pitre (nico@....org) wrote:
> > Currently, all existing users of cnt32_to_63() are fine since the CPU
> > architectures where it is used don't do read access reordering, and user
> > mode preemption is disabled already.  It is nevertheless a good idea to
> > better elaborate usage requirements wrt preemption, and use an explicit
> > memory barrier on SMP to avoid different CPUs accessing the counter
> > value in the wrong order.  On UP a simple compiler barrier is 
> > sufficient.
> > 
> > Signed-off-by: Nicolas Pitre <nico@...vell.com>
> > ---
> > 
> ...
> > @@ -68,9 +77,10 @@ union cnt32_to_63 {
> >   */
> >  #define cnt32_to_63(cnt_lo) \
> >  ({ \
> > -	static volatile u32 __m_cnt_hi; \
> > +	static u32 __m_cnt_hi; \
> 
> It's important to get the smp_rmb() here, which this patch provides, so
> consider this patch to be acked-by me. The added documentation is needed
> too.

Thanks.

> But I also think that declaring the static u32 __m_cnt_hi here is
> counter-intuitive for developers who wish to use it.

I'm rather not convinced of that.  And this is a much bigger change 
affecting all callers so I'd defer such change even if I was convinced 
of it.

> I'd recommend letting the declaration be done outside of cnt32_to_63 so
> the same variable can be passed as parameter to more than one execution
> site.

Do you really have such instances where multiple call sites are needed?  
That sounds even more confusing to me than the current model.  Better 
encapsulate the usage of this macro within some function which has a 
stronger meaning, such as sched_clock(), and call _that_ from multiple 
sites instead.


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