taskstats: Use better ifdef for alignment

PATCH In review Jeff Mahoney 5 participants 11 messages Dec 30, 2010 – Jan 8, 2011 Taskstats statistics interface

Share & tools

Jeff Mahoney

author

2010-12-30 00:12 UTC #0 lore raw

PATCH 1/1 taskstats: Use better ifdef for alignment

Commit 4be2c95d added a null field to align the taskstats structure but the discussion centered around ia64. The issue exists on other platforms with inefficient unaligned access and adding them piecemeal would be an unmaintainable mess.

This patch uses Dave Miller's suggestion of using a combination of CONFIG_64BIT && !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to determine whether alignment is needed.

Note that this will cause breakage on those platforms with applications like iotop which had hard-coded offsets into the packet to access the taskstats structure.

The message seen on systems without the alignment fixes looks like:
kernel unaligned access to 0xe000023879dca9bc, ip=0xa000000100133d10

The addresses may vary but resolve to locations inside __delayacct_add_tsk.

Reported-by David S. Miller <[email protected]>

Signed-off-by Jeff Mahoney <[email protected]>

---
 kernel/taskstats.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/taskstats.c +1 -1

@@ -349,7 +349,7 @@ static int parse(struct nlattr *na, stru
 	return ret;
 }
 
-#ifdef CONFIG_IA64
+#if defined(CONFIG_64BIT) && !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
 #define TASKSTATS_NEEDS_PADDING 1
 #endif

Andrew Morton

2010-12-30 00:15 UTC #1 lore raw

Quoted 5 lines · On Wed, 29 Dec 2010 19:12:08 -0500 Jeff Mahoney <[email protected]> wrote:

Commit 4be2c95d added a null field to align the taskstats structure but the discussion centered around ia64. The issue exists on other platforms with inefficient unaligned access and adding them piecemeal would be an unmaintainable mess.

Quoted 7 lines

This patch uses Dave Miller's suggestion of using a combination of CONFIG_64BIT && !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to determine whether alignment is needed.
Note that this will cause breakage on those platforms with applications like iotop which had hard-coded offsets into the packet to access the taskstats structure.

That seems a very good reason to not apply the patch.

Tell us more, please...

Quoted 22 lines

The message seen on systems without the alignment fixes looks like:
kernel unaligned access to 0xe000023879dca9bc, ip=0xa000000100133d10
The addresses may vary but resolve to locations inside __delayacct_add_tsk.
Reported-by David S. Miller <[email protected]>
Signed-off-by Jeff Mahoney <[email protected]>
---
 kernel/taskstats.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/taskstats.c +1 -1
@@ -349,7 +349,7 @@ static int parse(struct nlattr *na, stru
 	return ret;
 }
 
-#ifdef CONFIG_IA64
+#if defined(CONFIG_64BIT) && !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
 #define TASKSTATS_NEEDS_PADDING 1
 #endif
 

Jeff Mahoney

author

2010-12-30 05:26 UTC #2 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 11 lines · Andrew Morton

Quoted 9 lines · On Wed, 29 Dec 2010 19:12:08 -0500 Jeff Mahoney <[email protected]> wrote:
Commit 4be2c95d added a null field to align the taskstats structure but the discussion centered around ia64. The issue exists on other platforms with inefficient unaligned access and adding them piecemeal would be an unmaintainable mess.
This patch uses Dave Miller's suggestion of using a combination of CONFIG_64BIT && !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to determine whether alignment is needed.

Quoted 7 lines

Quoted 3 lines
Note that this will cause breakage on those platforms with applications like iotop which had hard-coded offsets into the packet to access the taskstats structure.
That seems a very good reason to not apply the patch.
Tell us more, please...

I don't want to rehash the same discussion, but any argument that applied to ia64 applies any other 64-bit arch other than powerpc and x86_64.

- -Jeff

Quoted 17 lines

Quoted 17 lines
The message seen on systems without the alignment fixes looks like:
kernel unaligned access to 0xe000023879dca9bc, ip=0xa000000100133d10
The addresses may vary but resolve to locations inside __delayacct_add_tsk.
Reported-by David S. Miller <[email protected]>
Signed-off-by Jeff Mahoney <[email protected]>
---
 kernel/taskstats.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/taskstats.c +0 -0
@@ -349,7 +349,7 @@ static int parse(struct nlattr *na, stru
 	return ret;
 }
 

Quoted 5 lines

kernel/taskstats.c static int parse(struct nlattr *na, stru · line 355
Quoted 5 lines
kernel/taskstats.c +1 -1
-#ifdef CONFIG_IA64
+#if defined(CONFIG_64BIT) && !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
 #define TASKSTATS_NEEDS_PADDING 1
 #endif
 

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.16 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/

iEYEARECAAYFAk0cGAoACgkQLPWxlyuTD7IdTwCfXZ0KfZ3N/zafSzGCSa4cHEbN EU8AoI/JK/1nkv5xHw4XP79wZAGo4hqk
=jAGp
-----END PGP SIGNATURE-----

Andrew Morton

2010-12-30 05:33 UTC #3 lore raw

Quoted 16 lines · On Thu, 30 Dec 2010 00:26:34 -0500 Jeff Mahoney <[email protected]> wrote:

Quoted 15 lines · Andrew Morton
Quoted 12 lines · On Wed, 29 Dec 2010 19:12:08 -0500 Jeff Mahoney <[email protected]> wrote:
Commit 4be2c95d added a null field to align the taskstats structure but the discussion centered around ia64. The issue exists on other platforms with inefficient unaligned access and adding them piecemeal would be an unmaintainable mess.
This patch uses Dave Miller's suggestion of using a combination of CONFIG_64BIT && !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to determine whether alignment is needed.
Note that this will cause breakage on those platforms with applications like iotop which had hard-coded offsets into the packet to access the taskstats structure.

Quoted 5 lines

Quoted 3 lines
That seems a very good reason to not apply the patch.
Tell us more, please...
I don't want to rehash the same discussion

Please do so. That discussion went on for a long time over many emails and multiple iterations of the patch. I personally have forgotten the reasoning and if I could remember it, I wouldn't remember which version of the patch it applied to.

Applying a patch which is *known* to break *known* userspace applications is a quite extraordinary thing to do. We owe it to people to fully explain the reasoning.

Jeff Mahoney

author

2010-12-30 15:52 UTC #4 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 24 lines · Andrew Morton

Quoted 16 lines · On Thu, 30 Dec 2010 00:26:34 -0500 Jeff Mahoney <[email protected]> wrote:
Quoted 15 lines · Andrew Morton
Quoted 12 lines · On Wed, 29 Dec 2010 19:12:08 -0500 Jeff Mahoney <[email protected]> wrote:
Commit 4be2c95d added a null field to align the taskstats structure but the discussion centered around ia64. The issue exists on other platforms with inefficient unaligned access and adding them piecemeal would be an unmaintainable mess.
This patch uses Dave Miller's suggestion of using a combination of CONFIG_64BIT && !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to determine whether alignment is needed.
Note that this will cause breakage on those platforms with applications like iotop which had hard-coded offsets into the packet to access the taskstats structure.
Quoted 5 lines
Quoted 3 lines
That seems a very good reason to not apply the patch.
Tell us more, please...
I don't want to rehash the same discussion

Quoted 8 lines

Please do so. That discussion went on for a long time over many emails and multiple iterations of the patch. I personally have forgotten the reasoning and if I could remember it, I wouldn't remember which version of the patch it applied to.
Applying a patch which is *known* to break *known* userspace applications is a quite extraordinary thing to do. We owe it to people to fully explain the reasoning.

Ok, so the gist is that iotop makes what I'd call unreasonable assumptions about the contents of a netlink genetlink packet containing generic attributes. They're typed and have headers that specify value lengths, so the client can (should) identify and skip the ones the client doesn't understand.

The kernel, as of version 2.6.36, presented a packet like so:

+--------------------------------+
| genlmsghdr - 4 bytes           |
+--------------------------------+
| NLA header - 4 bytes           | /* Aggregate header */
+-+------------------------------+
| | NLA header - 4 bytes         | /* PID header */
| +------------------------------+
| | pid/tgid   - 4 bytes         |
| +------------------------------+
| | NLA header - 4 bytes         | /* stats header */
| + -----------------------------+ <- oops. aligned on 4 byte boundary
| | struct taskstats - 328 bytes |
+-+------------------------------+

The iotop code expects that the kernel will behave as it did then, assuming that the packet format is set in stone. The format is set in stone, but the packet offsets are not. There's nothing in the packet format that guarantees that the packet will be sent in exactly the same way. The attribute contents are set (or versioned) and the aggregate contents are set but they can be anywhere in the packet.

The issue here isn't that an unaligned structure gets passed to userspace, it's that the NLA infrastructure has something of a weakness:
The 4 byte attribute header may force the payload to be unaligned. The taskstats structure is created at an unaligned location and then 64-bit values are operated on inside the kernel, so the unaligned access warnings gets spewed everywhere.

It's possible to use the unaligned access API to operate on the structure in the kernel but it seems like a wasted effort to work around userspace code that isn't following the packet format. Any new additions would also need the be worked around. It's a maintenance nightmare.

The conclusion of the earlier discussion seemed to be "ok fine, if we have to break it, don't break it on arches that don't have the problem." Dave pointed out that the unaligned access problem doesn't only exist on ia64, but also on other 64-bit arches that don't have efficient unaligned access and it should be fixed there as well. The committed version of the patch and this addition keep with the conclusion of that discussion not to break it unnecessarily, which the pid padding and the packet padding fixes did do. x86_64 and powerpc don't suffer this problem so they shouldn't suffer the solution. Other 64-bit architectures do and will, though.

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.16 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/

iEYEARECAAYFAk0cqqEACgkQLPWxlyuTD7KOtQCggszltwXS5RZwaJ9GYFI6XKj6 nyUAn30jbAJfICD0NtKLgTswee48V1jI
=WnDa
-----END PGP SIGNATURE-----

David Miller

Acked-by on v1 1/1

2010-12-31 00:22 UTC #10 lore raw

In reply to Jeff Mahoney

From: Jeff Mahoney <[email protected]>
Date: Wed, 29 Dec 2010 19:12:08 -0500

Quoted 13 lines

Commit 4be2c95d added a null field to align the taskstats structure but the discussion centered around ia64. The issue exists on other platforms with inefficient unaligned access and adding them piecemeal would be an unmaintainable mess.
This patch uses Dave Miller's suggestion of using a combination of CONFIG_64BIT && !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to determine whether alignment is needed.
Note that this will cause breakage on those platforms with applications like iotop which had hard-coded offsets into the packet to access the taskstats structure.

Quoted 7 lines

The message seen on systems without the alignment fixes looks like:
kernel unaligned access to 0xe000023879dca9bc, ip=0xa000000100133d10
The addresses may vary but resolve to locations inside __delayacct_add_tsk.
Reported-by David S. Miller <[email protected]>
Signed-off-by Jeff Mahoney <[email protected]>

Acked-by David S. Miller <[email protected]>

Florian Mickler

2011-01-01 16:19 UTC #5 lore raw

In reply to Jeff Mahoney

Quoted 33 lines · On Thu, 30 Dec 2010 10:52:01 -0500 Jeff Mahoney <[email protected]> wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 29 lines · Andrew Morton
Quoted 16 lines · On Thu, 30 Dec 2010 00:26:34 -0500 Jeff Mahoney <[email protected]> wrote:
Quoted 15 lines · Andrew Morton
Quoted 12 lines · On Wed, 29 Dec 2010 19:12:08 -0500 Jeff Mahoney <[email protected]> wrote:
Commit 4be2c95d added a null field to align the taskstats structure but the discussion centered around ia64. The issue exists on other platforms with inefficient unaligned access and adding them piecemeal would be an unmaintainable mess.
This patch uses Dave Miller's suggestion of using a combination of CONFIG_64BIT && !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to determine whether alignment is needed.
Note that this will cause breakage on those platforms with applications like iotop which had hard-coded offsets into the packet to access the taskstats structure.
Quoted 5 lines
Quoted 3 lines
That seems a very good reason to not apply the patch.
Tell us more, please...
I don't want to rehash the same discussion
Please do so. That discussion went on for a long time over many emails and multiple iterations of the patch. I personally have forgotten the reasoning and if I could remember it, I wouldn't remember which version of the patch it applied to.

Quoted 7 lines

Quoted 3 lines
Applying a patch which is *known* to break *known* userspace applications is a quite extraordinary thing to do. We owe it to people to fully explain the reasoning.
Ok, so the gist is that iotop makes what I'd call unreasonable assumptions about the contents of a netlink genetlink packet containing generic attributes.

Is there already a patch available or integrated into iotop which fixes this? I'd think that if the kernel could wait on fixed iotop's to be distributed it would be easier to sell the breakage on bugzilla & co...

Just my 2 cents, but I don't know enough to weigh the different aspects of this... iotop shurely is not a showstopper if the upsides of this patch are big enough?

Regards,
Flo

Dan Carpenter

2011-01-01 16:51 UTC #6 lore raw

Quoted 5 lines · Florian Mickler

Is there already a patch available or integrated into iotop which fixes this? I'd think that if the kernel could wait on fixed iotop's to be distributed it would be easier to sell the breakage on bugzilla & co...

The story is that 4be2c95d1f "taskstats: pad taskstats netlink response for aligment issues on ia64" broke iotop on 64 bit processors (32 bit was unaffected). This patch fixes it for x86-64 and powerpc-64.

On the other 64 bit processors iotop works but instead you get a message in dmesg:
kernel unaligned access to 0xe000023879dca9bc, ip=0xa000000100133d10

So on those arches you were already getting posts to bugzilla etc. Now those people can upgrade the kernel and download iotop version 0.4.2 or higher.

regards,
dan carpenter

Florian Mickler

2011-01-02 12:17 UTC #7 lore raw

Quoted 15 lines · On Sat, 1 Jan 2011 19:51:35 +0300 Dan Carpenter <[email protected]> wrote:

Quoted 5 lines · Florian Mickler
Is there already a patch available or integrated into iotop which fixes this? I'd think that if the kernel could wait on fixed iotop's to be distributed it would be easier to sell the breakage on bugzilla & co...
The story is that 4be2c95d1f "taskstats: pad taskstats netlink response for aligment issues on ia64" broke iotop on 64 bit processors (32 bit was unaffected). This patch fixes it for x86-64 and powerpc-64.
On the other 64 bit processors iotop works but instead you get a message in dmesg:
kernel unaligned access to 0xe000023879dca9bc, ip=0xa000000100133d10

Quoted 7 lines

So on those arches you were already getting posts to bugzilla etc. Now those people can upgrade the kernel and download iotop version 0.4.2 or higher.
regards,
dan carpenter

Ah ok. And probably bug #24272 is related to this. If I understood it correctly 85893120 fixed some runtime warnings (or more exactly the misaligning) and broke iotop in the process (on all of 64 bit).

4be2c95d1f undid that and enhanced the netlink messages on IA64 archs with a new type to fix the alignment and now it's changed to align well on all archs that need alignment, leaving archs unbroken that have efficient unaligned access (x86-64 and powerpc-64). Nice.

And to answer my original question, i just checked out (some?/the?) iotop sources from git://repo.or.cz/iotop.git and found a fix[1] for the issue with 85893120.

Is iotop parsing fixed to ignore the null-field in the meantime?

Thx,
Flo

[1]git://repo.or.cz/iotop.git:commit 08211d209ae8fc7e67ea3bebb09979ff61c70f97

Author: Guillaume Chazarain <[email protected]>
Date:   Sat Sep 4 13:57:43 2010 +0200

Instead of assuming the pid field is 4 bytes long, take its length from the header. This is needed for http://lkml.org/lkml/2010/2/12/167
[PATCH] delayacct: align to 8 byte boundary on 64-bit systems

Florian Mickler

2011-01-08 22:42 UTC #8 lore raw

Quoted 8 lines · On Sun, 2 Jan 2011 13:17:47 +0100 Florian Mickler <[email protected]> wrote:

Quoted 5 lines · On Sat, 1 Jan 2011 19:51:35 +0300 Dan Carpenter <[email protected]> wrote:
Quoted 4 lines · Florian Mickler
Is there already a patch available or integrated into iotop which fixes this? I'd think that if the kernel could wait on fixed iotop's to be distributed it would be easier to sell the breakage on bugzilla & co...

Ok, I sent a fix for the iotop parsing to Guillaume, but didn't receive any reaction... maybe someone here find's it useful...

commit 392f7e7455c94cdc64aeb0353a79d22b4780687c

Author: Florian Mickler <[email protected]>
Date:   Tue Jan 4 12:03:25 2011 +0100

fix parsing of netlink messages

The linux kernel may enhance the message with fields to pad the taskstats struct (commit 4be2c95d1f ["taskstats: pad taskstats netlink
response for aligment issues on ia64"] in Linus tree).
So now we honour the flexible format and extract the field we are interested in.

iotop/data.py +14 -9

index 1164344..c75c190 100644
@@ -136,6 +136,7 @@ TASKSTATS_CMD_GET = 1
 TASKSTATS_CMD_ATTR_PID = 1
 TASKSTATS_TYPE_AGGR_PID = 4
 TASKSTATS_TYPE_PID = 1
+TASKSTATS_TYPE_STATS = 3
 
 class TaskStatsNetlink(object):
     # Keep in sync with format_stats() and pinfo.did_some_io()

@@ -163,19 +164,23 @@ class TaskStatsNetlink(object):
         if len(reply.payload) < 292:
             # Short reply
             return
-        reply_length, reply_type = struct.unpack('HH', reply.payload[4:8])
-        assert reply_length >= 288
-        assert reply_type == TASKSTATS_TYPE_AGGR_PID
-
-        pid_length, pid_type = struct.unpack('HH', reply.payload[8:12])
-        assert pid_type == TASKSTATS_TYPE_PID
-
-        taskstats_start = 4 + 4 + pid_length + 4
-        taskstats_data = reply.payload[taskstats_start:]
+	reply.payload = reply.payload[4:]
+	data = get_data(TASKSTATS_TYPE_AGGR_PID, reply.payload)
+	assert( len(data) > 0)
+	taskstats_data = get_data(TASKSTATS_TYPE_STATS, data)
         taskstats_version = struct.unpack('H', taskstats_data[:2])[0]
         assert taskstats_version >= 4
         return Stats(taskstats_data)
 
+def get_data(field_type, payload):
+	while len(payload) > 3:
+		reply_length, reply_type = struct.unpack('HH', payload[:4])
+		if (reply_type == field_type):
+			break
+		payload = payload[reply_length:]
+
+	return payload[4:reply_length]
+
 #
 # PIDs manipulations
 #

Jeff Mahoney

author

2011-01-08 22:46 UTC #9 lore raw

In reply to Florian Mickler

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 36 lines · Florian Mickler

Quoted 15 lines · On Sat, 1 Jan 2011 19:51:35 +0300 Dan Carpenter <[email protected]> wrote:
Quoted 5 lines · Florian Mickler
Is there already a patch available or integrated into iotop which fixes this? I'd think that if the kernel could wait on fixed iotop's to be distributed it would be easier to sell the breakage on bugzilla & co...
The story is that 4be2c95d1f "taskstats: pad taskstats netlink response for aligment issues on ia64" broke iotop on 64 bit processors (32 bit was unaffected). This patch fixes it for x86-64 and powerpc-64.
On the other 64 bit processors iotop works but instead you get a message in dmesg:
kernel unaligned access to 0xe000023879dca9bc, ip=0xa000000100133d10
Quoted 7 lines
So on those arches you were already getting posts to bugzilla etc. Now those people can upgrade the kernel and download iotop version 0.4.2 or higher.
regards,
dan carpenter
Ah ok. And probably bug #24272 is related to this. If I understood it correctly 85893120 fixed some runtime warnings (or more exactly the misaligning) and broke iotop in the process (on all of 64 bit).
4be2c95d1f undid that and enhanced the netlink messages on IA64 archs with a new type to fix the alignment and now it's changed to align well on all archs that need alignment, leaving archs unbroken that have efficient unaligned access (x86-64 and powerpc-64). Nice.

Quoted 5 lines

And to answer my original question, i just checked out (some?/the?) iotop sources from git://repo.or.cz/iotop.git and found a fix[1] for the issue with 85893120.
Is iotop parsing fixed to ignore the null-field in the meantime?

It isn't yet, but I sent Guillaume a patch to iotop when I sent in the null-field patch that actually parses the packet instead of doing field-offset tricks.

- -Jeff

Quoted 10 lines

Thx,
Flo
[1]git://repo.or.cz/iotop.git:commit 08211d209ae8fc7e67ea3bebb09979ff61c70f97
Author: Guillaume Chazarain <[email protected]>
Date:   Sat Sep 4 13:57:43 2010 +0200
Instead of assuming the pid field is 4 bytes long, take its length from the header. This is needed for http://lkml.org/lkml/2010/2/12/167
[PATCH] delayacct: align to 8 byte boundary on 64-bit systems

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.16 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/

iEYEARECAAYFAk0o6UUACgkQLPWxlyuTD7Is/wCfTUBE/2hg19vfGO2Wzk3Zkguo mW8AnAv6ey78E7dJsvkKLy/YS6vW10cE
=fUIp
-----END PGP SIGNATURE-----