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