[v2] netlink: align attributes on 64-bits

Le 18/12/2012 13:57, Thomas Graf a écrit :
> On 12/17/12 at 05:49pm, Nicolas Dichtel wrote:
>> @@ -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);
>
> This is doomed to fail eventually. A netlink message may carry
> hundreds of attributes eventually. See my suggestion below.
>
>> 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;
>
> This does not look right. In order for the attribute data to be
> aligned properly you would need to check skb_tail_pointer(skb) +
> NLA_HDRLEN for proper alignment or you end up aligning the
> attribute header.
>
> How about we change nla_total_size() to return the size with
> needed padding taken into account. That should fix the message
> size caluclation problem and we only need to reserve room for
> the initial padding to align the very first attribute.
>
> Below is an untested patch that does this. What do you think?
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 9690b0f..7ce8e76 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -114,7 +114,6 @@
>    * Attribute Length Calculations:
>    *   nla_attr_size(payload)		length of attribute w/o padding
>    *   nla_total_size(payload)		length of attribute w/ padding
> - *   nla_padlen(payload)		length of padding
>    *
>    * Attribute Payload Access:
>    *   nla_data(nla)			head of attribute payload
> @@ -492,6 +491,14 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
>    */
>   static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
>   {
> +	/* If an exact size if specified, reserve some additional space to
> +	 * align the first attribute, all subsequent attributes should have
> +	 * padding accounted for.
> +	 */
> +	if (payload != NLMSG_DEFAULT_SIZE)
> +		payload = min_t(size_t, payload + NLA_ATTR_ALIGN,
> +				NLMSG_DEFAULT_SIZE);
> +
>   	return alloc_skb(nlmsg_total_size(payload), flags);
>   }
>
> @@ -653,16 +660,12 @@ static inline int nla_attr_size(int payload)
>    */
>   static inline int nla_total_size(int payload)
>   {
> -	return NLA_ALIGN(nla_attr_size(payload));
> -}
> +	size_t len = NLA_ALIGN(nla_attr_size(payload));
>
> -/**
> - * nla_padlen - length of padding at the tail of attribute
> - * @payload: length of payload
> - */
> -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);
> +
> +	return len;
>   }
>
>   /**
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 78d5b8a..1856729 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -149,5 +149,10 @@ struct nlattr {
>   #define NLA_ALIGN(len)		(((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
>   #define NLA_HDRLEN		((int) NLA_ALIGN(sizeof(struct nlattr)))
>
> +/* Padding attribute type, added automatically to align attributes,
> + * must be ignored by readers. */
> +#define NLA_PADDING		0
> +#define NLA_ATTR_ALIGN		8
> +
>
>   #endif /* _UAPI__LINUX_NETLINK_H */
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 18eca78..b09473c 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -322,18 +322,36 @@ int nla_strcmp(const struct nlattr *nla, const char *str)
>    * Adds a netlink attribute header to a socket buffer and reserves
>    * room for the payload but does not copy it.
>    *
> + * May add a padding attribute of type NLA_PADDING before the
> + * real attribute to ensure proper alignment.
> + *
>    * The caller is responsible to ensure that the skb provides enough
>    * tailroom for the attribute header and payload.
>    */
>   struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
>   {
>   	struct nlattr *nla;
> +	size_t offset;
> +
> +	offset = (size_t) skb_tail_pointer(skb);
> +	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN)) {
> +		struct nlattr *pad;
> +		size_t padlen;
> +
> +		padlen = nla_total_size(offset) - offset - NLA_HDRLEN;
Here padlen will return 4, which is wrong: padlen + NLA_HDRLEN = 8, alignment is 
the same than before. Here is a proposal fix:

  		pad->nla_len = nla_attr_size(padlen);

With this patch, it seems goods. attribute are always aligned on 8 bytes. Also
I did not notice any problem with size calculation (I try some ip link, ip xfrm, 
ip [m]route).

Do you want to make more tests? Or will your repost the full patch?
I can do it if you don't have time.
--
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

On 12/19/12 at 12:22pm, Nicolas Dichtel wrote:
> Here padlen will return 4, which is wrong: padlen + NLA_HDRLEN = 8,
> alignment is the same than before. Here is a proposal fix:
> 
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index e4f0329..1556313 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -338,7 +338,10 @@ struct nlattr *__nla_reserve(struct sk_buff
> *skb, int attrtype, int attrlen)
>  		struct nlattr *pad;
>  		size_t padlen;
> 
> -		padlen = nla_total_size(offset) - offset -  NLA_HDRLEN;
> +		/* We need to remove NLA_HDRLEN two times: one time for the
> +		 * attribute hdr and one time for the pad attribute hdr.
> +		 */
> +		padlen = nla_total_size(offset) - offset -  2 * NLA_HDRLEN;
>  		pad = (struct nlattr *) skb_put(skb, nla_attr_size(padlen));
>  		pad->nla_type = 0;
>  		pad->nla_len = nla_attr_size(padlen);
> 
> With this patch, it seems goods. attribute are always aligned on 8 bytes. Also
> I did not notice any problem with size calculation (I try some ip
> link, ip xfrm, ip [m]route).
> 
> Do you want to make more tests? Or will your repost the full patch?
> I can do it if you don't have time.

Thanks.

I would like to do some testing as well. I do expect some fallout from
this. There is likely some interface abuse that will now be exposed
due to this.

We'll have to wait for the next merge window to open anyway. I'd
consider this a new feature and not a bugfix based on the possible
regression impact it could have.

I'll post a new version of the patch integrating your fix above so
others (especially subsystem maintainers depending on netlink) can run
the patch as well.
--
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

Le 19/12/2012 18:09, Thomas Graf a écrit :
> On 12/19/12 at 12:22pm, Nicolas Dichtel wrote:
>> Here padlen will return 4, which is wrong: padlen + NLA_HDRLEN = 8,
>> alignment is the same than before. Here is a proposal fix:
>>
>> diff --git a/lib/nlattr.c b/lib/nlattr.c
>> index e4f0329..1556313 100644
>> --- a/lib/nlattr.c
>> +++ b/lib/nlattr.c
>> @@ -338,7 +338,10 @@ struct nlattr *__nla_reserve(struct sk_buff
>> *skb, int attrtype, int attrlen)
>>   		struct nlattr *pad;
>>   		size_t padlen;
>>
>> -		padlen = nla_total_size(offset) - offset -  NLA_HDRLEN;
>> +		/* We need to remove NLA_HDRLEN two times: one time for the
>> +		 * attribute hdr and one time for the pad attribute hdr.
>> +		 */
>> +		padlen = nla_total_size(offset) - offset -  2 * NLA_HDRLEN;
>>   		pad = (struct nlattr *) skb_put(skb, nla_attr_size(padlen));
>>   		pad->nla_type = 0;
>>   		pad->nla_len = nla_attr_size(padlen);
>>
>> With this patch, it seems goods. attribute are always aligned on 8 bytes. Also
>> I did not notice any problem with size calculation (I try some ip
>> link, ip xfrm, ip [m]route).
>>
>> Do you want to make more tests? Or will your repost the full patch?
>> I can do it if you don't have time.
>
> Thanks.
>
> I would like to do some testing as well. I do expect some fallout from
> this. There is likely some interface abuse that will now be exposed
> due to this.
>
> We'll have to wait for the next merge window to open anyway. I'd
> consider this a new feature and not a bugfix based on the possible
> regression impact it could have.
>
> I'll post a new version of the patch integrating your fix above so
> others (especially subsystem maintainers depending on netlink) can run
> the patch as well.
>
Ok, sounds good.
--
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