MTD pointer alignment

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).

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

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

Quoth Artem Bityutskiy [[email protected]]:

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.

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

Quoth David Woodhouse [[email protected]]:

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 :)