linux-kernel - Re: [RFC local_t removal V1 1/4] Add add_local() and add_local

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100107135757.GA6011@Krystal>
Date:	Thu, 7 Jan 2010 08:57:57 -0500
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Christoph Lameter <cl@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC local_t removal V1 1/4] Add add_local() and
	add_local_return()

* Arnd Bergmann (arnd@...db.de) wrote:
> On Tuesday 05 January 2010, Mathieu Desnoyers wrote:
> > 
> > The problem I see here is that with ~5-6 operations, we will end up
> > having 20*5 = 100 headers only for this. Can we combine these in a
> > single header file instead ? local.h wasn't bad in this respect.
> 
> I have an old patch that I was planning to dig out for 2.6.34,
> which autogenerates arch/*/include/foo.h files that only contain
> "#include <asm-generic/foo.h>".
> 
> I guess this would be sufficient to avoid the overload with all
> these header files.

Well, given we already have local.h, I am not completely sure that this
whole exercise is giving us.

[...]

> > > +#include <linux/types.h>
> > > +
> > > +extern unsigned long wrong_size_add_local(volatile void *ptr);
> > > +
> > > +/*
> > > + * Generic version of __add_return_local (disables interrupts). Takes an
> > > + * unsigned long parameter, supporting various types of architectures.
> > > + */
> > > +static inline unsigned long __add_return_local_generic(volatile void *ptr,
> > > +		unsigned long value, int size)
> 
> You could probably lose the 'volatile' here, if you want to discourage
> marking data as volatile in the code.
> 
> > > +{
> > > +	unsigned long flags, r;
> > > +
> > > +	/*
> > > +	 * Sanity checking, compile-time.
> > > +	 */
> > > +	if (size == 8 && sizeof(unsigned long) != 8)
> > > +		wrong_size_add_local(ptr);
> 
> It can be BUILD_BUG_ON if you move it to the outer macro.
> 
> > > +	local_irq_save(flags);
> > > +	switch (size) {
> > > +	case 1: r = (*((u8 *)ptr) += value);
> > > +		break;
> > > +	case 2: r = (*((u16 *)ptr) += value);
> > > +		break;
> > > +	case 4: r = (*((u32 *)ptr) += value);
> > > +		break;
> > > +	case 8: r = (*((u64 *)ptr) += value);
> > > +		break;
> 
> But I think here you actually need to add the volatile in order
> to make these atomic assignments.

Yes, you are right. If we ever try to access these variables from a
remote CPU with a load (but not with any concurrent store operation, as
this would be semantically invalid), then the volatile is important.

Mathieu

> 
> 	Arnd

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