Messages in this thread | | | Date | Fri, 12 Feb 2010 14:20:23 -0500 | From | Jeff Mahoney <> | Subject | Re: [PATCH] delayacct: align to 8 byte boundary on 64-bit systems |
| |
On 02/12/2010 01:19 PM, Andrew Morton wrote: > 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.
Is there a way to do that without needlessly complicating things? I didn't see any existing infrastructure to do that.
Another option was to put an empty attribute in with a garbage type, which would add a 4 byte header - but even the getdelays code included with the kernel can't deal with that.
It's ugly all the way around.
-Jeff
-- Jeff Mahoney SUSE Labs
| |