Re: [PATCH] delayacct: align to 8 byte boundary on 64-bit systems

Andrew Morton
2010-02-12T18:20:34Z

On Fri, 12 Feb 2010 11:48:27 -0500
Jeff Mahoney <jeffm@suse.com> wrote:

> prepare_reply sets up an skb for the response. If I understand it correctly,
> the payload contains:
>
> +--------------------------------+
> | genlmsghdr - 4 bytes |
> +--------------------------------+
> | NLA header - 4 bytes | /* Aggregate header */
> +-+------------------------------+
> | | NLA header - 4 bytes | /* PID header */
> | +------------------------------+
> | | pid/tgid - 4 bytes |

So we put another four zero bytes in here and add four to the "PID header".

> | +------------------------------+
> | | NLA header - 4 bytes | /* stats header */
> | + -----------------------------+ <- oops. aligned on 4 byte boundary
> | | struct taskstats - 328 bytes |
> +-+------------------------------+
>
> The start of the taskstats struct must be 8 byte aligned on IA64 (and other
> systems with 8 byte alignment rules for 64-bit types) or runtime alignment
> warnings will be issued.
>
> This patch pads the pid/tgid field out to sizeof(long), which forces
> the alignment of taskstats. The getdelays userspace code is ok with this
> since it assumes 32-bit pid/tgid and then honors that header's length field.
>
> An array is used to avoid exposing kernel memory contents to userspace in the
> response.
>
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
> kernel/taskstats.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -362,6 +362,12 @@ static struct taskstats *mk_reply(struct
> struct nlattr *na, *ret;
> int aggr;
>
> + /* If we don't pad, we end up with alignment on a 4 byte boundary.
> + * This causes lots of runtime warnings on systems requiring 8 byte
> + * alignment */
> + u32 pids[2] = { pid, 0 };
> + int pid_size = ALIGN(sizeof(pid), sizeof(long));
> +
> aggr = (type == TASKSTATS_TYPE_PID)
> ? TASKSTATS_TYPE_AGGR_PID
> : TASKSTATS_TYPE_AGGR_TGID;
> @@ -369,7 +375,7 @@ static struct taskstats *mk_reply(struct
> na = nla_nest_start(skb, aggr);
> if (!na)
> goto err;
> - if (nla_put(skb, type, sizeof(pid), &pid) < 0)
> + if (nla_put(skb, type, pid_size, pids) < 0)
> goto err;
> ret = nla_reserve(skb, TASKSTATS_TYPE_STATS, sizeof(struct taskstats));
> if (!ret)

So any code which assumes that the pid/tgid field is four bytes long
will break. Code which takes that length from the netlink message
header will work OK.

32-bit architectures are unaltered.

Seems safe enough. We'd be safer still if we didn't do this on 64-bit
architectures which don't need it. ie: x86_64. But if we do that we
add a risk that people will develop shoddy code which works on x86_64
and doesn't work on ia64.

hmm.