On 2019-12-16, David Laight David.Laight@ACULAB.COM wrote:
From: Aleksa Sarai
Sent: 15 December 2019 12:35 On 2019-12-14, Rasmus Villemoes linux@rasmusvillemoes.dk wrote:
On 13/12/2019 23.23, Aleksa Sarai wrote:
The design of the original open_how struct layout was such that it ensured that there would be no un-labelled (and thus potentially non-zero) padding to avoid issues with struct expansion, as well as providing a uniform representation on all architectures (to avoid complications with OPEN_HOW_SIZE versioning).
However, there were a few other desirable features which were not fulfilled by the previous struct layout:
Adding new features (other than new flags) should always result in the struct getting larger. However, by including a padding field, it was possible for new fields to be added without expanding the structure. This would somewhat complicate version-number based checking of feature support.
A non-zero bit in __padding yielded -EINVAL when it should arguably have been -E2BIG (because the padding bits are effectively yet-to-be-used fields). However, the semantics are not entirely clear because userspace may expect -E2BIG to only signify that the structure is too big. It's much simpler to just provide the guarantee that new fields will always result in a struct size increase, and -E2BIG indicates you're using a field that's too recent for an older kernel.
And when the first extension adds another u64 field, that padding has to be added back in and checked for being 0, at which point the padding is again yet-to-be-used fields.
Maybe I'm missing something, but what is the issue with
struct open_how { u64 flags; u64 resolve; u16 mode; u64 next_extension; } __attribute__((packed));
Compile anything that accesses it for (say) sparc and look at the object code. You really, really, REALLY, don't want to EVER use 'packed'.
Right, so it's related to the "garbage code" problem. As mentioned above, I wasn't aware it was as bad as folks in this thread have mentioned.
Just use u64 for all the fields.
That is an option (and is the one that clone3 went with), but it's a bit awkward because umode_t is a u16 -- and it would be a waste of 6 bytes to store it as a u64. Arguably it could be extended but I personally find that to be very unlikely (and lots of other syscalls would need be updated).
I'm just going to move the padding to the end and change the error for non-zero padding to -E2BIG.
Use 'flags' bits to indicate whether the additional fields should be looked at. Error if a 'flags' bit requires a value that isn't passed in the structure.
Then you can add an extra field and old source code recompiled with the new headers will still work - because the 'junk' value isn't looked at.
This problem is already handled entirely by copy_struct_from_user().
It is true that for some new fields it will be necessary to add a new flag (such as passing fds -- where 0 is a valid value) but for most new fields (especially pointer or flag fields) it will not be necessary because the 0 value is equivalent to the old behaviour. It also allows us to entirely avoid accepting junk from userspace.