compat: autofs v5 packet size ambiguity - update

[Linus Torvalds]

Btw, thinking more about the autofs patch, I realized that despite it
all working well for Thomas in his case, it's fundamentally wrong.

And it's not fundamentally wrong because of any ambiguities about the
size of the structure: that structure is clearly 304 bytes on x86-64
(and most other platforms, buth 32-bit and 64-bit), but it's 300 bytes
on x86-32 and m68k.

No, the problem is that "is_compat_task()" is not the right check.
It's not the task that *waits* for autofs that matters, it's that damn
autofs daemon task.

IOW, what we actually want to test is whether the other end of that
autofs sbi->pipe is a compat task or not.

And I have no idea how to do that. Can I assume that whoever does the
original "mount()" system call is the daemon? It needs to have that
pipe somehow.. Is there something that the daemon does early on that
we can use to capture whether the daemon is a compat task or not?

Ian, Peter, anybody who knows autofs? Is perhaps one of the ioctl's
always done by the daemon, where we could then use "is_compat_task()"
at that point to figure out whether it is going to expect the 300-byte
packet or the 304-byte packet?

We could just initialize sbi->v5_packet_size to the plain sizeof(),
but when we see that ioctl and realize that the daemon is a x86-32
binary we'd reset the packet size to 300.

Anyway, here's the patch again with a long explanation, but with a
"THIS IS WRONG" comment in the code, and an explanation in the commit
log. It works for Thomas, but it works for the wrong reasons - in his
setup, all binaries are compat binaries, so "is_compat_task()" just
happens to get the right value for the daemon too. But if you have a
mixture of binaries, you might get the autofs *request* in a compat
binary while the daemon is a 64-bit native x86-64 binary, or the other
way around, and then this patch would use the wrong packet size to
communicate with the daemon.

Hmm?

                     Linus
From bca793d60d07d79501a83b4e83a856c3bdbfd535 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Tue, 21 Feb 2012 17:36:06 -0800
Subject: [PATCH] autofs: work around unhappy compat problem on x86-64

When the autofs protocol version 5 packet type was added in commit
5c0a32fc2cd0 ("autofs4: add new packet type for v5 communications"), it
obvously tried quite hard to be word-size agnostic, and uses explicitly
sized fields that are all correctly aligned.

However, with the final "char name[NAME_MAX+1]" array at the end, the
actual size of the structure ends up being not very well defined:
because the struct isn't marked 'packed', doing a "sizeof()" on it will
align the size of the struct up to the biggest alignment of the members
it has.

And despite all the members being the same, the alignment of them is
different: a "__u64" has 4-byte alignment on x86-32, but native 8-byte
alignment on x86-64.  And while 'NAME_MAX+1' ends up being a nice round
number (256), the name[] array starts out a 4-byte aligned.

End result: the "packed" size of the structure is 300 bytes: 4-byte, but
not 8-byte aligned.

As a result, despite all the fields being in the same place on all
architectures, sizeof() will round up that size to 304 bytes on
architectures that have 8-byte alignment for u64.

Note that this is *not* a problem for 32-bit compat mode on POWER, since
there __u64 is 8-byte aligned even in 32-bit mode.  But on x86, 32-bit
and 64-bit alignment is different for 64-bit entities, and as a result
the structure that has exactly the same layout has different sizes.

So on x86-64, but no other architecture, we will just subtract 4 from
the size of the structure when running in a compat task.  That way we
will write the properly sized packet that user mode expects.

Not pretty.  Sadly, this very subtle, and unnecessary, size difference
has been encoded in user space that wants to read packets of *exactly*
the right size, and will refuse to touch anything else.

And the sad part is that this patch is *wrong*: we need to know whether
the autofs *daemon* is a compat task, now whether the *current* task is
a compat task.

Reported-and-tested-by: Thomas Meyer <thomas@xxxxxxxx>
Cc: Ian Kent <raven@xxxxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
---
 fs/autofs4/waitq.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index da8876d38a7b..d4b911909869 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -15,6 +15,7 @@
 #include <linux/time.h>
 #include <linux/signal.h>
 #include <linux/file.h>
+#include <linux/compat.h>
 #include "autofs_i.h"
 
 /* We make this a static variable rather than a part of the superblock; it
@@ -91,6 +92,24 @@ static int autofs4_write(struct autofs_sb_info *sbi,
 
 	return (bytes > 0);
 }
+
+/*
+ * The autofs_v5 packet was misdesigned.
+ *
+ * The packets are identical on x86-32 and x86-64, but have different
+ * alignment. Which means that 'sizeof()' will give different results.
+ * Fix it up for the case of running 32-bit user mode on a 64-bit kernel.
+ */
+static noinline size_t autofs_v5_packet_size(void)
+{
+	size_t pktsz = sizeof(struct autofs_v5_packet);
+#if defined(CONFIG_X86_64) && defined(CONFIG_COMPAT)
+	/* THIS IS WRONG - WE SHOULD CHECK IF THE *DEAMON* IS A COMPAT TASK! */
+	if (is_compat_task())
+		pktsz -= 4;
+#endif
+	return pktsz;
+}
 	
 static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 				 struct autofs_wait_queue *wq,
@@ -155,8 +174,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 	{
 		struct autofs_v5_packet *packet = &pkt.v5_pkt.v5_packet;
 
-		pktsz = sizeof(*packet);
-
+		pktsz = autofs_v5_packet_size();
 		packet->wait_queue_token = wq->wait_queue_token;
 		packet->len = wq->name.len;
 		memcpy(packet->name, wq->name.name, wq->name.len);
-- 
1.7.9.188.g12766.dirty