| Message ID | 1355762980-4285-1-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
> 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
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
> 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
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
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; }
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(-)