Patchwork netlink: align attributes on 64-bits

login
register
mail settings
Submitter Nicolas Dichtel
Date Dec. 14, 2012, 1:16 p.m.
Message ID <1355491002-3931-1-git-send-email-nicolas.dichtel@6wind.com>
Download mbox | patch
Permalink /patch/206453/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Nicolas Dichtel - Dec. 14, 2012, 1:16 p.m.
On 64 bits arch, we must ensure that attributes are always aligned on 64-bits
boundary. 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>
---
 include/net/netlink.h |  9 +++++++++
 lib/nlattr.c          | 11 ++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)
Ben Hutchings - Dec. 14, 2012, 3:49 p.m.
On Fri, 2012-12-14 at 14:16 +0100, Nicolas Dichtel wrote:
> On 64 bits arch, we must ensure that attributes are always aligned on 64-bits
> boundary. 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.
[...]
> --- 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), sizeof(void *)) ? 0 : 4;

The assumption here is that nothing needs to be aligned to a greater
width than that of a pointer.  However, for most 32-bit architectures
(i386 being an exception) the C ABI requires 64-bit alignment for 64-bit
types.  There may be cases where a mostly 32-bit processor really
requires 64-bit alignment, e.g. to load or save a pair of registers.

Ben.

> +	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;
>  }
Nicolas Dichtel - Dec. 14, 2012, 4:04 p.m.
Le 14/12/2012 16:49, Ben Hutchings a écrit :
> On Fri, 2012-12-14 at 14:16 +0100, Nicolas Dichtel wrote:
>> On 64 bits arch, we must ensure that attributes are always aligned on 64-bits
>> boundary. 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.
> [...]
>> --- 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), sizeof(void *)) ? 0 : 4;
>
> The assumption here is that nothing needs to be aligned to a greater
> width than that of a pointer.  However, for most 32-bit architectures
> (i386 being an exception) the C ABI requires 64-bit alignment for 64-bit
> types.  There may be cases where a mostly 32-bit processor really
> requires 64-bit alignment, e.g. to load or save a pair of registers.
Ok, I will wait other comments to send a v2 (which will align these attributes 
for all arch).

Nicolas
--
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. 17, 2012, 9:59 a.m.
> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), sizeof(void *)) ? 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);
> +	}
> +

Shouldn't the size of the dummy parameter be based on the value
of 'align' - and that be based on the amount of padding needed?

That aligns the write pointer, what guarantees the alignment of
the start of the buffer - so that the reader will find aligned data?

What guarantees that the reader will read the data into an
8-byte aligned buffer.

There is also the lurking issue of items that require more
than 8-byte alignment.
(x86/amd64 requires 16-byte alignment for 16-byte SSE2 regs and
32-byte alignment for the AVX regs.)

Will anyone ever want to put such items into a netlink message?

	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, 4:53 p.m.
Le 17/12/2012 10:59, David Laight a écrit :
>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), sizeof(void *)) ? 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);
>> +	}
>> +
>
> Shouldn't the size of the dummy parameter be based on the value
> of 'align' - and that be based on the amount of padding needed?
>
Align is 4 or 0. Instead of the comment and 0, I can put 'NLA_HDRLEN - align', 
which will always be 0, because we made this patch because we don't want to 
change values like NLA_HDRLEN, because many user apps have these values 
/structures hardcoded.

> That aligns the write pointer, what guarantees the alignment of
> the start of the buffer - so that the reader will find aligned data?
As Thomas said, skb->head will be aligned, am I wrong?

>
> What guarantees that the reader will read the data into an
> 8-byte aligned buffer.
>
> There is also the lurking issue of items that require more
> than 8-byte alignment.
> (x86/amd64 requires 16-byte alignment for 16-byte SSE2 regs and
> 32-byte alignment for the AVX regs.)
>
> Will anyone ever want to put such items into a netlink message?
>
> 	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

Patch

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 9690b0f..aeb9fba 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 (sizeof(void *) > 4 && 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..29ace9f 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), sizeof(void *)) ? 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;
 }