[PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Thomas Meyer]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

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.

Signed-off-by: Thomas Meyer <thomas@m3y3r>
---
 include/linux/auto_fs4.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/auto_fs4.h b/include/linux/auto_fs4.h
index e02982f..4be222a 100644
--- a/include/linux/auto_fs4.h
+++ b/include/linux/auto_fs4.h
@@ -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;
-- 
1.7.6.2


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Al Viro]

On Fri, Sep 16, 2011 at 12:06:38PM +0200, Thomas Meyer wrote:
> 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.

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.

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Ian Kent]

On Fri, 2011-09-16 at 11:19 +0100, Al Viro wrote:
> On Fri, Sep 16, 2011 at 12:06:38PM +0200, Thomas Meyer wrote:
> > 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.
> 
> 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;
}

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Thomas Meyer]

Am Freitag, den 16.09.2011, 18:38 +0800 schrieb Ian Kent:
> On Fri, 2011-09-16 at 11:19 +0100, Al Viro wrote:
> > On Fri, Sep 16, 2011 at 12:06:38PM +0200, Thomas Meyer wrote:
> > > 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.
> > 
> > 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.
> 

btw. where are the padding bytes are added? how to tell gcc to display
this?

with kind regards
thomas

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Ian Kent]

On Sun, 2011-09-18 at 10:01 +0200, Thomas Meyer wrote:
> Am Freitag, den 16.09.2011, 18:38 +0800 schrieb Ian Kent:
> > On Fri, 2011-09-16 at 11:19 +0100, Al Viro wrote:
> > > On Fri, Sep 16, 2011 at 12:06:38PM +0200, Thomas Meyer wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> 
> 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

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Al Viro]

On Mon, Sep 19, 2011 at 11:52:48AM +0800, Ian Kent wrote:

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

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Thomas Meyer]

Am 19.09.2011 um 08:10 schrieb Al Viro:

> On Mon, Sep 19, 2011 at 11:52:48AM +0800, Ian Kent wrote:
> 
>>> 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.

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

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Al Viro]

On Mon, Sep 19, 2011 at 11:30:42AM +0200, Thomas Meyer wrote:

> @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"...

Re: [PATCH] Force same size of struct autofs_v5_packet on x86 and x86_64

[Ian Kent]

On Mon, 2011-09-19 at 11:30 +0200, Thomas Meyer wrote:
> Am 19.09.2011 um 08:10 schrieb Al Viro:
> 
> > On Mon, Sep 19, 2011 at 11:52:48AM +0800, Ian Kent wrote:
> > 
> >>> 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.
> 
> 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.

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

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.

> 
> @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