lkml.org 
[lkml]   [2013]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectABI violations eventually caused by short sighted coding style
From
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


\
 
 \ /
  Last update: 2013-02-01 03:41    [W:0.031 / U:0.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site