Patchwork [v2] netlink: align attributes on 64-bits

login
register
mail settings
Submitter Nicolas Dichtel
Date Dec. 17, 2012, 4:49 p.m.
Message ID <1355762980-4285-1-git-send-email-nicolas.dichtel@6wind.com>
Download mbox | patch
Permalink /patch/206920/
State Superseded
Delegated to: David Miller
Headers show

Comments

Nicolas Dichtel - Dec. 17, 2012, 4:49 p.m.
We must ensure that attributes are always aligned on 64-bits boundary because
some arch may trap when accessing unaligned 64 bits value. We do that by adding
attributes of type 0, size 4 (alignment on 32-bits is already done) when needed.
Attribute type 0 should be available and unused in all netlink families.

Some callers of nlmsg_new() calculates the exact length of the attributes they
want to add to their netlink messages. Because we may add some unexpected
attributes type 0, we should take more room for that.

Note that I made the choice to align all kind of netlink attributes (even u8,
u16, ...) to simplify netlink API. Having two sort of nla_put() functions will
certainly be a source of wrong usage. Moreover, it ensures that all existing
code will be fine.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v2: align attributes on all arch, not only on 64-bits arch

 include/net/netlink.h |  9 +++++++++
 lib/nlattr.c          | 11 ++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)
David Laight - Dec. 17, 2012, 5:06 p.m.
>  int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>  {
> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;

I've just realised where you are adding this!
You only want to add pad if the attribute is a single 64bit item,
not whenever the destination is misaligned.

Eg what happens if you add a 4-byte item after an 8 byte one.

Are there are attributes that consist of a pair of 4 byte values?

...
> +	if (align) {
> +		/* Goal is to add an attribute with size 4. We know that
> +		 * NLA_HDRLEN is 4, hence payload is 0.
> +		 */
> +		__nla_reserve(skb, 0, 0);

One of those zeros should be 'align - 4', then the comment
can be more descriptive.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel - Dec. 17, 2012, 5:35 p.m.
Le 17/12/2012 18:06, David Laight a écrit :
>>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>>   {
>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
>
> I've just realised where you are adding this!
> You only want to add pad if the attribute is a single 64bit item,
> not whenever the destination is misaligned.
As said in the commit log, I want to align all attributes. An attribute can be 
like this:

struct foo {
	__u32 bar1;
	__u32 bar2;
	__u64 bar3;
}

nla_put() don't know what is contained in the attribute.

>
> Eg what happens if you add a 4-byte item after an 8 byte one.
>
> Are there are attributes that consist of a pair of 4 byte values?
>
> ...
>> +	if (align) {
>> +		/* Goal is to add an attribute with size 4. We know that
>> +		 * NLA_HDRLEN is 4, hence payload is 0.
>> +		 */
>> +		__nla_reserve(skb, 0, 0);
>
> One of those zeros should be 'align - 4', then the comment
> can be more descriptive.
I thought if you were to research why we use 0, you would know that the first 0 
is the type and the second is the payload size...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Dec. 18, 2012, 9:19 a.m.
> Le 17/12/2012 18:06, David Laight a écrit :
> >>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
> >>   {
> >> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> >> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
> >
> > I've just realised where you are adding this!
> > You only want to add pad if the attribute is a single 64bit item,
> > not whenever the destination is misaligned.
> As said in the commit log, I want to align all attributes. An attribute can be
> like this:
> 
> struct foo {
> 	__u32 bar1;
> 	__u32 bar2;
> 	__u64 bar3;
> }
> 
> nla_put() don't know what is contained in the attribute.

Put there is no need to 8-byte align something whose size isn't a
multiple of 8 bytes.

> > ...
> >> +	if (align) {
> >> +		/* Goal is to add an attribute with size 4. We know that
> >> +		 * NLA_HDRLEN is 4, hence payload is 0.
> >> +		 */
> >> +		__nla_reserve(skb, 0, 0);
> >
> > One of those zeros should be 'align - 4', then the comment
> > can be more descriptive.

> I thought if you were to research why we use 0, you would know that the first 0
> is the type and the second is the payload size...

I can tell that one is the type and the other the size, you've
implied that the 'type+size' actually total 4 bytes.
I don't need to find out which is which!
Now you've told me I'd have written:
	_nla_reserve(skb, 0, align - NLA_HDRLEN);

The compiler could well have tracked the value - so know it is 4.
OTOH you might want to generate the size of 'align' without
using a conditional.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel - Dec. 18, 2012, 10:18 a.m.
Le 18/12/2012 10:19, David Laight a écrit :
>> Le 17/12/2012 18:06, David Laight a écrit :
>>>>    int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>>>>    {
>>>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>>>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
>>>
>>> I've just realised where you are adding this!
>>> You only want to add pad if the attribute is a single 64bit item,
>>> not whenever the destination is misaligned.
>> As said in the commit log, I want to align all attributes. An attribute can be
>> like this:
>>
>> struct foo {
>> 	__u32 bar1;
>> 	__u32 bar2;
>> 	__u64 bar3;
>> }
>>
>> nla_put() don't know what is contained in the attribute.
>
> Put there is no need to 8-byte align something whose size isn't a
> multiple of 8 bytes.
Even if you cast the structure in a buffer and read bar3 (without any memcpy 
before)?

>
>>> ...
>>>> +	if (align) {
>>>> +		/* Goal is to add an attribute with size 4. We know that
>>>> +		 * NLA_HDRLEN is 4, hence payload is 0.
>>>> +		 */
>>>> +		__nla_reserve(skb, 0, 0);
>>>
>>> One of those zeros should be 'align - 4', then the comment
>>> can be more descriptive.
>
>> I thought if you were to research why we use 0, you would know that the first 0
>> is the type and the second is the payload size...
>
> I can tell that one is the type and the other the size, you've
> implied that the 'type+size' actually total 4 bytes.
> I don't need to find out which is which!
> Now you've told me I'd have written:
> 	_nla_reserve(skb, 0, align - NLA_HDRLEN);
>
> The compiler could well have tracked the value - so know it is 4.
> OTOH you might want to generate the size of 'align' without
> using a conditional.
Yes, it is the goal ;-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 9690b0f..bd9e48f 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -492,6 +492,15 @@  static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
  */
 static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
 {
+	/* Because attributes may be aligned on 64-bits boundary with fake
+	 * attribute (type 0, size 4 (attributes are 32-bits align by default)),
+	 * an exact payload size cannot be calculated. Hence, we need to reserve
+	 * more space for these attributes.
+	 * 128 is arbitrary: it allows to align up to 32 attributes.
+	 */
+	if (payload < NLMSG_DEFAULT_SIZE)
+		payload = min(payload + 128, (size_t)NLMSG_DEFAULT_SIZE);
+
 	return alloc_skb(nlmsg_total_size(payload), flags);
 }
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 18eca78..7440a80 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -450,9 +450,18 @@  EXPORT_SYMBOL(__nla_put_nohdr);
  */
 int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
 {
-	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
+	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
+
+	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen) + align))
 		return -EMSGSIZE;
 
+	if (align) {
+		/* Goal is to add an attribute with size 4. We know that
+		 * NLA_HDRLEN is 4, hence payload is 0.
+		 */
+		__nla_reserve(skb, 0, 0);
+	}
+
 	__nla_put(skb, attrtype, attrlen, data);
 	return 0;
 }