ABI violations eventually caused by short sighted coding style

ABI violations eventually caused by short sighted coding style

[Jon Pry]

I have recently been bitten by an ABI change. I'd prefer not to name names, but
it seems at this point the ABI cannot even be reverted. This sequence of events
results from the following strategy to deal with ioctl's using structures and
attempting to maintain backward compatibility.

Take for example the following:

foo_ioctl(struct *myioctldata);

The creator of this structure knows that the driver may have it's features
extended in the future. So they invent this:

struct foodata{
	u32 foo;
};

struct myioctldata{
	u32 type;
	union {
		struct foodata;
		u8 data[64];
	} u;
};


The creator is proud. Because now this ioctl is future proof. More structures
can simply be added in the future without breaking existing code. Right? Wrong.
If a new structure is added to the union that requires higher alignment. Like
adding a 64-bit element. The union will now be moved in the structure and
sizeof(myioctldata) will now be different to accommodate the padding.

This is even cooler if myioctldata.type is a u8, then it is possible to get 4
whole different sized structs depending on the contents of the union. I find
this to be rather stupid. The kernel is littered with this stuff so it's
probably impossible to remove it. At least new code should not be encouraged to
follow this form. Most developers just don't know and assume if the struct is
small enough, it will not cause any problems. To make matters worse,
the effects
can be different on 32/64bit platforms, allowing the code to pass testing.

I believe changes should be made to the coding style forcing developers to take
action. There are a couple of solutions I can think of:

1. 	Put the unions at the beginning of structures if possible, or at least put
	them in a position that would ensure 8 byte offset from the beginning of
	the struct. Such as after a u64. Multiple unions should ensure the data[]
	of the preceding unions to be a multiple of 8 bytes.
2.  Require the data element to be declared as u64[size/8].
3.  Pack the ioctl structures

My preference is for the former since it does not cause any
unnecessary padding.

Regards,

Jon Pry

Re: ABI violations eventually caused by short sighted coding style

[H. Peter Anvin]

On 01/31/2013 02:20 PM, Jon Pry wrote:
> 
> I believe changes should be made to the coding style forcing developers to take
> action. There are a couple of solutions I can think of:
> 
> 1. 	Put the unions at the beginning of structures if possible, or at least put
> 	them in a position that would ensure 8 byte offset from the beginning of
> 	the struct. Such as after a u64. Multiple unions should ensure the data[]
> 	of the preceding unions to be a multiple of 8 bytes.
> 2.  Require the data element to be declared as u64[size/8].
> 3.  Pack the ioctl structures
> 
> My preference is for the former since it does not cause any
> unnecessary padding.
> 

"The former" doesn't work when you are talking about a choice of three
elements.

packed structures are *very* expensive to use on some architectures, so
that is not a good solution.

In many, if not most cases, this is actually better handled by having
different ioctl numbers for each version/interface, but if you have to
make it self-contained, it makes most sense to make the common portion a
u64 or explicitly align the union to u64.

Another thing to keep in mind is that it often makes a lot more sense to
have a length member, and skip the problematic union.  That way a future
version of the kernel can simply memset() the part of the structure that
isn't initialized, and doesn't need a huge apparatus to keep track of
each individual version.

	-hpa