lkml.org 
[lkml]   [2012]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subjectcompat: autofs v5 packet size ambiguity - update
    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@linux-foundation.org>
    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@m3y3r.de>
    Cc: Ian Kent <raven@themaw.net>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    ---
    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
    \
     
     \ /
      Last update: 2012-02-22 03:27    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean