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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <201001071445.50416.arnd@arndb.de>
Date:	Thu, 7 Jan 2010 14:45:50 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
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()

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.

> Also, separating all these in sub-files will make it a bit difficult to
> pinpoint errors that would arise from using a bad combination of, e.g.
> generic add with a non-protected read or set.

Yes.

> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/include/asm-generic/add-local-generic.h	2010-01-05 15:36:02.000000000 -0600
> > @@ -0,0 +1,40 @@
> > +#ifndef __ASM_GENERIC_ADD_LOCAL_GENERIC_H
> > +#define __ASM_GENERIC_ADD_LOCAL_GENERIC_H

Why split the file between asm-generic/add-local.h and add-local-generic.h?
I don't see how any architecture could use one but not the other.

> > +#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.

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