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

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* Andrew Morton <akpm@linux-foundation.org> [2010-02-12 10:19:57]:

> 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".
>

Do you know if these breaks existing applications?
 
> >  | +------------------------------+
> >  | | 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.
> > 

Could you define OK above? Does the application work without
recompiling? Have you checked for endianness issues?

> >  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 };

Shouldn't this be endianness dependent?

> > +	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.
> 

I think we assume that PID/TGID is 32 bits long, we use the length to
extract the rest of the message. We cast NLA_DATA to int* in
getdelays.c.

> 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.
>

May be, this deserves an ABI and version bump in taskstats. I'll test
this patch soon. 

-- 
	Three Cheers,
	Balbir