Message-ID: <20121218170853.GH27746@casper.infradead.org>
Date:	Tue, 18 Dec 2012 17:08:53 +0000
From:	Thomas Graf <tgraf@...g.ch>
To:	Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:	bhutchings@...arflare.com, netdev@...r.kernel.org,
	davem@...emloft.net, David.Laight@...LAB.COM
Subject: Re: [PATCH v2] netlink: align attributes on 64-bits

On 12/18/12 at 05:23pm, Nicolas Dichtel wrote:
> Le 18/12/2012 13:57, Thomas Graf a écrit :
> >-static inline int nla_padlen(int payload)
> >-{
> >-	return nla_total_size(payload) - nla_attr_size(payload);
> >+	if (!IS_ALIGNED(len, NLA_ATTR_ALIGN))
> >+		len = ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN);
> Two comments:
> 1/ should it be ALIGN(len, NLA_ATTR_ALIGN)? If we want to add a __u64:
>    => nla_attr_size(sizeof(__u64)) = 12
>    => NLA_ALIGN(nla_attr_size(sizeof(__u64))) => 12 (= len)
>    => ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN) = 0 but it should be 4

We can't add 1-3 bytes of padding, therefore we need to add
NLA_HDRLEN to len before aligning it to enforce a minimal
padding. We can't hit it right now because 4 byte alignment
of the previous attribute is a given but if we ever change
the alignment it could become an issue and the above should
be bullet proof.

Your example would come out like this:
  nla_attr_size(8) = 12
  ALIGN(12 + 4, 8) = 16

> 2/ Suppose that the attribute is:
> 
>   struct foo {
>   	__u64 bar1;
>   	__u32 bar2;
>   }
>   => sizeof(struct foo) = 12 (= payload)
>   => nla_attr_size(payload) = 16
>   => NLA_ALIGN(nla_attr_size(payload)) = 16 (= len)
>   => IS_ALIGNED(len, NLA_ATTR_ALIGN) = true
>   => extra room is not reserved
>   But it's not guaranteed that bar1 is aligned on 8 bytes, only on 4 bytes.

That's correct, that's why I have added the additional
NLA_ATTR_ALIGN of room in nlmsg_new(). It will account
for the one time padding that is needed before we add
the very first attribute.

If all attributes after that have a size aligned to 8
bytes no padding is needed. Padding will only be needed
again if a struct is missized in which case we reserve
room with the above. Correct?

> >+	offset = (size_t) skb_tail_pointer(skb);
> >+	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN)) {
> With the previous struct foo, this test may be true even if we don't
> have reserved extra room. This test depends on previous attribute.
> I think the exact size of the netlink message depends on the order
> of attributes, not only on the attribute itself.
> What about taking the assumption that the start will never be
> aligned and always allocating extra room: ALIGN(NLA_ALIGNTO,
> NLA_ATTR_ALIGN) (= 4)?

See my explanation above. I think this works. The order does not
matter, the sum of all padding required will always be the same.

> >+static bool nla_insufficient_space(struct sk_buff *skb, int attrlen)
> >+{
> >+	size_t needed = nla_pre_padlen(skb) + nla_total_size(attrlen);
> If nla_total_size() was right, nla_pre_padlen(skb) should already be
> included. Am I wrong?

No, nla_pre_padlen() contains the number of bytes needed to align
skb_tail_pointer() to an alignment of 8. If that is > 0 but the
attribute to follow is already aligned.

The tricky part here is that accounting for padding in
nla_total_size() only works for the sum of all attributes.
It does not account for the specific padding required for the
previous attribute.

Therefore the above check. The above could be changed to
nla_attr_size() theoretically as we don't need space for the
final padding eventually but we checked for space before so I
kept it that way.

I realize it's slightly confusign and needs better documentation
and please double check my thinking :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists