MTD pointer alignment

MTD pointer alignment

[Gavin Lambert]

Given that mtdchar assumes that it can mess with the low 2 bits of the
MTD pointer it keeps around with impunity, add_mtd_device in mtdcore.c
should probably refuse to add any MTD device pointer that doesn't fall
on a 4-byte boundary.

As of kernel 2.6.15, at least, it happily adds it, leading to much
weirdness if the pointer wasn't actually aligned that way (which can
happen if it's embedded in a structure -- since struct mtd_info starts
with a byte-sized field then by default the structure as a whole is
allowed to be byte aligned).

Re: MTD pointer alignment

[Artem Bityutskiy]

On Wed, 2006-11-29 at 13:09 +1300, Gavin Lambert wrote:
> Given that mtdchar assumes that it can mess with the low 2 bits of the
> MTD pointer it keeps around with impunity, add_mtd_device in mtdcore.c
> should probably refuse to add any MTD device pointer that doesn't fall
> on a 4-byte boundary.

Hmm, I personally I do not understand what is this about.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

RE: MTD pointer alignment

[Gavin Lambert]

Quoth Artem Bityutskiy [dedekind@infradead.org]:
> On Wed, 2006-11-29 at 13:09 +1300, Gavin Lambert wrote:
>> Given that mtdchar assumes that it can mess with the low 2 bits of
>> the MTD pointer it keeps around with impunity, add_mtd_device in
>> mtdcore.c should probably refuse to add any MTD device pointer that
>> doesn't fall on a 4-byte boundary.
> 
> Hmm, I personally I do not understand what is this about.

In mtdchar.c, when an MTD character device is opened (mtd_open), its
pointer value is assigned to file->private_data.  All subsequent
operations then use the TO_MTD macro, which is defined as:

#define TO_MTD(file) (struct mtd_info *)((long)((file)->private_data) &
~3L)

(ie. masking off the lower two bits).  There's even a comment above that
those bits have been hijacked for OTP modes, and it's expecting that
alignment of the pointer is at least 32 bits.

While this is true for a standalone kalloc'd MTD structure, if the MTD
structure is embedded within another structure then it is not
necessarily the case.  Since struct mtd_info starts with a byte-sized
field, the default padding rules say that the structure as a whole is
allowed to start on a byte boundary (although on many arches it'll get
16-bit aligned regardless).  Hence the 32-bit alignment is most
definitely *not* guaranteed.

If you're going to hijack the bits then you should at the very least
test your assertion in either mtd_open or add_mtd_device, by making it
reject MTD devices that aren't aligned as you'd expect.  Otherwise it
leads to a whole raft of weird bugs.

As I said before, though, I'm looking at the MTD sources as of kernel
2.6.15.  It's possible this has been sorted out since then.  If so, I
apologise.

RE: MTD pointer alignment

[David Woodhouse]

On Mon, 2006-12-04 at 10:43 +1300, Gavin Lambert wrote:
> Quoth Artem Bityutskiy [dedekind@infradead.org]:
> > On Wed, 2006-11-29 at 13:09 +1300, Gavin Lambert wrote:
> >> Given that mtdchar assumes that it can mess with the low 2 bits of
> >> the MTD pointer it keeps around with impunity, add_mtd_device in
> >> mtdcore.c should probably refuse to add any MTD device pointer that
> >> doesn't fall on a 4-byte boundary.
> > 
> > Hmm, I personally I do not understand what is this about.
> 
> In mtdchar.c, when an MTD character device is opened (mtd_open), its
> pointer value is assigned to file->private_data.  All subsequent
> operations then use the TO_MTD macro, which is defined as:
> 
> #define TO_MTD(file) (struct mtd_info *)((long)((file)->private_data) &
> ~3L)
> 
> (ie. masking off the lower two bits).  There's even a comment above that
> those bits have been hijacked for OTP modes, and it's expecting that
> alignment of the pointer is at least 32 bits.
> 
> While this is true for a standalone kalloc'd MTD structure, if the MTD
> structure is embedded within another structure then it is not
> necessarily the case.  Since struct mtd_info starts with a byte-sized
> field, the default padding rules say that the structure as a whole is
> allowed to start on a byte boundary (although on many arches it'll get
> 16-bit aligned regardless).  Hence the 32-bit alignment is most
> definitely *not* guaranteed.

That's news to me. If we don't always align to sizeof(int) and have
appropriate (i.e. three bytes) padding after the initial 'char' field,
then surely the 'int' fields are sometimes going to be misaligned?

What platform is this? Precisely how is it padded/aligned?

-- 
dwmw2

RE: MTD pointer alignment

[Gavin Lambert]

Quoth David Woodhouse [dwmw2@infradead.org]:
> That's news to me. If we don't always align to sizeof(int)
> and have appropriate (i.e. three bytes) padding after the
> initial 'char' field, then surely the 'int' fields are
> sometimes going to be misaligned?
> 
> What platform is this? Precisely how is it padded/aligned?

The int fields do get their natural alignment (ie. three bytes padding
after the initial char).  But that initial char (and the structure as a
whole) is not necessarily aligned on a four-byte boundary.

This is on Coldfire, using m68k-elf-gcc 3.4.4.

Test case for making it go wrong was embedding it in a structure similar
to this:

struct some_device
{
    u8 a_byte_field;
    struct mtd_info mtd;
    struct device dev;
};

The compiler inserted one single byte of padding between the byte field
and the MTD structure, making the structure aligned on a 2-byte
boundary, not a 4-byte one.


However I've now looked at the latest MTD sources (in 2.6.18) and it
looks like this issue may be moot, since file->private_data is now
pointing at some other structure rather than pointing directly at the
mtd_info.  I didn't check where you're keeping your OTP mode info now,
but as long as you're not hijacking bits then it shouldn't be a problem
:)