LinuxLists.cc - Linux 3.3-rc4

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Sun, Feb 19, 2012 at 5:42 AM, Thomas Meyer <[email protected]> wrote:
>
> Theoretically yes, practically (at least with Fedora 15/16) no. I did hit these two bugs:
>
> 1.) autofs4 interface is broken between x86 and x86_64. as systemd uses autofs, this bug hangs the boot process as e.g. binfmt is mounted via autofs. see also http://lists.freedesktop.org/archives/systemd-devel/2011-September/003396.html

Duh.

That is just broken.

The code even *talks* about how the packet layout is the same on
32-bit and 64-bit architectures, and that's largely true.

However, while true, x86-64 has 8-byte alignment for 'long', and
x86-32 has 4-byte alignment. Which means that even though the
structure layout is exactly the same, on x86-64 the *alignment* issue
will push it out to 304 bytes.

That's just stupid. We've had that problem before. It's easy to
overlook, but that packet is just mis-designed.

The attached patch isn't pretty, but this is definitely a kernel bug.
Binary compatibility is *important*, dammit.

Does this fix it?

Sadly, the *right* fix would have been to just mark the structure
packed, or select a maximum name length that padded things out to 8
bytes on all architectures, but we can't change that any more, because
changing the size on native 32-bit would break binaries there.

> 2.) while debugging above issue: I did find an minor bug in sys_poll() - nobody did take care of my proposed patch: https://lkml.org/lkml/2011/9/24/35

Looks correct, and searching my lkml archives shows that it was acked
by Eric. But it never went any further.

Mind re-sending that patch updated for the x86 system call
re-organization? Sure, I could do it myself, but it would be much
better if somebody who then actually *tests* the end result did it
(hpa cc'd, just because he's the one that touched the x86 compat layer
system call table thing. "Tag, you're it")

Anyway, if that, together with the sizeof hack in the attached patch,
makes everything work for you, I'll happily apply both of them.

Linus

Attachments:

patch.diff (1.48 kB)

2012-02-19 19:50:17

by David Miller

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

From: Linus Torvalds <[email protected]>
Date: Sun, 19 Feb 2012 10:45:26 -0800

> However, while true, x86-64 has 8-byte alignment for 'long', and
> x86-32 has 4-byte alignment. Which means that even though the
> structure layout is exactly the same, on x86-64 the *alignment* issue
> will push it out to 304 bytes.
>
> That's just stupid. We've had that problem before. It's easy to
> overlook, but that packet is just mis-designed.

A real shame, this should have used "__aligned_u64" from the
beginning.

2012-02-19 20:02:29

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Sun, Feb 19, 2012 at 11:49 AM, David Miller <[email protected]> wrote:
>
> A real shame, this should have used "__aligned_u64" from the
> beginning.

I agree. Sadly, this is exactly the kind of thing that is *really*
easy to overlook, and once it is overlooked we're screwed because
fixing it just breaks the native 32-bit case.

We probably should have made __u64 itself be marked as aligned, but
that's too late now too, unless somebody wants to go through and fix
any cases like this ;(

Binary compatibility is really important, and while arguably
compat-compatibility is slightly less critical, I think we should aim
to DTRT there too. I don't think we need to necessarily bend over
quite as far backwards for the compat case, but in places like this
where we are so close to being compatible - and not being compatible
kills the boot sequence and isn't just some theoretical thing - I do
think that it's worth

Linus

2012-02-20 03:28:34

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

2012-02-21 03:29:41

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Sun, 2012-02-19 at 12:02 -0800, Linus Torvalds wrote:
> On Sun, Feb 19, 2012 at 11:49 AM, David Miller <[email protected]> wrote:
> >
> > A real shame, this should have used "__aligned_u64" from the
> > beginning.
>
> I agree. Sadly, this is exactly the kind of thing that is *really*
> easy to overlook, and once it is overlooked we're screwed because
> fixing it just breaks the native 32-bit case.
>
> We probably should have made __u64 itself be marked as aligned, but
> that's too late now too, unless somebody wants to go through and fix
> any cases like this ;(
>
> Binary compatibility is really important, and while arguably
> compat-compatibility is slightly less critical, I think we should aim
> to DTRT there too. I don't think we need to necessarily bend over
> quite as far backwards for the compat case, but in places like this
> where we are so close to being compatible - and not being compatible
> kills the boot sequence and isn't just some theoretical thing - I do
> think that it's worth

Not sure this is the way you'd like to go with this but here is a patch
that bumps the major autofs kernel communications version (as yet not
even compile tested).

The advantage of this is that users of the v5 protocol that use a
workaround like the one in list archive post (which includes autofs
itself) in the first post on this won't be broken and users can elect to
use the packed data structure from now on.

Comments please.

autofs4 - add packed autofs_v6_packet to avoid struct alignment problems.

From: Thomas Meyer <[email protected]>

Updated by Ian Kent <[email protected]>

autofs_v5_packet is 300 bytes on archectures that are 32-bit aligned and
304 bytes on archectures that are 64-bit aligned. This leads to an error
in at least systemd when running a x86_64 kernel on an x86 userspace.

Fix this by adding a new protocol version 6 packet that is packed so that
it will have the same size on all archectures regardless of alignement.

Signed-off-by: Thomas Meyer <[email protected]>
Signed-off-by: Ian Kent <[email protected]>
---

fs/autofs4/inode.c | 36 ++++++++++++++++++++++++++++--------
fs/autofs4/waitq.c | 46 +++++++++++++++++++++++++++++++---------------
include/linux/auto_fs4.h | 36 +++++++++++++++++++++++++++++++++---
3 files changed, 92 insertions(+), 26 deletions(-)

diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index e16980b..18b5b89 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -197,6 +197,28 @@ static int parse_options(char *options, int *pipefd, uid_t *uid, gid_t *gid,
return (*pipefd < 0);
}

+static bool check_protocol_version(int minproto, int maxproto)
+{
+ bool rc = true;
+
+ if (minproto > maxproto || maxproto < minproto) {
+ printk("autofs: protocol min(%d)/max(%d) version error!",
+ minproto, maxproto);
+ rc = false;
+ }
+
+ if (maxproto < AUTOFS_MIN_PROTO_VERSION ||
+ minproto > AUTOFS_MAX_PROTO_VERSION) {
+ printk("autofs: kernel does not match daemon version "
+ "daemon (%d, %d) kernel (%d, %d)\n",
+ minproto, maxproto,
+ AUTOFS_MIN_PROTO_VERSION, AUTOFS_MAX_PROTO_VERSION);
+ rc = false;
+ }
+
+ return rc;
+}
+
int autofs4_fill_super(struct super_block *s, void *data, int silent)
{
struct inode * root_inode;
@@ -270,21 +292,19 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
root_inode->i_op = &autofs4_dir_inode_operations;

/* Couldn't this be tested earlier? */
- if (sbi->max_proto < AUTOFS_MIN_PROTO_VERSION ||
- sbi->min_proto > AUTOFS_MAX_PROTO_VERSION) {
- printk("autofs: kernel does not match daemon version "
- "daemon (%d, %d) kernel (%d, %d)\n",
- sbi->min_proto, sbi->max_proto,
- AUTOFS_MIN_PROTO_VERSION, AUTOFS_MAX_PROTO_VERSION);
+ if(check_protocol_version(sbi->min_proto, sbi->max_proto) == false)
goto fail_dput;
- }

/* Establish highest kernel protocol version */
if (sbi->max_proto > AUTOFS_MAX_PROTO_VERSION)
sbi->version = AUTOFS_MAX_PROTO_VERSION;
else
sbi->version = sbi->max_proto;
- sbi->sub_version = AUTOFS_PROTO_SUBVERSION;
+
+ if (sbi->version == 5)
+ sbi->sub_version = AUTOFS_V5_PROTO_SUBVERSION;
+ else
+ sbi->sub_version = AUTOFS_MAX_PROTO_SUBVERSION;

DPRINTK("pipe fd = %d, pgrp = %u", pipefd, sbi->oz_pgrp);
pipe = fget(pipefd);
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index da8876d..361a9bf 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -100,6 +100,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
struct autofs_packet_hdr hdr;
union autofs_packet_union v4_pkt;
union autofs_v5_packet_union v5_pkt;
+ struct autofs_v6_packet_union v6_pkt;
} pkt;
struct file *pipe = NULL;
size_t pktsz;
@@ -145,7 +146,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
break;
}
/*
- * Kernel protocol v5 packet for handling indirect and direct
+ * Kernel protocol v5/v6 packet for handling indirect and direct
* mount missing and expire requests
*/
case autofs_ptype_missing_indirect:
@@ -153,20 +154,35 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
case autofs_ptype_missing_direct:
case autofs_ptype_expire_direct:
{
- struct autofs_v5_packet *packet = &pkt.v5_pkt.v5_packet;
-
- pktsz = sizeof(*packet);
-
- packet->wait_queue_token = wq->wait_queue_token;
- packet->len = wq->name.len;
- memcpy(packet->name, wq->name.name, wq->name.len);
- packet->name[wq->name.len] = '\0';
- packet->dev = wq->dev;
- packet->ino = wq->ino;
- packet->uid = wq->uid;
- packet->gid = wq->gid;
- packet->pid = wq->pid;
- packet->tgid = wq->tgid;
+ if(sbi->version == 5) {
+ struct autofs_v5_packet *packet = &pkt.v5_pkt.v5_packet;
+
+ packet->wait_queue_token = wq->wait_queue_token;
+ packet->len = wq->name.len;
+ memcpy(packet->name, wq->name.name, wq->name.len);
+ packet->name[wq->name.len] = '\0';
+ packet->dev = wq->dev;
+ packet->ino = wq->ino;
+ packet->uid = wq->uid;
+ packet->gid = wq->gid;
+ packet->pid = wq->pid;
+ packet->tgid = wq->tgid;
+ pktsz = sizeof(packet);
+ } else { /* all other versions, currently only version 6 */
+ struct autofs_v6_packet *packet = &pkt.v6_pkt.v6_packet;
+
+ packet->wait_queue_token = wq->wait_queue_token;
+ packet->len = wq->name.len;
+ memcpy(packet->name, wq->name.name, wq->name.len);
+ packet->name[wq->name.len] = '\0';
+ packet->dev = wq->dev;
+ packet->ino = wq->ino;
+ packet->uid = wq->uid;
+ packet->gid = wq->gid;
+ packet->pid = wq->pid;
+ packet->tgid = wq->tgid;
+ pktsz = sizeof(packet);
+ }
break;
}
default:
diff --git a/include/linux/auto_fs4.h b/include/linux/auto_fs4.h
index e02982f..e8c644a 100644
--- a/include/linux/auto_fs4.h
+++ b/include/linux/auto_fs4.h
@@ -20,11 +20,12 @@
#undef AUTOFS_MIN_PROTO_VERSION
#undef AUTOFS_MAX_PROTO_VERSION

-#define AUTOFS_PROTO_VERSION 5
+#define AUTOFS_PROTO_VERSION 6
#define AUTOFS_MIN_PROTO_VERSION 3
-#define AUTOFS_MAX_PROTO_VERSION 5
+#define AUTOFS_MAX_PROTO_VERSION 6

-#define AUTOFS_PROTO_SUBVERSION 2
+#define AUTOFS_V5_PROTO_SUBVERSION 2
+#define AUTOFS_MAX_PROTO_SUBVERSION 0

/* Mask for expire behaviour */
#define AUTOFS_EXP_IMMEDIATE 1
@@ -154,6 +155,35 @@ union autofs_v5_packet_union {
autofs_packet_expire_direct_t expire_direct;
};

+/* autofs v6 common packet struct */
+/* packed structure for same packet size on all archs */
+struct autofs_v6_packet {
+ struct autofs_packet_hdr hdr;
+ autofs_wqt_t wait_queue_token;
+ __u32 dev;
+ __u64 ino;
+ __u32 uid;
+ __u32 gid;
+ __u32 pid;
+ __u32 tgid;
+ __u32 len;
+ char name[NAME_MAX+1];
+} __attribute__ ((packed));
+
+typedef struct autofs_v6_packet autofs_v6_packet_missing_indirect_t;
+typedef struct autofs_v6_packet autofs_v6_packet_expire_indirect_t;
+typedef struct autofs_v6_packet autofs_v6_packet_missing_direct_t;
+typedef struct autofs_v6_packet autofs_v6_packet_expire_direct_t;
+
+union autofs_v6_packet_union {
+ struct autofs_packet_hdr hdr;
+ struct autofs_v6_packet v6_packet;
+ autofs_v6_packet_missing_indirect_t missing_indirect;
+ autofs_v6_packet_expire_indirect_t expire_indirect;
+ autofs_v6_packet_missing_direct_t missing_direct;
+ autofs_v6_packet_expire_direct_t expire_direct;
+};
+
#define AUTOFS_IOC_EXPIRE_MULTI _IOW(0x93,0x66,int)
#define AUTOFS_IOC_EXPIRE_INDIRECT AUTOFS_IOC_EXPIRE_MULTI
#define AUTOFS_IOC_EXPIRE_DIRECT AUTOFS_IOC_EXPIRE_MULTI

2012-02-21 03:39:15

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, Feb 20, 2012 at 7:29 PM, Ian Kent <[email protected]> wrote:
>
> Not sure this is the way you'd like to go with this but here is a patch
> that bumps the major autofs kernel communications version (as yet not
> even compile tested).

.. and exactly how does this fix existing binaries?

Linus

2012-02-21 04:02:42

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, 2012-02-20 at 19:38 -0800, Linus Torvalds wrote:
> On Mon, Feb 20, 2012 at 7:29 PM, Ian Kent <[email protected]> wrote:
> >
> > Not sure this is the way you'd like to go with this but here is a patch
> > that bumps the major autofs kernel communications version (as yet not
> > even compile tested).
>
> .. and exactly how does this fix existing binaries?

It doesn't but changing it for exiting binaries will break existing
binaries that use a workaround.

I'm proposing this because the systemd folks were happy to do it this
way. But if you would like any other existing user space users to change
to using a correctly sized packet then, yes, it isn't what you want to
happen.

Ian

2012-02-21 04:17:55

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, Feb 20, 2012 at 8:02 PM, Ian Kent <[email protected]> wrote:
>
> It doesn't but changing it for exiting binaries will break existing
> binaries that use a workaround.

What existing binaries?

> I'm proposing this because the systemd folks were happy to do it this
> way. But if you would like any other existing user space users to change
> to using a correctly sized packet then, yes, it isn't what you want to
> happen.

Existing binaries *do* use the correct size packet - it's the correct
size for native x86-32!

It's our x86-64 compat layer that is wrong. It's a clear bug. Nothing else.

We don't start making up new interfaces because we have clear bugs: we
fix the damn bugs.

How could you even sanely do "workarounds"? A x86-32 binary shouldn't
even be able to *tell* that the kernel is 64-bit. And if it does that
somehow, we should fix that too!

Linus

2012-02-21 04:19:12

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On 02/20/2012 07:29 PM, Ian Kent wrote:
>
> Fix this by adding a new protocol version 6 packet that is packed so that
> it will have the same size on all archectures regardless of alignement.
>

You probably also want to make sure there aren't any holes in it, ever.
Alignment holes not only cause these kinds of problems, but also cause
security holes.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-02-21 04:22:34

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On 02/19/2012 12:02 PM, Linus Torvalds wrote:
> On Sun, Feb 19, 2012 at 11:49 AM, David Miller <[email protected]> wrote:
>>
>> A real shame, this should have used "__aligned_u64" from the
>> beginning.
>
> I agree. Sadly, this is exactly the kind of thing that is *really*
> easy to overlook, and once it is overlooked we're screwed because
> fixing it just breaks the native 32-bit case.
>

I'm starting to think we should compile the kernel with -Wpadded by
default (currently it's only done at "warning level 3", which I doubt
anyone ever uses, especially since that also includes -Wpacked which is
an actively toxic warning) and force people to add explicit padding
where it needs to go, if necessary. Unfortunately even that doesn't
guarantee compatibility with userspace, but it's at least something.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-02-21 04:52:45

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, 2012-02-20 at 20:17 -0800, Linus Torvalds wrote:
> On Mon, Feb 20, 2012 at 8:02 PM, Ian Kent <[email protected]> wrote:
> >
> > It doesn't but changing it for exiting binaries will break existing
> > binaries that use a workaround.
>
> What existing binaries?

My bad, autofs for one, but obviously when running in compat mode.
And any other user space users, such as the auto_dir package, but I
don't know if that is maintained any more.

>
> > I'm proposing this because the systemd folks were happy to do it this
> > way. But if you would like any other existing user space users to change
> > to using a correctly sized packet then, yes, it isn't what you want to
> > happen.
>
> Existing binaries *do* use the correct size packet - it's the correct
> size for native x86-32!
>
> It's our x86-64 compat layer that is wrong. It's a clear bug. Nothing else.
>
> We don't start making up new interfaces because we have clear bugs: we
> fix the damn bugs.
>
> How could you even sanely do "workarounds"? A x86-32 binary shouldn't
> even be able to *tell* that the kernel is 64-bit. And if it does that
> somehow, we should fix that too!

Sure, not an acceptable excuse I know, but it was too late when I
realized my mistake.

So, as you say, let's fix the bug.

autofs is by far the biggest user and I can manage the change for it and
I can at least post to the auto_dir list (if it still exists) and Thomas
can feedback to the systemd folks.

Then there is the question of how it should be done.

I think the patch attached to your original post needs a little work if
that is to be used. Correct me if I'm wrong but AFAICT there are more
architectures that use 8-byte alignment than just x86-64, such as alpha,
ia64 and ppc64 and I believe they may also be used in a compat mode.

Is there a better way to do this change, anyone?

Ian

2012-02-21 05:07:13

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, Feb 20, 2012 at 8:52 PM, Ian Kent <[email protected]> wrote:
>
> I think the patch attached to your original post needs a little work if
> that is to be used. Correct me if I'm wrong but AFAICT there are more
> architectures that use 8-byte alignment than just x86-64, such as alpha,
> ia64 and ppc64 and I believe they may also be used in a compat mode.

The only issue is compat mode, and afaik, all other architectures
except for x86-32 do __u64 with natural alignment.

So all 64-bit architectures use natural alignment, the only issue is
the alignment of __u64 in 32-bit mode.

So it really is *not* about 8-byte alignment. Quite the reverse. It's
about 4-byte alignment of 64-bit entities, and I suspect x86-32 is the
only one that does that.

See "compat_u64", and notice how only in arch/x86/include/asm/compat.h
do we have

typedef u64 __attribute__((aligned(4))) compat_u64;

So it really is limited to only x86.

Linus

2012-02-21 05:21:28

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, Feb 20, 2012 at 9:06 PM, Linus Torvalds
<[email protected]> wrote:
>
> So it really is limited to only x86.

Oh, and if people really are using "uname()" to figure out that they
are running a 64-bit kernel, then we should probably make uname() use
"is_compat_task()" instead of checking the PER_LINUX32 personality.

So we already do have support for returning a different machine-name
to 32-bit binaries, but it uses the "personality" thing that nobody
cares about, rather than the compat layer. Looks like purely
historical reasons.

Linus

2012-02-21 05:27:41

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On 02/20/2012 09:21 PM, Linus Torvalds wrote:
> On Mon, Feb 20, 2012 at 9:06 PM, Linus Torvalds
> <[email protected]> wrote:
>>
>> So it really is limited to only x86.
>
> Oh, and if people really are using "uname()" to figure out that they
> are running a 64-bit kernel, then we should probably make uname() use
> "is_compat_task()" instead of checking the PER_LINUX32 personality.
>
> So we already do have support for returning a different machine-name
> to 32-bit binaries, but it uses the "personality" thing that nobody
> cares about, rather than the compat layer. Looks like purely
> historical reasons.
>

No, it serves a real function. I use both directions of this to deal
with various compatibility things. PER_LINUX32 lets you run, say, an
installer as if it was on a 32-bit program, even if it is written in a
scripting language (and hence running a native 64-bit interpreter).
Similarly, a 32-bit legacy binary can still function as part of a bigger
64-bit system.

So let's not change that just because someone did something idiotic.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-02-21 05:28:58

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On 02/20/2012 09:06 PM, Linus Torvalds wrote:
> On Mon, Feb 20, 2012 at 8:52 PM, Ian Kent <[email protected]> wrote:
>>
>> I think the patch attached to your original post needs a little work if
>> that is to be used. Correct me if I'm wrong but AFAICT there are more
>> architectures that use 8-byte alignment than just x86-64, such as alpha,
>> ia64 and ppc64 and I believe they may also be used in a compat mode.
>
> The only issue is compat mode, and afaik, all other architectures
> except for x86-32 do __u64 with natural alignment.
>
> So all 64-bit architectures use natural alignment, the only issue is
> the alignment of __u64 in 32-bit mode.
>
> So it really is *not* about 8-byte alignment. Quite the reverse. It's
> about 4-byte alignment of 64-bit entities, and I suspect x86-32 is the
> only one that does that.
>
> See "compat_u64", and notice how only in arch/x86/include/asm/compat.h
> do we have
>
> typedef u64 __attribute__((aligned(4))) compat_u64;
>
> So it really is limited to only x86.
>

m68k has alignment 2 for 32- and 64-bit quantities, so it's not just
x86; the only reason you don't see that one is because m68k doesn't have
a compat layer to worry about.

Holes are highly undesirable for another reason: they create security
holes where kernel information leaks out.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-02-21 05:32:31

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, Feb 20, 2012 at 9:28 PM, H. Peter Anvin <[email protected]> wrote:
>
> Holes are highly undesirable for another reason: they create security
> holes where kernel information leaks out.

.. however, this is not an argument for adding a *new* interface.

You are still stuck handling the old one, so adding a new interface
without holes doesn't help *anything*. It's just a bad idea.

So we're stuck with the interfaces we have. Don't say "let's fix the
problems by adding new ones". It doesn't work, it doesn't solve
anything, and all it results in is even *more* interfaces to maintain
and find bugs in.

It's also a major pain for testing, since different people will
invariably use different interfaces. So a person running an older
distro will see a bug that the maintainer cannot reproduce, because
they maintainer has in the meantime updated to all the new and
"improved" interfaces.

So "new and improved" is just bad. Fix the existing ones, instead of
saying "oops, that was a bad interface so let's make a new one".
Always.

Linus

2012-02-21 05:39:54

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Mon, 2012-02-20 at 21:06 -0800, Linus Torvalds wrote:
> On Mon, Feb 20, 2012 at 8:52 PM, Ian Kent <[email protected]> wrote:
> >
> > I think the patch attached to your original post needs a little work if
> > that is to be used. Correct me if I'm wrong but AFAICT there are more
> > architectures that use 8-byte alignment than just x86-64, such as alpha,
> > ia64 and ppc64 and I believe they may also be used in a compat mode.
>
> The only issue is compat mode, and afaik, all other architectures
> except for x86-32 do __u64 with natural alignment.
>
> So all 64-bit architectures use natural alignment, the only issue is
> the alignment of __u64 in 32-bit mode.
>
> So it really is *not* about 8-byte alignment. Quite the reverse. It's
> about 4-byte alignment of 64-bit entities, and I suspect x86-32 is the
> only one that does that.
>
> See "compat_u64", and notice how only in arch/x86/include/asm/compat.h
> do we have
>
> typedef u64 __attribute__((aligned(4))) compat_u64;
>
> So it really is limited to only x86.

Thanks, I wasn't aware of this.

>
> Linus

2012-02-21 13:02:27

by Alan

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

> I'm starting to think we should compile the kernel with -Wpadded by
> default (currently it's only done at "warning level 3", which I doubt
> anyone ever uses, especially since that also includes -Wpacked which is
> an actively toxic warning) and force people to add explicit padding
> where it needs to go, if necessary. Unfortunately even that doesn't
> guarantee compatibility with userspace, but it's at least something.

We generate a set of believed correct export headers and check them these
days as an option. Can that work not be turned to do this. Ie create a
temporary .c file that includes all the user visible header data (without
KERNEL being defined) and runs it through -Wpacked etc

Alan

2012-02-21 18:26:08

by Sam Ravnborg

[permalink] [raw]

Subject: Use gcc to check exported files [Was: Linux 3.3-rc4]

On Tue, Feb 21, 2012 at 01:03:45PM +0000, Alan Cox wrote:
> > I'm starting to think we should compile the kernel with -Wpadded by
> > default (currently it's only done at "warning level 3", which I doubt
> > anyone ever uses, especially since that also includes -Wpacked which is
> > an actively toxic warning) and force people to add explicit padding
> > where it needs to go, if necessary. Unfortunately even that doesn't
> > guarantee compatibility with userspace, but it's at least something.
>
> We generate a set of believed correct export headers and check them these
> days as an option. Can that work not be turned to do this. Ie create a
> temporary .c file that includes all the user visible header data (without
> KERNEL being defined) and runs it through -Wpacked etc

Hi Alan.

If we require every exported file to be buildable on thier own
then it is simple to run gcc on every exported file.

We have before discussed that our header files should include everything
they need - and at least forcing this for our user-space headers would not be bad.

Simple hack I cooked up (will not work if you use O=...):

diff --git a/scripts/headers_check.pl b/scripts/headers_check.pl
index 7957e7a..b479575 100644
--- a/scripts/headers_check.pl
+++ b/scripts/headers_check.pl
@@ -27,12 +27,16 @@ my $line;
my $lineno = 0;
my $filename;

+my $gcc_options = "-Wall -Wpadded";
+my $gcc_include = "-I usr/include";
+
foreach my $file (@files) {
$filename = $file;

open(my $fh, '<', $filename)
or die "$filename: $!\n";
$lineno = 0;
+ &check_build();
while ($line = <$fh>) {
$lineno++;
&check_include();
@@ -45,6 +49,13 @@ foreach my $file (@files) {
}
exit $ret;

+# Check that the header-file can build
+# All exported headers are assumed to include what they need
+sub check_build
+{
+ system("gcc -xc -c $gcc_include $gcc_options $filename")
+}
+
sub check_include
{
if ($line =~ m/^\s*#\s*include\s+<((asm|linux).*)>/) {

Now we just need someone to fix up the headers....
I counted around 870 errors in 160 files.
But 99% looked trivial to fix...

Almost all is a matter of adding one or a few missing includes.

Sam

2012-02-21 18:58:43

by Thomas Meyer

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

Am Sonntag, den 19.02.2012, 10:45 -0800 schrieb Linus Torvalds:
> On Sun, Feb 19, 2012 at 5:42 AM, Thomas Meyer <[email protected]> wrote:
> >
> > 1.) autofs4 interface is broken between x86 and x86_64. as systemd uses autofs, this bug hangs the boot process as e.g. binfmt is mounted via autofs. see also http://lists.freedesktop.org/archives/systemd-devel/2011-September/003396.html
>
> Duh.
>
> That is just broken.
>
> The code even *talks* about how the packet layout is the same on
> 32-bit and 64-bit architectures, and that's largely true.
>
> However, while true, x86-64 has 8-byte alignment for 'long', and
> x86-32 has 4-byte alignment. Which means that even though the
> structure layout is exactly the same, on x86-64 the *alignment* issue
> will push it out to 304 bytes.
>
> That's just stupid. We've had that problem before. It's easy to
> overlook, but that packet is just mis-designed.
>
> The attached patch isn't pretty, but this is definitely a kernel bug.
> Binary compatibility is *important*, dammit.
>
> Does this fix it?

yes, it does!

thanks.

2012-02-21 22:07:56

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Tue, Feb 21, 2012 at 10:58 AM, Thomas Meyer <[email protected]> wrote:
> Am Sonntag, den 19.02.2012, 10:45 -0800 schrieb Linus Torvalds:
>>
>> Does this fix it?
>
> yes, it does!

Do you know if anybody ever applied that disgusting (and incorrect -
the 4-byte off thing *only* happens in 32-bit mode with an x86-64
kernel - so the other architecture strcmp's are wrong, and even the
x86-64 strcmp is valid only when compiling as a legacy 32-bit x86 app)
patch of yours?

Because if that actually did happen, there are i386 binaries that are
now broken and know of the kernel bug as Ian was afraid there would
be.

But hopefully that patch never got anywhere, and we could still fix
this in the kernel with my suggested patch.

Not to say that *my* patch isn't also disgusting, of course. But at
least my patch fixes a real compat task problem.

Linus

2012-02-21 22:43:39

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Sun, Feb 19, 2012 at 5:42 AM, Thomas Meyer <[email protected]> wrote:
>
> 2.) while debugging above issue: I did find an minor bug in sys_poll() - nobody did take care of my proposed patch: https://lkml.org/lkml/2011/9/24/35

Ok, so I started out forward-porting that patch to current -git
(trivial: it's just that the system call tables are differently
generated now), but the more I look at it, the more I suspect that we
should perhaps just globally fix "sys_poll()" to have the timeout
argument be 'int'.

Because that *is* the standard user interface (just do "man 2 poll"),
and while all of the git history (and all of the BK history) we've had
it as "long", I suspect we should just fix it.

So I suspect the correct patch is just as attached instead: make
sys_poll() just take an "int timeout". Any user who tried to use a
long value would already have got truncated by glibc - I just checked.

Of course, there is a remote possibility that somebody might not use
glibc, and have used "poll()" with the raw system call interface, and
depended on using a 64-bit "long timeout" on 64-bit architectures.

But quite frankly, that sounds rather unlikely in the extreme.

Comments? If we do this, and somebody actually reports that they use a
64-bit timeout, we could always go back to the broken 'long' argument,
and take your patch to fix the compat case.

Linus

Attachments:

patch.diff (1.67 kB)

2012-02-21 23:10:55

by Eric Dumazet

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

Le mardi 21 février 2012 à 14:43 -0800, Linus Torvalds a écrit :
> On Sun, Feb 19, 2012 at 5:42 AM, Thomas Meyer <[email protected]> wrote:
> >
> > 2.) while debugging above issue: I did find an minor bug in sys_poll() - nobody did take care of my proposed patch: https://lkml.org/lkml/2011/9/24/35
>
> Ok, so I started out forward-porting that patch to current -git
> (trivial: it's just that the system call tables are differently
> generated now), but the more I look at it, the more I suspect that we
> should perhaps just globally fix "sys_poll()" to have the timeout
> argument be 'int'.
>
> Because that *is* the standard user interface (just do "man 2 poll"),
> and while all of the git history (and all of the BK history) we've had
> it as "long", I suspect we should just fix it.
>
> So I suspect the correct patch is just as attached instead: make
> sys_poll() just take an "int timeout". Any user who tried to use a
> long value would already have got truncated by glibc - I just checked.
>
> Of course, there is a remote possibility that somebody might not use
> glibc, and have used "poll()" with the raw system call interface, and
> depended on using a 64-bit "long timeout" on 64-bit architectures.
>
> But quite frankly, that sounds rather unlikely in the extreme.
>
> Comments? If we do this, and somebody actually reports that they use a
> 64-bit timeout, we could always go back to the broken 'long' argument,
> and take your patch to fix the compat case.
>
> Linus

Yep, this is what I thought, but when this was raised last september,
both Andrew and Andi disagreed.

https://lkml.org/lkml/2011/10/6/389

2012-02-21 23:20:06

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Tue, Feb 21, 2012 at 3:10 PM, Eric Dumazet <[email protected]> wrote:
>
> Yep, this is what I thought, but when this was raised last september,
> both Andrew and Andi disagreed.
>
> https://lkml.org/lkml/2011/10/6/389

Well, I agree that it *could* break things, but considering that at
least glibc does the sign-exitension, any code that puts a large
number in the 'timeout' field would *already* have broken.

Which is why I think we should first try to fix the system call
interface - because it's the simpler patch, and it's the
RightThing(tm) to do from a standards standpoint. It's also almost
guaranteed to work, exactly because of how glibc already does that
conversion.

But if something does break - however unlikely and perverse the code
has to be to be able to do that - we'd clearly have to undo that "just
fix sys_poll()" and use Thomas' patch to have a compat_sys_poll()
instead.

I just don't like the notion of doing that silly compat thing when it
really shouldn't be needed to begin with.

Linus

2012-02-24 10:39:27

by Jiri Kosina

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Sat, 18 Feb 2012, Linus Torvalds wrote:

> So it's almost getting to be a habit: yet another -rc release that is
> delayed by a couple of days.

I just got the BUG below (with g45196ce being the topmost commit).

It happened when trying to start 'gwenview', but I am not able to
reproduce it again. Adding a few people to CC just in case someone
immediately sees what might be the problem.

The IP resolves to

#ifdef CONFIG_MMU
static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
{
[ ... snip ... ]
if (file) {
===> this line struct inode *inode = file->f_path.dentry->d_inode;
struct address_space *mapping = file->f_mapping;

get_file(file);
if (tmp->vm_flags & VM_DENYWRITE)
atomic_dec(&inode->i_writecount);
mutex_lock(&mapping->i_mmap_mutex);
if (tmp->vm_flags & VM_SHARED)
mapping->i_mmap_writable++;
flush_dcache_mmap_lock(mapping);
/* insert tmp into the share list, just after mpnt */
vma_prio_tree_add(tmp, mpnt);
flush_dcache_mmap_unlock(mapping);
mutex_unlock(&mapping->i_mmap_mutex);
}

more precisely:

[ ... snip ... ]
0xffffffff8103a4f9 <+409>: andq $0xffffffffffffdfff,0x30(%rbx)
0xffffffff8103a501 <+417>: movq $0x0,0x20(%rbx)
0xffffffff8103a509 <+425>: movq $0x0,0x18(%rbx)
0xffffffff8103a511 <+433>: test %rdx,%rdx
0xffffffff8103a514 <+436>: je 0xffffffff8103a565 <dup_mmap+517>
0xffffffff8103a516 <+438>: mov 0x18(%rdx),%rax
0xffffffff8103a51a <+442>: mov 0x130(%rdx),%r12
===> this line 0xffffffff8103a521 <+449>: mov 0x30(%rax),%rax
0xffffffff8103a525 <+453>: lock incq 0x68(%rdx)
0xffffffff8103a52a <+458>: testb $0x8,0x31(%rbx)
[ ... snip ... ]

The machine has gone through several suspend-resume cycles before this
happened, so it might well also be some memory corruption caused by a
random driver.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
IP: [<ffffffff8103a521>] dup_mmap+0x1c1/0x3b0
PGD 3774f067 PUD 36cf7067 PMD 0
Oops: 0000 [#1] SMP
CPU 0
Modules linked in: af_packet iwlwifi tun iptable_mangle xt_DSCP xt_tcpudp nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tab
conntrack cpufreq_conservative iptable_filter cpufreq_userspace cpufreq_powersave acpi_cpufreq ip_tables mperf x_tables microcode
ooth snd_hda_codec_conexant pcspkr iTCO_wdt iTCO_vendor_support i2c_i801 cfg80211 snd_hda_intel snd_hda_codec snd_hwdep snd_pcm sn
l ac snd tpm_tis soundcore tpm tpm_bios battery wmi autofs4 uhci_hcd i915 drm_kms_helper drm i2c_algo_bit ehci_hcd button video us
ermal thermal_sys [last unloaded: iwlwifi]

Pid: 1993, comm: Xorg Not tainted 3.3.0-rc4-00074-g45196ce #1 LENOVO 7470BN2/7470BN2
RIP: 0010:[<ffffffff8103a521>] [<ffffffff8103a521>] dup_mmap+0x1c1/0x3b0
RSP: 0018:ffff8800780bdd50 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff880077f25d98 RCX: 0000000000000000
RDX: ffff88003767ed00 RSI: ffff880037b36298 RDI: ffff880077f25d98
RBP: ffff8800780bddb0 R08: ffff880067ded4e0 R09: 0000000000000014
R10: 0000000000000000 R11: 0000000000000001 R12: ffff8800767a5d50
R13: ffff880037b36298 R14: ffff880056d520c0 R15: 0000000000000000
FS: 00007f96b2bd6880(0000) GS:ffff88007c200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000030 CR3: 00000000372a3000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process Xorg (pid: 1993, threadinfo ffff8800780bc000, task ffff880078044040)
Stack:
ffff880037b7ba80 ffff880037b7bb18 ffff880056d52158 ffff880077f25e48
ffff880077f25e60 ffff880077f25e88 ffff880077f25e80 ffff880056d520c0
ffff880037b7ba80 ffff880041afe040 0000000000000000 00007f96b2bd6b50
Call Trace:
[<ffffffff8103ab5f>] dup_mm+0xbf/0x150
[<ffffffff8103bb72>] copy_process+0xf82/0xfa0
[<ffffffff8103bf78>] do_fork+0xb8/0x300
[<ffffffff8104f94c>] ? do_sigaction+0x13c/0x1e0
[<ffffffff81164040>] ? fd_install+0x30/0x60
[<ffffffff812eb3c9>] ? lockdep_sys_exit_thunk+0x35/0x67
[<ffffffff8100af83>] sys_clone+0x23/0x30
[<ffffffff8157b553>] stub_clone+0x13/0x20
[<ffffffff8157b1f9>] ? system_call_fastpath+0x16/0x1b
Code: 00 00 00 48 81 63 30 ff df ff ff 48 c7 43 20 00 00 00 00 48 c7 43 18 00 00 00 00 48 85 d2 74 4f 48 8b 42 18 4c 8b a2 30 01 00 00 <48> 8b 40 30 f0 48 ff 42 68 f6 43 31 08 74 07 f0 ff 88 cc 01 00
RIP [<ffffffff8103a521>] dup_mmap+0x1c1/0x3b0
RSP <ffff8800780bdd50>
CR2: 0000000000000030

--
Jiri Kosina
SUSE Labs

2012-02-24 16:52:54

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Fri, Feb 24, 2012 at 2:39 AM, Jiri Kosina <[email protected]> wrote:
>
> I just got the BUG below (with g45196ce being the topmost commit).
>
> It happened when trying to start 'gwenview', but I am not able to
> reproduce it again. Adding a few people to CC just in case someone
> immediately sees what might be the problem.

Hmm. That is the code that increments the file counter, afaik:

0: 48 81 63 30 ff df ff ff andq $0xffffffffffffdfff,0x30(%rbx)
8: 48 c7 43 20 00 00 00 00 movq $0x0,0x20(%rbx)
10: 48 c7 43 18 00 00 00 00 movq $0x0,0x18(%rbx)
18: 48 85 d2 test %rdx,%rdx
1b: 74 4f je 0x6c
1d: 48 8b 42 18 mov 0x18(%rdx),%rax
21: 4c 8b a2 30 01 00 00 mov 0x130(%rdx),%r12
28:* 48 8b 40 30 mov 0x30(%rax),%rax <--
trapping instruction
2c: f0 48 ff 42 68 lock incq 0x68(%rdx)
31: f6 43 31 08 testb $0x8,0x31(%rbx)
35: 74 07 je 0x3e

and that preceding test is testing for a NULL "file", and then the

mov 0x18(%rdx),%rax

is "dentry = file->f_path.dentry", while the trapping "mov
0x30(%rax),%rax" is the continuation of that: "dentry->d_inode" (and
the "lock incq" is the get_file() - it's incrementing the file
counter). That "mov 0x130(%rdx),%r12" in between is doing "mapping =
file->f_mapping"

So dentry seems to be NULL for you.

> The machine has gone through several suspend-resume cycles before this
> happened, so it might well also be some memory corruption caused by a
> random driver.

I almost think it is, because "file->dentry" should never be NULL in a
mapping afaik. Especially as your "mapping" certainly isn't NULL (it's
in %r12, so you can see it in your register dump).

This isn't some unusual code sequence either, so I don't see it as
some random latent bug that is just very unlikely and hard to trigger
in that code itself.

I'll think about it, but my first reaction is "memory corruption". Do
you think you could try to run with a kernel that has SLAB debugging
and poisoning on? If it's a stale pointer dereference that has cleared
that dentry, that _might_ show it closer to the actual bug (rather
than a long time later when the NULL dereference happens).

Linus

2012-02-24 18:01:18

by Jiri Kosina

[permalink] [raw]

Subject: Re: Linux 3.3-rc4

On Fri, 24 Feb 2012, Linus Torvalds wrote:

> > The machine has gone through several suspend-resume cycles before this
> > happened, so it might well also be some memory corruption caused by a
> > random driver.
>
> I almost think it is, because "file->dentry" should never be NULL in a
> mapping afaik. Especially as your "mapping" certainly isn't NULL (it's
> in %r12, so you can see it in your register dump).
>
> This isn't some unusual code sequence either, so I don't see it as
> some random latent bug that is just very unlikely and hard to trigger
> in that code itself.
>
> I'll think about it, but my first reaction is "memory corruption". Do
> you think you could try to run with a kernel that has SLAB debugging
> and poisoning on? If it's a stale pointer dereference that has cleared
> that dentry, that _might_ show it closer to the actual bug (rather
> than a long time later when the NULL dereference happens).

Running DEBUG_SLAB kernel since I have first hit the bug, but nothing
popped up yet. Seems undebuggable so far.

On the other hand I wouldn't blame HW for a bit-flip, as it was a clear
NULL pointer (plus 0x30 offset), not a random garbage.

--
Jiri Kosina
SUSE Labs

2012-02-24 18:21:35