Force same size of struct autofs_v5_packet on x86 and x86_64
PATCH 1/1 Force same size of struct autofs_v5_packet on x86 and x86_64
autofs_v5_packet is 300 bytes on x86 and 304 bytes on x86_64. This difference is wrong and leads to a hang in systemd when running a x86 userspace on an x86_64 kernel.
--- include/linux/auto_fs4.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/auto_fs4.h +1 -1
@@ -138,7 +138,7 @@ struct autofs_v5_packet {__u32 tgid;__u32 len;char name[NAME_MAX +1 ]; -}; +} __attribute__ ((packed));typedef struct autofs_v5_packet autofs_packet_missing_indirect_t ;typedef struct autofs_v5_packet autofs_packet_expire_indirect_t ;
Signature · 1 line
1.7.6.2
Al Viro
NAKQuoted 3 lines · Thomas Meyer
autofs_v5_packetis 300 bytes on x86 and 304 bytes onx86_64. This difference is wrong and leads to a hang in systemd when running a x86 userspace on anx86_64kernel.
NAK.
You are talking about the userland ABI. With existing users. Changing it might help systemd (what the hell is it doing with autofs, anyway?), but breaking existing binaries (you know, ones that *do* have some business dealing with autofs) is not acceptable.
Quoted 11 lines · Al Viro
Quoted 3 lines · Thomas Meyer
autofs_v5_packetis 300 bytes on x86 and 304 bytes onx86_64. This difference is wrong and leads to a hang in systemd when running a x86 userspace on anx86_64kernel.NAK.
You are talking about the userland ABI. With existing users. Changing it might help systemd (what the hell is it doing with autofs, anyway?), but breaking existing binaries (you know, ones that *do* have some business dealing with autofs) is not acceptable.
That's right.
Certainly my mistake (made a long time ago) and when I realized it I decided I would need to handle it in user space for the same reasons Al has mentioned.
I think this should be enough (but any further recommendations are welcome, apart from the fact we might get new archs):
static size_t get_kpkt_len(void)
{
size_t pkt_len = sizeof(struct autofs_v5_packet);
struct utsname un;
uname(&un);
if (pkt_len % 8) {
if (strcmp(un.machine, "alpha") == 0 ||
strcmp(un.machine, "ia64") == 0 ||
strcmp(un.machine, "x86_64") == 0 ||
strcmp(un.machine, "ppc64") == 0)
pkt_len += 4;
}
return pkt_len;
}Am Freitag, den 16.09.2011, 18:38 +0800 schrieb Ian Kent:
Quoted 13 lines · Al Viro
Quoted 11 lines · Thomas Meyer
Quoted 3 lines
autofs_v5_packetis 300 bytes on x86 and 304 bytes onx86_64. This difference is wrong and leads to a hang in systemd when running a x86 userspace on anx86_64kernel.NAK.
You are talking about the userland ABI. With existing users. Changing it might help systemd (what the hell is it doing with autofs, anyway?), but breaking existing binaries (you know, ones that *do* have some business dealing with autofs) is not acceptable.
Quoted 6 lines
That's right.
Certainly my mistake (made a long time ago) and when I realized it I decided I would need to handle it in user space for the same reasons Al has mentioned.
btw. where are the padding bytes are added? how to tell gcc to display this?
with kind regards
thomas
Quoted 16 lines · Thomas Meyer
Am Freitag, den 16.09.2011, 18:38 +0800 schrieb Ian Kent:
Quoted 15 lines · Al Viro
Quoted 11 lines · Thomas Meyer
Quoted 3 lines
autofs_v5_packetis 300 bytes on x86 and 304 bytes onx86_64. This difference is wrong and leads to a hang in systemd when running a x86 userspace on anx86_64kernel.NAK.
You are talking about the userland ABI. With existing users. Changing it might help systemd (what the hell is it doing with autofs, anyway?), but breaking existing binaries (you know, ones that *do* have some business dealing with autofs) is not acceptable.
That's right.
Quoted 7 lines
Quoted 4 lines
Certainly my mistake (made a long time ago) and when I realized it I decided I would need to handle it in user space for the same reasons Al has mentioned.
btw. where are the padding bytes are added? how to tell gcc to display this?
It's also been a long time since I looked into this so I don't remember the processor alignment details.
Not sure if there is a compiler option that would report this, sorry, anyone else?
Ian
Quoted 8 lines · Ian Kent
Quoted 2 lines
btw. where are the padding bytes are added? how to tell gcc to display this?
It's also been a long time since I looked into this so I don't remember the processor alignment details.
Not sure if there is a compiler option that would report this, sorry, anyone else?
Padding is added in the very end. The thing is, it's not a misaligned field per se; they all have a natural alignment in there. It's that __u64 ino has alignment 8 on amd64 and alignment 4 on i386. So the alignment requirements for the entire structure are different - on amd64 the address of the entire thing must be a multiple of 8 and on i386 it can be any multiple of 4. IOW, on i386 you can just put them one after another in e.g. an array (the size without any padding is 300 bytes) and on amd64 you need 4 bytes of padding added between them. I.e. sizeof needs to go up by 4, with the padding added in the end.
Am 19.09.2011 um 08:10 schrieb Al Viro:
Quoted 11 lines · Ian Kent
Quoted 9 lines
Quoted 3 lines
btw. where are the padding bytes are added? how to tell gcc to display this?
It's also been a long time since I looked into this so I don't remember the processor alignment details.
Not sure if there is a compiler option that would report this, sorry, anyone else?
Quoted 9 lines
Padding is added in the very end. The thing is, it's not a misaligned field per se; they all have a natural alignment in there. It's that
__u64ino has alignment 8 on amd64 and alignment 4 on i386. So the alignment requirements for the entire structure are different - on amd64 the address of the entire thing must be a multiple of 8 and on i386 it can be any multiple of 4. IOW, on i386 you can just put them one after another in e.g. an array (the size without any padding is 300 bytes) and on amd64 you need 4 bytes of padding added between them. I.e. sizeof needs to go up by 4, with the padding added in the end.
okay. that's what I thought.
@Ian: Is the read() from the pipe fd the only way to consume this structure from userspace?
if so: why not align the size of this structure for x86 and x86_64 to the same size?
the systemd compiled for x86_64 will probably work with this change, but does maybe report something like a short read or so, as it currently expects 304 bytes to read. i didn't test this yet.
@al: what other autofs4 users are out there besides systemd?
with kind regards
thomas
Quoted 1 line · Thomas Meyer
@al: what other autofs4 users are out there besides systemd?
Are you serious? You *do* realize that autofs is quite usable on
the benighted systems not infested with that... Fabulous Piece Of Software, don't you? Depending on your distribution, say apt-cache search autofs / yum search autofs / whatever your equivalent is and examine the results. Plus anything that has demonstrated the lack of taste equal to yours and reimplemented that stuff, of course.
Sigh... At least DJB fanboys usually do not ask "what other SMTP users are out there besides qmail"...
Quoted 23 lines · Thomas Meyer
Am 19.09.2011 um 08:10 schrieb Al Viro:
Quoted 11 lines · Ian Kent
Quoted 9 lines
Quoted 3 lines
btw. where are the padding bytes are added? how to tell gcc to display this?
It's also been a long time since I looked into this so I don't remember the processor alignment details.
Not sure if there is a compiler option that would report this, sorry, anyone else?
Quoted 9 lines
Padding is added in the very end. The thing is, it's not a misaligned field per se; they all have a natural alignment in there. It's that
__u64ino has alignment 8 on amd64 and alignment 4 on i386. So the alignment requirements for the entire structure are different - on amd64 the address of the entire thing must be a multiple of 8 and on i386 it can be any multiple of 4. IOW, on i386 you can just put them one after another in e.g. an array (the size without any padding is 300 bytes) and on amd64 you need 4 bytes of padding added between them. I.e. sizeof needs to go up by 4, with the padding added in the end.
Quoted 3 lines
okay. that's what I thought.
@Ian: Is the
read()from the pipe fd the only way to consume this structure from userspace?
No, a pipe has been the way it has been done since before I started maintaining the autofs code. I did try to start moving toward using netlink but found that it wouldn't work without a major re-write of the daemon which I decided not to do. At that time netlink wouldn't have included the mount callback but, if it had worked, would have eventually done so. So, until I decide to do such a huge re-write we are stuck with this.
Quoted 7 lines
if so: why not align the size of this structure for x86 and
x86_64to the same size?the systemd compiled for
x86_64will probably work with this change, but does maybe report something like a short read or so, as it currently expects 304 bytes to read.
i didn't test this yet.
What do you mean?
This problem is only evident where the compiled binary arch is different from the kernel compiled arch. Like when you run i386 (or i686) binaries on an x86_64 arch with an x86_64 install or ppc binaries on a ppc64 install. The common case of using like binaries on like arch works fine as far as I know.
Quoted 2 lines
@al: what other autofs4 users are out there besides systemd?
autofs and autodir as far as I know but then systemd started using the direct mount feature of the kernel autofs module and no-one asked me for advice so I didn't know about it for a long time. Maybe if someone did we could have had this conversation at a more appropriate time.
Ian