[v3] devres: Explicitly align datai[] to 64-bit

Message ID 20180709134550.29541-1-abrodkin@synopsys.com
State New
Headers show
Series
  • [v3] devres: Explicitly align datai[] to 64-bit
Related show

Commit Message

Alexey Brodkin July 9, 2018, 1:45 p.m.
data[] must be 64-bit aligned even on 32-bit architectures because
it might be accessed by instructions that require aligned memory arguments.

One example is "atomic64_t" type accessed by special atomic instructions
which may read/write entire 64-bit word.

Atomic instructions are a bit special compared to normal loads and stores.
Even if normal loads and stores may deal with unaligned data, atomic
instructions still require data to be aligned because it's hard to manage
atomic value that spans through multiple cache lines or even MMU pages.
And hardware just raises an alignment fault exception.

The problem with previously used approach is that depending on ABI
"long long" type of a particular 32-bit CPU might be aligned to
8-, 16-, 32- or 64-bit boundary. Which will get in the way of mentioned
above atomic instructions.

Consider the following snippet:
|        struct mystruct {
|                atomic64_t myvar;
|        }
|
|        struct mystruct *p;
|        p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);

Here address of "myvar" will match  data[] in "struct devres",
that said if "data" is not 64-bit aligned atomic instruction will
fail on the first access to "myvar".

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Greg KH <greg@kroah.com>
Cc: <stable@vger.kernel.org> # 4.8+
---

Changes v2 -> v3:

 * Align explicitly to 8 bytes [David]
 * Rephrased in-line comment [David]
 * Added more techinical details to commit message [Greg]
 * Mention more alignment options in commit message [Geert]

Changes v1 -> v2:

 * Reworded commit message
 * Inserted comment right in source [Thomas]

 drivers/base/devres.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Peter Zijlstra July 9, 2018, 1:54 p.m. | #1
On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote:
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index f98a097e73f2..d65327cb83c9 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -24,8 +24,12 @@ struct devres_node {
>  
>  struct devres {
>  	struct devres_node		node;
> -	/* -- 3 pointers */
> -	unsigned long long		data[];	/* guarantee ull alignment */
> +	/*
> +	 * data[] must be 64 bit aligned even on 32 bit architectures
> +	 * because it might be accessed by instructions that require
> +	 * aligned memory arguments such as atomic64_t.
> +	 */
> +	u8 __aligned(8)			data[];
>  };

From a quick reading in Documentation/driver-model/devres.txt this
devres muck is supposed to be device memory, right?

atomics (as in atomic*_t) are not defined for use on mmio.

wth kind of code is relying on this?
Mark Rutland July 9, 2018, 2:04 p.m. | #2
On Mon, Jul 09, 2018 at 03:54:09PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote:
> > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > index f98a097e73f2..d65327cb83c9 100644
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -24,8 +24,12 @@ struct devres_node {
> >  
> >  struct devres {
> >  	struct devres_node		node;
> > -	/* -- 3 pointers */
> > -	unsigned long long		data[];	/* guarantee ull alignment */
> > +	/*
> > +	 * data[] must be 64 bit aligned even on 32 bit architectures
> > +	 * because it might be accessed by instructions that require
> > +	 * aligned memory arguments such as atomic64_t.
> > +	 */
> > +	u8 __aligned(8)			data[];
> >  };
> 
> From a quick reading in Documentation/driver-model/devres.txt this
> devres muck is supposed to be device memory, right?

It's for associating resources (e.g. memory allocations) with a struct
device.

e.g. you do:

	devm_kmalloc(dev, size, GFP_KERNEL);

... and that allocates sizeof(struct devres) + size, putting some
accounting data into that devres, and returning a pointer to the
remaining size bytes.

The data[] thing is a hack to ensure that the structure is padded to
64-bit alignment, in case you'd done:

struct foo {
	atomic64_t counter;
}

struct foo *f = devm_kmalloc(dev, sizeof(*f), GFP_KERNEL);

Mark.
Peter Zijlstra July 9, 2018, 2:07 p.m. | #3
On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote:
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -24,8 +24,12 @@ struct devres_node {
>  
>  struct devres {
>  	struct devres_node		node;
> -	/* -- 3 pointers */
> -	unsigned long long		data[];	/* guarantee ull alignment */
> +	/*
> +	 * data[] must be 64 bit aligned even on 32 bit architectures
> +	 * because it might be accessed by instructions that require
> +	 * aligned memory arguments such as atomic64_t.
> +	 */
> +	u8 __aligned(8)			data[];
>  };

Seeing that this ends up in a semi generic allocation thing, I don't
feel this should be different from ARCH_KMALLOC_MINALIGN.
Alexey Brodkin July 9, 2018, 2:07 p.m. | #4
Hi Peter,

On Mon, 2018-07-09 at 15:54 +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote:
> > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > index f98a097e73f2..d65327cb83c9 100644
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -24,8 +24,12 @@ struct devres_node {
> >  
> >  struct devres {
> >  	struct devres_node		node;
> > -	/* -- 3 pointers */
> > -	unsigned long long		data[];	/* guarantee ull alignment */
> > +	/*
> > +	 * data[] must be 64 bit aligned even on 32 bit architectures
> > +	 * because it might be accessed by instructions that require
> > +	 * aligned memory arguments such as atomic64_t.
> > +	 */
> > +	u8 __aligned(8)			data[];
> >  };
> 
> From a quick reading in Documentation/driver-model/devres.txt this
> devres muck is supposed to be device memory, right?
> 
> atomics (as in atomic*_t) are not defined for use on mmio.
> 
> wth kind of code is relying on this?

It's Etnaviv (GPU/DRM) driver in etnaviv_gpu_platform_probe(),
see https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/etnaviv/etnaviv_gpu.c#L1740:
---------------------->8---------------------
	struct drm_gpu_scheduler {
		...
		atomic64_t			job_id_count;
		...
	};

	struct etnaviv_gpu {
		...
		struct drm_gpu_scheduler	sched;
	};

	struct etnaviv_gpu *gpu;

	gpu = devm_kzalloc(dev, sizeof(*gpu), GFP_KERNEL);
---------------------->8---------------------

-Alexey
Geert Uytterhoeven July 9, 2018, 2:10 p.m. | #5
On Mon, Jul 9, 2018 at 4:04 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Jul 09, 2018 at 03:54:09PM +0200, Peter Zijlstra wrote:
> > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote:
> > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > > index f98a097e73f2..d65327cb83c9 100644
> > > --- a/drivers/base/devres.c
> > > +++ b/drivers/base/devres.c
> > > @@ -24,8 +24,12 @@ struct devres_node {
> > >
> > >  struct devres {
> > >     struct devres_node              node;
> > > -   /* -- 3 pointers */
> > > -   unsigned long long              data[]; /* guarantee ull alignment */
> > > +   /*
> > > +    * data[] must be 64 bit aligned even on 32 bit architectures
> > > +    * because it might be accessed by instructions that require
> > > +    * aligned memory arguments such as atomic64_t.
> > > +    */
> > > +   u8 __aligned(8)                 data[];
> > >  };
> >
> > From a quick reading in Documentation/driver-model/devres.txt this
> > devres muck is supposed to be device memory, right?
>
> It's for associating resources (e.g. memory allocations) with a struct
> device.
>
> e.g. you do:
>
>         devm_kmalloc(dev, size, GFP_KERNEL);
>
> ... and that allocates sizeof(struct devres) + size, putting some
> accounting data into that devres, and returning a pointer to the
> remaining size bytes.
>
> The data[] thing is a hack to ensure that the structure is padded to
> 64-bit alignment, in case you'd done:
>
> struct foo {
>         atomic64_t counter;
> }
>
> struct foo *f = devm_kmalloc(dev, sizeof(*f), GFP_KERNEL);

So the big issue is that the minimum alignment of a buffer allocated with
devm_kmalloc() and friends is different (lower) than when allocated with
kmalloc().

On 32-bit, it's only aligned to 4 bytes. Ugh.
I wouldn't be surprised if some callers assume it to be cacheline-aligned...

Which means blind conversions to the devm_*() versions can be dangerous.

Gr{oetje,eeting}s,

                        Geert
Peter Zijlstra July 9, 2018, 2:10 p.m. | #6
On Mon, Jul 09, 2018 at 04:07:17PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote:
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -24,8 +24,12 @@ struct devres_node {
> >  
> >  struct devres {
> >  	struct devres_node		node;
> > -	/* -- 3 pointers */
> > -	unsigned long long		data[];	/* guarantee ull alignment */
> > +	/*
> > +	 * data[] must be 64 bit aligned even on 32 bit architectures
> > +	 * because it might be accessed by instructions that require
> > +	 * aligned memory arguments such as atomic64_t.
> > +	 */
> > +	u8 __aligned(8)			data[];
> >  };
> 
> Seeing that this ends up in a semi generic allocation thing, I don't
> feel this should be different from ARCH_KMALLOC_MINALIGN.

In fact, since alloc_dr() uses kmalloc() to allocate the entire thing,
it is impossible to guarantee a larger alignment than kmalloc does.
Alexey Brodkin July 9, 2018, 2:33 p.m. | #7
Hi Peter,

On Mon, 2018-07-09 at 16:10 +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 04:07:17PM +0200, Peter Zijlstra wrote:
> > On Mon, Jul 09, 2018 at 04:45:50PM +0300, Alexey Brodkin wrote:
> > > --- a/drivers/base/devres.c
> > > +++ b/drivers/base/devres.c
> > > @@ -24,8 +24,12 @@ struct devres_node {
> > >  
> > >  struct devres {
> > >  	struct devres_node		node;
> > > -	/* -- 3 pointers */
> > > -	unsigned long long		data[];	/* guarantee ull alignment */
> > > +	/*
> > > +	 * data[] must be 64 bit aligned even on 32 bit architectures
> > > +	 * because it might be accessed by instructions that require
> > > +	 * aligned memory arguments such as atomic64_t.
> > > +	 */
> > > +	u8 __aligned(8)			data[];
> > >  };
> > 
> > Seeing that this ends up in a semi generic allocation thing, I don't
> > feel this should be different from ARCH_KMALLOC_MINALIGN.
> 
> In fact, since alloc_dr() uses kmalloc() to allocate the entire thing,
> it is impossible to guarantee a larger alignment than kmalloc does.

Well but 4-bytes [which is critical for atomic64_t] should be much less
than a sane cache line length so above should work.

-Alexey
Peter Zijlstra July 9, 2018, 2:49 p.m. | #8
On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote:
> > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing,
> > it is impossible to guarantee a larger alignment than kmalloc does.
> 
> Well but 4-bytes [which is critical for atomic64_t] should be much less
> than a sane cache line length so above should work.

AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't
define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the
thing).

So unconditionally setting the alignment of devres::data to 8 seems
broken.
Alexey Brodkin July 9, 2018, 2:53 p.m. | #9
Hi Peter,

On Mon, 2018-07-09 at 16:49 +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote:
> > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing,
> > > it is impossible to guarantee a larger alignment than kmalloc does.
> > 
> > Well but 4-bytes [which is critical for atomic64_t] should be much less
> > than a sane cache line length so above should work.
> 
> AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't
> define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the
> thing).
> 
> So unconditionally setting the alignment of devres::data to 8 seems
> broken.

Well but then what other options do we have to fix a problem with
misaligned access to atomic64_t in drm_gpu_scheduler?

-Alexey
David Laight July 9, 2018, 3:02 p.m. | #10
From: Peter Zijlstra
> Sent: 09 July 2018 15:49
> On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote:
> > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing,
> > > it is impossible to guarantee a larger alignment than kmalloc does.
> >
> > Well but 4-bytes [which is critical for atomic64_t] should be much less
> > than a sane cache line length so above should work.
> 
> AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't
> define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the
> thing).

That seems broken.

I wonder what the minimal alignment really is?
I suspect some code expects (and gets) 8-byte alignment.
The min alignment might even be 16 or 32 bytes.
There aren't many x86 instructions that fault on mis-aligned addresses,
but there are a few.
Mostly related to the fpu - probably including the fpu save area.

	David
Peter Zijlstra July 9, 2018, 3:14 p.m. | #11
On Mon, Jul 09, 2018 at 03:02:08PM +0000, David Laight wrote:

> Mostly related to the fpu - probably including the fpu save area.

So for the FPU save area in particular I know we play some horrific
games. As to the rest I really dont know.

I wouldn't mind it being changed, but from a cursory look, I couldn't
see it being anything other than 4 for x86_32.
Mark Rutland July 9, 2018, 3:29 p.m. | #12
On Mon, Jul 09, 2018 at 04:49:25PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 02:33:26PM +0000, Alexey Brodkin wrote:
> > > In fact, since alloc_dr() uses kmalloc() to allocate the entire thing,
> > > it is impossible to guarantee a larger alignment than kmalloc does.
> > 
> > Well but 4-bytes [which is critical for atomic64_t] should be much less
> > than a sane cache line length so above should work.
> 
> AFAICT ARCH_KMALLOC_MINALIGN ends up being 4 on x86_32 (it doesn't
> define ARCH_DMA_MINALIGN and doesn't seem to otherwise override the
> thing).

Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on
x86_32:

----
[mark@lakrids:~]% cat test.c     
#include <stdio.h>

#define PRINT_TYPE_INFO(t) \
        printf("%10s %5d %5d\n", #t, sizeof(t), __alignof__(t))

int main(int argc, char *argv[])
{
        printf("%10s %5s %5s\n", "TYPE", "SIZE", "ALIGN");
        PRINT_TYPE_INFO(int);
        PRINT_TYPE_INFO(long);
        PRINT_TYPE_INFO(long long);

        return 0;
}
[mark@lakrids:~]% gcc -m32 test.c -o test
[mark@lakrids:~]% ./test 
      TYPE  SIZE ALIGN
       int     4     4
      long     4     4
 long long     8     8
----

Mark.
Peter Zijlstra July 9, 2018, 3:34 p.m. | #13
On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote:
> Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on
> x86_32:

Curious, I wonder why we put that align in atomic64_32 then.
Peter Zijlstra July 9, 2018, 3:45 p.m. | #14
On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote:
> > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on
> > x86_32:
> 
> Curious, I wonder why we put that align in atomic64_32 then.

Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188
Mark Rutland July 9, 2018, 3:48 p.m. | #15
On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote:
> > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote:
> > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on
> > > x86_32:
> > 
> > Curious, I wonder why we put that align in atomic64_32 then.
> 
> Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188
> 

Ouch.

[mark@lakrids:~]% cat test.c
#include <stdio.h>

#define PRINT_TYPE_INFO(t) \
        printf("%10s %5d %5d\n", #t, sizeof(t), __alignof__(t))

struct ull {
        unsigned long long v;
};

int main(int argc, char *argv[])
{
        printf("%10s %5s %5s\n", "TYPE", "SIZE", "ALIGN");
        PRINT_TYPE_INFO(int);
        PRINT_TYPE_INFO(long);
        PRINT_TYPE_INFO(long long);
        PRINT_TYPE_INFO(struct ull);

        return 0;
}
[mark@lakrids:~]% gcc -m32 test.c -o test
[mark@lakrids:~]% ./test 
      TYPE  SIZE ALIGN
       int     4     4
      long     4     4
 long long     8     8
struct ull     8     4

Mark.
David Laight July 9, 2018, 4:10 p.m. | #16
From: Mark Rutland
> Sent: 09 July 2018 16:49
> 
> On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote:
> > On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote:
> > > On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote:
> > > > Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on
> > > > x86_32:
> > >
> > > Curious, I wonder why we put that align in atomic64_32 then.
> >
> > Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188
> >
> 
> Ouch.

Indeed.

changing the definition to:
struct ull {
        unsigned long long v __attribute__((aligned(__alignof__(long long))));
};

prints 8 for the structure alignment.

Time to audit uses of __alignof__().

#define actual_alignof(type) __alignof__(struct { type jsdjdhjdjh; })

	David
Vineet Gupta July 9, 2018, 8:28 p.m. | #17
On 07/09/2018 09:10 AM, David Laight wrote:
> From: Mark Rutland
>> Sent: 09 July 2018 16:49
>>
>> On Mon, Jul 09, 2018 at 05:45:21PM +0200, Peter Zijlstra wrote:
>>> On Mon, Jul 09, 2018 at 05:34:27PM +0200, Peter Zijlstra wrote:
>>>> On Mon, Jul 09, 2018 at 04:29:58PM +0100, Mark Rutland wrote:
>>>>> Shouldn't that be 8? AFAICT, __alignof__(unsigned long long) is 8 on
>>>>> x86_32:
>>>>
>>>> Curious, I wonder why we put that align in atomic64_32 then.
>>>
>>> Shiny, look at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54188
>>>
>>
>> Ouch.
> 
> Indeed.
> 
> changing the definition to:
> struct ull {
>         unsigned long long v __attribute__((aligned(__alignof__(long long))));
> };
> 
> prints 8 for the structure alignment.

So this will help force alignment of outer structures triggered by inner members,
but only for globals and perhaps those on stack. I'm not sure how this helps
aligning such  a struct allocated dynamically - we are not passing this extra
alignment info the the allocator API here. Surely the size will increase and we
end up with extra padding in the end as intended originally and pointed to by
Mark, but how does it help with alignment ? What am I missing ?

> 
> Time to audit uses of __alignof__().
> 
> #define actual_alignof(type) __alignof__(struct { type jsdjdhjdjh; })

Patch

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index f98a097e73f2..d65327cb83c9 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -24,8 +24,12 @@  struct devres_node {
 
 struct devres {
 	struct devres_node		node;
-	/* -- 3 pointers */
-	unsigned long long		data[];	/* guarantee ull alignment */
+	/*
+	 * data[] must be 64 bit aligned even on 32 bit architectures
+	 * because it might be accessed by instructions that require
+	 * aligned memory arguments such as atomic64_t.
+	 */
+	u8 __aligned(8)			data[];
 };
 
 struct devres_group {