From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:49
The goal of this serie is to be able to monitor multicast activities via
rtnetlink.
The main changes are:
- when user dumps mfc entries it now get all entries, included the unresolved
cache.
- kernel sends rtnetlink when it adds/deletes mfc entries.
As usual, the patch against iproute2 will be sent once the patches are included and
net-next merged. I can send it on demand.
include/linux/inetdevice.h | 3 +
include/net/addrconf.h | 3 +
include/uapi/linux/netconf.h | 1 +
include/uapi/linux/rtnetlink.h | 8 +++
net/ipv4/devinet.c | 10 ++-
net/ipv4/ipmr.c | 107 +++++++++++++++++++++++++++++--
net/ipv6/addrconf.c | 10 ++-
net/ipv6/ip6mr.c | 141 +++++++++++++++++++++++++++++++++++------
8 files changed, 253 insertions(+), 30 deletions(-)
Comments are welcome.
Regards,
Nicolas
From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:49
This patch removes the skb manipulations when nested attributes are added by
using standard helpers.
Signed-off-by: Nicolas Dichtel <redacted>
---
net/ipv6/ip6mr.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
@@ -2119,8 +2119,7 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,{intct;structrtnexthop*nhp;-u8*b=skb_tail_pointer(skb);-structrtattr*mp_head;+structnlattr*mp_attr;/* If cache is unresolved, don't try to parse IIF and OIF */if(c->mf6c_parent>=MAXMIFS)
From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:49
These statistics can be checked only via /proc/net/ip_mr_cache or
SIOCGETSGCNT[_IN6] and thus only for the table RT_TABLE_DEFAULT.
Advertising them via rtnetlink allows to get statistics for all cache entries,
whatever the table is.
Signed-off-by: Nicolas Dichtel <redacted>
---
include/uapi/linux/rtnetlink.h | 7 +++++++
net/ipv4/ipmr.c | 7 +++++++
net/ipv6/ip6mr.c | 7 +++++++
3 files changed, 21 insertions(+)
@@ -2046,6 +2046,7 @@ static int __ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,intct;structrtnexthop*nhp;structnlattr*mp_attr;+structrta_mfc_statsmfcs;/* If cache is unresolved, don't try to parse IIF and OIF */if(c->mfc_parent>=MAXVIFS)
@@ -2120,6 +2120,7 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,intct;structrtnexthop*nhp;structnlattr*mp_attr;+structrta_mfc_statsmfcs;/* If cache is unresolved, don't try to parse IIF and OIF */if(c->mf6c_parent>=MAXMIFS)
From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:49
This patch advertise the MC_FORWARDING status for IPv4 and IPv6.
This field is readonly, only multicast engine in the kernel updates it.
Signed-off-by: Nicolas Dichtel <redacted>
---
include/linux/inetdevice.h | 3 +++
include/net/addrconf.h | 3 +++
include/uapi/linux/netconf.h | 1 +
net/ipv4/devinet.c | 10 ++++++++--
net/ipv4/ipmr.c | 12 ++++++++++++
net/ipv6/addrconf.c | 10 ++++++++--
net/ipv6/ip6mr.c | 20 ++++++++++++++++++--
7 files changed, 53 insertions(+), 6 deletions(-)
@@ -582,6 +583,9 @@ static int vif_delete(struct mr_table *mrt, int vifi, int notify,in_dev=__in_dev_get_rtnl(dev);if(in_dev){IPV4_DEVCONF(in_dev->cnf,MC_FORWARDING)--;+inet_netconf_notify_devconf(dev_net(dev),+NETCONFA_MC_FORWARDING,+dev->ifindex,&in_dev->cnf);ip_rt_multicast_event(in_dev);}
@@ -772,6 +776,8 @@ static int vif_add(struct net *net, struct mr_table *mrt,return-EADDRNOTAVAIL;}IPV4_DEVCONF(in_dev->cnf,MC_FORWARDING)++;+inet_netconf_notify_devconf(net,NETCONFA_MC_FORWARDING,dev->ifindex,+&in_dev->cnf);ip_rt_multicast_event(in_dev);/* Fill in the VIF structures */
@@ -469,6 +469,8 @@ static int inet6_netconf_msgsize_devconf(int type)/* type -1 is used for ALL */if(type==-1||type==NETCONFA_FORWARDING)size+=nla_total_size(4);+if(type==-1||type==NETCONFA_MC_FORWARDING)+size+=nla_total_size(4);returnsize;}
@@ -496,6 +498,10 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,if((type==-1||type==NETCONFA_FORWARDING)&&nla_put_s32(skb,NETCONFA_FORWARDING,devconf->forwarding)<0)gotonla_put_failure;+if((type==-1||type==NETCONFA_MC_FORWARDING)&&+nla_put_s32(skb,NETCONFA_MC_FORWARDING,+devconf->mc_forwarding)<0)+gotonla_put_failure;returnnlmsg_end(skb,nlh);
From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:50
A mfc entry can be static or not (added via the mroute_sk socket). The patch
reports MFC_STATIC flag into rtm_protocol by setting rtm_protocol to
RTPROT_STATIC or RTPROT_MROUTED.
Signed-off-by: Nicolas Dichtel <redacted>
---
include/uapi/linux/rtnetlink.h | 1 +
net/ipv4/ipmr.c | 5 ++++-
net/ipv6/ip6mr.c | 5 ++++-
3 files changed, 9 insertions(+), 2 deletions(-)
From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:50
This patch allows to monitor mf6c activities via rtnetlink.
To avoid parsing two times the mf6c oifs, we use maxvif to allocate the rtnl
msg, thus we may allocate some superfluous space.
Signed-off-by: Nicolas Dichtel <redacted>
---
net/ipv6/ip6mr.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 59 insertions(+), 4 deletions(-)
@@ -2231,13 +2240,13 @@ int ip6mr_get_route(struct net *net,}staticintip6mr_fill_mroute(structmr6_table*mrt,structsk_buff*skb,-u32portid,u32seq,structmfc6_cache*c)+u32portid,u32seq,structmfc6_cache*c,intcmd){structnlmsghdr*nlh;structrtmsg*rtm;interr;-nlh=nlmsg_put(skb,portid,seq,RTM_NEWROUTE,sizeof(*rtm),NLM_F_MULTI);+nlh=nlmsg_put(skb,portid,seq,cmd,sizeof(*rtm),NLM_F_MULTI);if(nlh==NULL)return-EMSGSIZE;
From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:50
/proc/net/ip[6]_mr_cache allows to get all mfc entries, even if they are put in
the unresolved list (mfc[6]_unres_queue). But only the table RT_TABLE_DEFAULT is
displayed.
This patch adds the parsing of the unresolved list when the dump is made via
rtnetlink, hence each table can be checked.
In IPv6, we set rtm_type in ip6mr_fill_mroute(), because in case of unresolved
mfc __ip6mr_fill_mroute() will not set it. In IPv4, it is already done.
Signed-off-by: Nicolas Dichtel <redacted>
---
net/ipv4/ipmr.c | 21 ++++++++++++++++++++-
net/ipv6/ip6mr.c | 22 +++++++++++++++++++++-
2 files changed, 41 insertions(+), 2 deletions(-)
@@ -2178,7 +2179,9 @@ static int ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,if(nla_put_be32(skb,RTA_SRC,c->mfc_origin)||nla_put_be32(skb,RTA_DST,c->mfc_mcastgrp))gotonla_put_failure;-if(__ipmr_fill_mroute(mrt,skb,c,rtm)<0)+err=__ipmr_fill_mroute(mrt,skb,c,rtm);+/* do not break the dump if cache is unresolved */+if(err<0&&err!=-ENOENT)gotonla_put_failure;returnnlmsg_end(skb,nlh);
@@ -2258,7 +2260,9 @@ static int ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,if(nla_put(skb,RTA_SRC,16,&c->mf6c_origin)||nla_put(skb,RTA_DST,16,&c->mf6c_mcastgrp))gotonla_put_failure;-if(__ip6mr_fill_mroute(mrt,skb,c,rtm)<0)+err=__ip6mr_fill_mroute(mrt,skb,c,rtm);+/* do not break the dump if cache is unresolved */+if(err<0&&err!=-ENOENT)gotonla_put_failure;returnnlmsg_end(skb,nlh);
From: Nicolas Dichtel <hidden> Date: 2012-12-04 11:13:51
This patch allows to monitor mfc activities via rtnetlink.
To avoid parsing two times the mfc oifs, we use maxvif to allocate the rtnl
msg, thus we may allocate some superfluous space.
Signed-off-by: Nicolas Dichtel <redacted>
---
net/ipv4/ipmr.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 59 insertions(+), 5 deletions(-)
From: David Miller <davem@davemloft.net> Date: 2012-12-04 18:09:16
From: Nicolas Dichtel <redacted>
Date: Tue, 4 Dec 2012 12:13:34 +0100
The goal of this serie is to be able to monitor multicast activities via
rtnetlink.
The main changes are:
- when user dumps mfc entries it now get all entries, included the unresolved
cache.
- kernel sends rtnetlink when it adds/deletes mfc entries.
As usual, the patch against iproute2 will be sent once the patches are included and
net-next merged. I can send it on demand.
This looks good, applied, thanks Nicolas.
The one thing I worry about are those 64-bit statistics. I fear that they
not be 64-bit aligned in the final netlink message. This matters on cpus
that trap on unaligned loads/stores, such as sparc and MIPS.
Can you validate this?
From: Nicolas Dichtel <hidden> Date: 2012-12-04 20:02:30
Le 04/12/2012 19:09, David Miller a écrit :
From: Nicolas Dichtel <redacted>
Date: Tue, 4 Dec 2012 12:13:34 +0100
quoted
The goal of this serie is to be able to monitor multicast activities via
rtnetlink.
The main changes are:
- when user dumps mfc entries it now get all entries, included the unresolved
cache.
- kernel sends rtnetlink when it adds/deletes mfc entries.
As usual, the patch against iproute2 will be sent once the patches are included and
net-next merged. I can send it on demand.
This looks good, applied, thanks Nicolas.
The one thing I worry about are those 64-bit statistics. I fear that they
not be 64-bit aligned in the final netlink message. This matters on cpus
that trap on unaligned loads/stores, such as sparc and MIPS.
Can you validate this?
I can have a try on a tile platform. I don't have access to sparc or mips.
From: Nicolas Dichtel <hidden> Date: 2012-12-05 11:02:54
Le 04/12/2012 21:02, Nicolas Dichtel a écrit :
Le 04/12/2012 19:09, David Miller a écrit :
quoted
From: Nicolas Dichtel <redacted>
Date: Tue, 4 Dec 2012 12:13:34 +0100
quoted
The goal of this serie is to be able to monitor multicast activities via
rtnetlink.
The main changes are:
- when user dumps mfc entries it now get all entries, included the unresolved
cache.
- kernel sends rtnetlink when it adds/deletes mfc entries.
As usual, the patch against iproute2 will be sent once the patches are
included and
net-next merged. I can send it on demand.
This looks good, applied, thanks Nicolas.
The one thing I worry about are those 64-bit statistics. I fear that they
not be 64-bit aligned in the final netlink message. This matters on cpus
that trap on unaligned loads/stores, such as sparc and MIPS.
Can you validate this?
I can have a try on a tile platform. I don't have access to sparc or mips.
Hmm, I've read arm instead of mips! So I've tried on mips. Data are aligned on
32-bit, like for all netlink messages. nla_put_u64() will do the same, as it
calls nla_put().
And the kernel will only use memcpy() to treat this attribute. Reader will be in
userland.
From: David Laight <hidden> Date: 2012-12-05 11:49:18
quoted
quoted
The one thing I worry about are those 64-bit statistics. I fear that they
not be 64-bit aligned in the final netlink message. This matters on cpus
that trap on unaligned loads/stores, such as sparc and MIPS.
Can you validate this?
I can have a try on a tile platform. I don't have access to sparc or mips.
Hmm, I've read arm instead of mips! So I've tried on mips. Data are aligned on
32-bit, like for all netlink messages. nla_put_u64() will do the same, as it
calls nla_put().
And the kernel will only use memcpy() to treat this attribute. Reader will be in
userland.
Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.
I think they are currently done with memcpy() - which should be ok.
It might be possibly to optimise by using a structure containing
a 64bit value marked __attribute__((aligned(4))).
David
From: David Miller <davem@davemloft.net> Date: 2012-12-05 17:53:27
From: Nicolas Dichtel <redacted>
Date: Wed, 05 Dec 2012 12:02:50 +0100
Le 04/12/2012 21:02, Nicolas Dichtel a écrit :
quoted
I can have a try on a tile platform. I don't have access to sparc or
mips.
Hmm, I've read arm instead of mips! So I've tried on mips. Data are
aligned on 32-bit, like for all netlink messages. nla_put_u64() will
do the same, as it calls nla_put().
And the kernel will only use memcpy() to treat this attribute. Reader
will be in userland.
Then userland will trap if the 64-bit values are only 32-bit aligned.
That's the problem I'm talking about.
I don't want to export any more unaligned 64-bit values in netlink
messages, it's a complete mess.
Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.
Rather, let's not create this situation at all.
It's totally inappropriate to have special code to handle every single
time we want to put 64-bit values into netlink messages.
We need a real solution to this issue.
Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.
Rather, let's not create this situation at all.
It's totally inappropriate to have special code to handle every single
time we want to put 64-bit values into netlink messages.
We need a real solution to this issue.
The easiest way is to update *_ALIGNTO values (maybe we can keep NLMSG_ALIGNTO
to 4). But I think that many userland apps have these values hardcoded and, the
most important thing, this may increase size of many netlink messages. Hence we
need probably to find something better.
Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.
Rather, let's not create this situation at all.
It's totally inappropriate to have special code to handle every single
time we want to put 64-bit values into netlink messages.
We need a real solution to this issue.
The easiest way is to update *_ALIGNTO values (maybe we can keep
NLMSG_ALIGNTO to 4). But I think that many userland apps have these
values hardcoded and, the most important thing, this may increase
size of many netlink messages. Hence we need probably to find
something better.
We can't do this, as you say, ALIGNTO is compiled into all the
binaries.
A simple backwards compatible workaround would be to include an
unknown, empty padding attribute if needed. That would be 4 bytes
in size and could be used to include padding as needed.
We could use nla_type = 0 as it is a reserved value that should
be available in all protocols. All readers (kernel and user space)
must ignore such an attribute just like any other unknown
attribute they encounter.
We could easily extend nla_put_u64() and variants to automatically
include such a padding attribute as needed.
The only situation that I can think of where this would not work
is if we have code like this:
foo = nla_nest_start();
for ([..])
nla_put_u64([...])
nla_nest_end([...])
and a reader would stupidly do a nla_for_each_attr() in user space
and assume all attributes found must be NLA_U64 without even
checking the length of the attribute.
I would say we take that risk and let such code die horribly.
Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.
Rather, let's not create this situation at all.
It's totally inappropriate to have special code to handle every single
time we want to put 64-bit values into netlink messages.
We need a real solution to this issue.
The easiest way is to update *_ALIGNTO values (maybe we can keep
NLMSG_ALIGNTO to 4). But I think that many userland apps have these
values hardcoded and, the most important thing, this may increase
size of many netlink messages. Hence we need probably to find
something better.
We can't do this, as you say, ALIGNTO is compiled into all the
binaries.
A simple backwards compatible workaround would be to include an
unknown, empty padding attribute if needed. That would be 4 bytes
in size and could be used to include padding as needed.
We could use nla_type = 0 as it is a reserved value that should
be available in all protocols. All readers (kernel and user space)
must ignore such an attribute just like any other unknown
attribute they encounter.
We could easily extend nla_put_u64() and variants to automatically
include such a padding attribute as needed.
The only situation that I can think of where this would not work
is if we have code like this:
foo = nla_nest_start();
for ([..])
nla_put_u64([...])
nla_nest_end([...])
and a reader would stupidly do a nla_for_each_attr() in user space
and assume all attributes found must be NLA_U64 without even
checking the length of the attribute.
I would say we take that risk and let such code die horribly.
Ok, I can work to a patch next week if you want (I will be off until Tuesday).
Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.
Rather, let's not create this situation at all.
It's totally inappropriate to have special code to handle every single
time we want to put 64-bit values into netlink messages.
We need a real solution to this issue.
The easiest way is to update *_ALIGNTO values (maybe we can keep
NLMSG_ALIGNTO to 4). But I think that many userland apps have these
values hardcoded and, the most important thing, this may increase
size of many netlink messages. Hence we need probably to find
something better.
We can't do this, as you say, ALIGNTO is compiled into all the
binaries.
A simple backwards compatible workaround would be to include an
unknown, empty padding attribute if needed. That would be 4 bytes
in size and could be used to include padding as needed.
What are you going to align the data with respect to?
I doubt you can assume that the start of the netlink
message itself is 8 byte aligned - so any attempt
to 8 byte align an item is probably doomed to failure.
David
From: Thomas Graf <tgraf@suug.ch> Date: 2012-12-07 10:58:12
On 12/07/12 at 10:38am, David Laight wrote:
What are you going to align the data with respect to?
I doubt you can assume that the start of the netlink
message itself is 8 byte aligned - so any attempt
to 8 byte align an item is probably doomed to failure.
Correct me if I'm wrong but skb->head will be aligned to
SKB_DATA_ALIGN() which as I understand is guaranted to
be 8 bytse aligned. So we can just add needed padding if
skb->data would not be aligned correctly after the next
netlink attribute header was added.
All user space needs to do is use a receive buffer that
is 8 byte aligned as well and we are good to go.
Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.
Rather, let's not create this situation at all.
It's totally inappropriate to have special code to handle every single
time we want to put 64-bit values into netlink messages.
We need a real solution to this issue.
The easiest way is to update *_ALIGNTO values (maybe we can keep
NLMSG_ALIGNTO to 4). But I think that many userland apps have these
values hardcoded and, the most important thing, this may increase
size of many netlink messages. Hence we need probably to find
something better.
We can't do this, as you say, ALIGNTO is compiled into all the
binaries.
A simple backwards compatible workaround would be to include an
unknown, empty padding attribute if needed. That would be 4 bytes
in size and could be used to include padding as needed.
We could use nla_type = 0 as it is a reserved value that should
be available in all protocols. All readers (kernel and user space)
must ignore such an attribute just like any other unknown
attribute they encounter.
We could easily extend nla_put_u64() and variants to automatically
include such a padding attribute as needed.
In fact, it seems not so easy because most users of nlmsg_new() calculate
the exact needed length, thus if we add an unpredicted attribute, the message
will be too small.
From: Thomas Graf <tgraf@suug.ch> Date: 2012-12-11 18:40:20
On 12/11/12 at 04:03pm, Nicolas Dichtel wrote:
In fact, it seems not so easy because most users of nlmsg_new() calculate
the exact needed length, thus if we add an unpredicted attribute,
the message will be too small.
True, we would either need to fix the calculations by accounting
for an additional 4 bytes for each 64bit arg or just reserve an
additional fixed amount for padding per message in nlmsg_new().
From: Nicolas Dichtel <hidden> Date: 2012-12-12 17:30:55
Le 11/12/2012 19:40, Thomas Graf a écrit :
On 12/11/12 at 04:03pm, Nicolas Dichtel wrote:
quoted
In fact, it seems not so easy because most users of nlmsg_new() calculate
the exact needed length, thus if we add an unpredicted attribute,
the message will be too small.
True, we would either need to fix the calculations by accounting
for an additional 4 bytes for each 64bit arg or just reserve an
additional fixed amount for padding per message in nlmsg_new().
I would say that reserving additional space is better, because we also
need to align attributes that contain u64 fields:
struct attribute_foo {
__u32 bar;
__u32 bar2;
__u64 foo;
};
I wonder if it is better to align all attribute on 64-bits or only u64 and
add a new function nla_put_align64() for attribute with u64 fields.
From: Nicolas Dichtel <hidden> Date: 2012-12-14 13:19:43
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 <redacted>
---
include/net/netlink.h | 9 +++++++++
lib/nlattr.c | 11 ++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
@@ -492,6 +492,15 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,*/staticinlinestructsk_buff*nlmsg_new(size_tpayload,gfp_tflags){+/* Because attributes may be aligned on 64-bits boundary with fake+*attribute(type0,size4(attributesare32-bitsalignbydefault)),+*anexactpayloadsizecannotbecalculated.Hence,weneedtoreserve+*morespacefortheseattributes.+*128isarbitrary:itallowstoalignupto32attributes.+*/+if(sizeof(void*)>4&&payload<NLMSG_DEFAULT_SIZE)+payload=min(payload+128,(size_t)NLMSG_DEFAULT_SIZE);+returnalloc_skb(nlmsg_total_size(payload),flags);}
@@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);*/intnla_put(structsk_buff*skb,intattrtype,intattrlen,constvoid*data){-if(unlikely(skb_tailroom(skb)<nla_total_size(attrlen)))+intalign=IS_ALIGNED((unsignedlong)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_HDRLENis4,hencepayloadis0.+*/+__nla_reserve(skb,0,0);+}+__nla_put(skb,attrtype,attrlen,data);return0;}
From: Ben Hutchings <hidden> Date: 2012-12-14 15:49:26
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.
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;
}
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Nicolas Dichtel <hidden> Date: 2012-12-14 16:04:04
Le 14/12/2012 16:49, Ben Hutchings a écrit :
On Fri, 2012-12-14 at 14:16 +0100, Nicolas Dichtel wrote:
quoted
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.
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
From: David Laight <hidden> Date: 2012-12-17 10:09:27
- 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
From: Nicolas Dichtel <hidden> Date: 2012-12-17 16:53:51
Le 17/12/2012 10:59, David Laight a écrit :
quoted
- 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
From: Nicolas Dichtel <hidden> Date: 2012-12-17 16:53:56
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 <redacted>
---
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(-)
@@ -492,6 +492,15 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,*/staticinlinestructsk_buff*nlmsg_new(size_tpayload,gfp_tflags){+/* Because attributes may be aligned on 64-bits boundary with fake+*attribute(type0,size4(attributesare32-bitsalignbydefault)),+*anexactpayloadsizecannotbecalculated.Hence,weneedtoreserve+*morespacefortheseattributes.+*128isarbitrary:itallowstoalignupto32attributes.+*/+if(payload<NLMSG_DEFAULT_SIZE)+payload=min(payload+128,(size_t)NLMSG_DEFAULT_SIZE);+returnalloc_skb(nlmsg_total_size(payload),flags);}
@@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);*/intnla_put(structsk_buff*skb,intattrtype,intattrlen,constvoid*data){-if(unlikely(skb_tailroom(skb)<nla_total_size(attrlen)))+intalign=IS_ALIGNED((unsignedlong)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_HDRLENis4,hencepayloadis0.+*/+__nla_reserve(skb,0,0);+}+__nla_put(skb,attrtype,attrlen,data);return0;}
From: David Laight <hidden> Date: 2012-12-17 17:12:42
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
From: Nicolas Dichtel <hidden> Date: 2012-12-17 17:35:29
Le 17/12/2012 18:06, David Laight a écrit :
quoted
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?
...
quoted
+ 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...
From: David Laight <hidden> Date: 2012-12-18 09:21:40
Le 17/12/2012 18:06, David Laight a écrit :
quoted
quoted
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.
quoted
...
quoted
+ 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
From: Nicolas Dichtel <hidden> Date: 2012-12-18 10:46:24
Le 18/12/2012 10:19, David Laight a écrit :
quoted
Le 17/12/2012 18:06, David Laight a écrit :
quoted
quoted
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)?
quoted
quoted
...
quoted
+ 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.
quoted
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.
From: Thomas Graf <tgraf@suug.ch> Date: 2012-12-18 12:57:43
On 12/17/12 at 05:49pm, Nicolas Dichtel wrote:
quoted hunk
@@ -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.
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?
@@ -492,6 +491,14 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,*/staticinlinestructsk_buff*nlmsg_new(size_tpayload,gfp_tflags){+/* If an exact size if specified, reserve some additional space to+*alignthefirstattribute,allsubsequentattributesshouldhave+*paddingaccountedfor.+*/+if(payload!=NLMSG_DEFAULT_SIZE)+payload=min_t(size_t,payload+NLA_ATTR_ALIGN,+NLMSG_DEFAULT_SIZE);+returnalloc_skb(nlmsg_total_size(payload),flags);}
@@ -653,16 +660,12 @@ static inline int nla_attr_size(int payload)*/staticinlineintnla_total_size(intpayload){-returnNLA_ALIGN(nla_attr_size(payload));-}+size_tlen=NLA_ALIGN(nla_attr_size(payload));-/**-*nla_padlen-lengthofpaddingatthetailofattribute-*@payload:lengthofpayload-*/-staticinlineintnla_padlen(intpayload)-{-returnnla_total_size(payload)-nla_attr_size(payload);+if(!IS_ALIGNED(len,NLA_ATTR_ALIGN))+len=ALIGN(len+NLA_HDRLEN,NLA_ATTR_ALIGN);++returnlen;}/**
From: Nicolas Dichtel <hidden> Date: 2012-12-18 16:29:37
Le 18/12/2012 13:57, Thomas Graf a écrit :
On 12/17/12 at 05:49pm, Nicolas Dichtel wrote:
quoted
@@ -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.
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.
Good point.
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?
I still have some doubts about the size calculation (see bellow).
For the rest of the patch, it seems ok (except some minor point). I will test it.
@@ -492,6 +491,14 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,*/staticinlinestructsk_buff*nlmsg_new(size_tpayload,gfp_tflags){+/* If an exact size if specified, reserve some additional space to+*alignthefirstattribute,allsubsequentattributesshouldhave+*paddingaccountedfor.+*/+if(payload!=NLMSG_DEFAULT_SIZE)+payload=min_t(size_t,payload+NLA_ATTR_ALIGN,+NLMSG_DEFAULT_SIZE);+returnalloc_skb(nlmsg_total_size(payload),flags);}
@@ -653,16 +660,12 @@ static inline int nla_attr_size(int payload)*/staticinlineintnla_total_size(intpayload){-returnNLA_ALIGN(nla_attr_size(payload));-}+size_tlen=NLA_ALIGN(nla_attr_size(payload));-/**-*nla_padlen-lengthofpaddingatthetailofattribute-*@payload:lengthofpayload-*/-staticinlineintnla_padlen(intpayload)-{-returnnla_total_size(payload)-nla_attr_size(payload);+if(!IS_ALIGNED(len,NLA_ATTR_ALIGN))+len=ALIGN(len+NLA_HDRLEN,NLA_ATTR_ALIGN);
Two comments:
1/ should it be ALIGN(len, NLA_ATTR_ALIGN)? If we want to add a __u64:
=> nla_attr_size(sizeof(__u64)) = 12
=> NLA_ALIGN(nla_attr_size(sizeof(__u64))) => 12 (= len)
=> ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN) = 0 but it should be 4
2/ Suppose that the attribute is:
struct foo {
__u64 bar1;
__u32 bar2;
}
=> sizeof(struct foo) = 12 (= payload)
=> nla_attr_size(payload) = 16
=> NLA_ALIGN(nla_attr_size(payload)) = 16 (= len)
=> IS_ALIGNED(len, NLA_ATTR_ALIGN) = true
=> extra room is not reserved
But it's not guaranteed that bar1 is aligned on 8 bytes, only on 4 bytes.
With the previous struct foo, this test may be true even if we don't have
reserved extra room. This test depends on previous attribute.
I think the exact size of the netlink message depends on the order of
attributes, not only on the attribute itself.
What about taking the assumption that the start will never be aligned and always
allocating extra room: ALIGN(NLA_ALIGNTO, NLA_ATTR_ALIGN) (= 4)?
From: David Laight <hidden> Date: 2012-12-18 16:58:56
2/ Suppose that the attribute is:
struct foo {
__u64 bar1;
__u32 bar2;
}
=> sizeof(struct foo) = 12 (= payload)
That is only true if the host architecture aligns 64bit items
on 32 it boundaries (as i386 does).
Otherwise there are 4 bytes of padding at the end and the
size is 16.
Actually it is worse than that.
Consider the structure:
struct bar {
__u32 foo1;
__u64 foo2;
}
On i386 it will have size 12 and foo2 will be at offset 4.
On sparc32 (and most 64bit) it will have size 16 with foo2
at offset 8 (and 4 bytes of pad after foo1).
Do these messages move between systems?
If they do then any 64bit items need an explicit alignment
eg tag with __attribute__((aligned(8))) (or aligned(4)).
David
From: Thomas Graf <tgraf@suug.ch> Date: 2012-12-18 17:08:58
On 12/18/12 at 05:23pm, Nicolas Dichtel wrote:
Le 18/12/2012 13:57, Thomas Graf a écrit :
quoted
-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);
Two comments:
1/ should it be ALIGN(len, NLA_ATTR_ALIGN)? If we want to add a __u64:
=> nla_attr_size(sizeof(__u64)) = 12
=> NLA_ALIGN(nla_attr_size(sizeof(__u64))) => 12 (= len)
=> ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN) = 0 but it should be 4
We can't add 1-3 bytes of padding, therefore we need to add
NLA_HDRLEN to len before aligning it to enforce a minimal
padding. We can't hit it right now because 4 byte alignment
of the previous attribute is a given but if we ever change
the alignment it could become an issue and the above should
be bullet proof.
Your example would come out like this:
nla_attr_size(8) = 12
ALIGN(12 + 4, 8) = 16
2/ Suppose that the attribute is:
struct foo {
__u64 bar1;
__u32 bar2;
}
=> sizeof(struct foo) = 12 (= payload)
=> nla_attr_size(payload) = 16
=> NLA_ALIGN(nla_attr_size(payload)) = 16 (= len)
=> IS_ALIGNED(len, NLA_ATTR_ALIGN) = true
=> extra room is not reserved
But it's not guaranteed that bar1 is aligned on 8 bytes, only on 4 bytes.
That's correct, that's why I have added the additional
NLA_ATTR_ALIGN of room in nlmsg_new(). It will account
for the one time padding that is needed before we add
the very first attribute.
If all attributes after that have a size aligned to 8
bytes no padding is needed. Padding will only be needed
again if a struct is missized in which case we reserve
room with the above. Correct?
With the previous struct foo, this test may be true even if we don't
have reserved extra room. This test depends on previous attribute.
I think the exact size of the netlink message depends on the order
of attributes, not only on the attribute itself.
What about taking the assumption that the start will never be
aligned and always allocating extra room: ALIGN(NLA_ALIGNTO,
NLA_ATTR_ALIGN) (= 4)?
See my explanation above. I think this works. The order does not
matter, the sum of all padding required will always be the same.
If nla_total_size() was right, nla_pre_padlen(skb) should already be
included. Am I wrong?
No, nla_pre_padlen() contains the number of bytes needed to align
skb_tail_pointer() to an alignment of 8. If that is > 0 but the
attribute to follow is already aligned.
The tricky part here is that accounting for padding in
nla_total_size() only works for the sum of all attributes.
It does not account for the specific padding required for the
previous attribute.
Therefore the above check. The above could be changed to
nla_attr_size() theoretically as we don't need space for the
final padding eventually but we checked for space before so I
kept it that way.
I realize it's slightly confusign and needs better documentation
and please double check my thinking :-)
From: Thomas Graf <tgraf@suug.ch> Date: 2012-12-18 17:11:05
On 12/18/12 at 04:50pm, David Laight wrote:
quoted
2/ Suppose that the attribute is:
struct foo {
__u64 bar1;
__u32 bar2;
}
=> sizeof(struct foo) = 12 (= payload)
That is only true if the host architecture aligns 64bit items
on 32 it boundaries (as i386 does).
Otherwise there are 4 bytes of padding at the end and the
size is 16.
Actually it is worse than that.
Consider the structure:
struct bar {
__u32 foo1;
__u64 foo2;
}
On i386 it will have size 12 and foo2 will be at offset 4.
On sparc32 (and most 64bit) it will have size 16 with foo2
at offset 8 (and 4 bytes of pad after foo1).
This is a known problem and I can't think of anything
that can be done about it except for memcpy()ing the
data before accessing it.
If you have ideas, I'm more that willing to listen :)
Do these messages move between systems?
If they do then any 64bit items need an explicit alignment
eg tag with __attribute__((aligned(8))) (or aligned(4)).
They don't. Netlink has and will be host bound. It also
uses host byte order for that reason.
From: Nicolas Dichtel <hidden> Date: 2012-12-18 22:07:31
Le 18/12/2012 18:08, Thomas Graf a écrit :
On 12/18/12 at 05:23pm, Nicolas Dichtel wrote:
quoted
Le 18/12/2012 13:57, Thomas Graf a écrit :
quoted
-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);
Two comments:
1/ should it be ALIGN(len, NLA_ATTR_ALIGN)? If we want to add a __u64:
=> nla_attr_size(sizeof(__u64)) = 12
=> NLA_ALIGN(nla_attr_size(sizeof(__u64))) => 12 (= len)
=> ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN) = 0 but it should be 4
We can't add 1-3 bytes of padding, therefore we need to add
NLA_HDRLEN to len before aligning it to enforce a minimal
padding. We can't hit it right now because 4 byte alignment
of the previous attribute is a given but if we ever change
the alignment it could become an issue and the above should
be bullet proof.
Your example would come out like this:
nla_attr_size(8) = 12
ALIGN(12 + 4, 8) = 16
Got it, right.
quoted
2/ Suppose that the attribute is:
struct foo {
__u64 bar1;
__u32 bar2;
}
=> sizeof(struct foo) = 12 (= payload)
=> nla_attr_size(payload) = 16
=> NLA_ALIGN(nla_attr_size(payload)) = 16 (= len)
=> IS_ALIGNED(len, NLA_ATTR_ALIGN) = true
=> extra room is not reserved
But it's not guaranteed that bar1 is aligned on 8 bytes, only on 4 bytes.
That's correct, that's why I have added the additional
NLA_ATTR_ALIGN of room in nlmsg_new(). It will account
for the one time padding that is needed before we add
the very first attribute.
If all attributes after that have a size aligned to 8
bytes no padding is needed. Padding will only be needed
again if a struct is missized in which case we reserve
room with the above. Correct?
With the previous struct foo, this test may be true even if we don't
have reserved extra room. This test depends on previous attribute.
I think the exact size of the netlink message depends on the order
of attributes, not only on the attribute itself.
What about taking the assumption that the start will never be
aligned and always allocating extra room: ALIGN(NLA_ALIGNTO,
NLA_ATTR_ALIGN) (= 4)?
See my explanation above. I think this works. The order does not
matter, the sum of all padding required will always be the same.
If nla_total_size() was right, nla_pre_padlen(skb) should already be
included. Am I wrong?
No, nla_pre_padlen() contains the number of bytes needed to align
skb_tail_pointer() to an alignment of 8. If that is > 0 but the
attribute to follow is already aligned.
The tricky part here is that accounting for padding in
nla_total_size() only works for the sum of all attributes.
It does not account for the specific padding required for the
previous attribute.
Therefore the above check. The above could be changed to
nla_attr_size() theoretically as we don't need space for the
final padding eventually but we checked for space before so I
kept it that way.
I realize it's slightly confusign and needs better documentation
and please double check my thinking :-)
From: David Laight <hidden> Date: 2012-12-19 09:25:23
quoted
Consider the structure:
struct bar {
__u32 foo1;
__u64 foo2;
}
On i386 it will have size 12 and foo2 will be at offset 4.
On sparc32 (and most 64bit) it will have size 16 with foo2
at offset 8 (and 4 bytes of pad after foo1).
This is a known problem and I can't think of anything
that can be done about it except for memcpy()ing the
data before accessing it.
You can't use memcpy() to copy a pointer to a misaligned
structure into an aligned buffer. The compiler assumes
the pointer is aligned and will use instructions that
depend on the alignment.
If you have ideas, I'm more that willing to listen :)
... Netlink has and will be host bound. It also
uses host byte order for that reason.
I think:
1) Alignment is only needed on systems that have 'strict alignment'
requirements (maybe disable for testing?)
2) Alignment is only needed for parameters whose size is a
multiple of the alignment (a structure containing a
field that needs 8 byte alignment will always be a multiple
of 8 bytes long).
3) You need to add NA_HDR_LEN to the write pointer before
determining the size of the pad.
So a structure of three uint32_t will never need aligning.
David
From: Nicolas Dichtel <hidden> Date: 2012-12-19 11:22:40
Le 18/12/2012 13:57, Thomas Graf a écrit :
quoted hunk
On 12/17/12 at 05:49pm, Nicolas Dichtel wrote:
quoted
@@ -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.
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?
@@ -492,6 +491,14 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,*/staticinlinestructsk_buff*nlmsg_new(size_tpayload,gfp_tflags){+/* If an exact size if specified, reserve some additional space to+*alignthefirstattribute,allsubsequentattributesshouldhave+*paddingaccountedfor.+*/+if(payload!=NLMSG_DEFAULT_SIZE)+payload=min_t(size_t,payload+NLA_ATTR_ALIGN,+NLMSG_DEFAULT_SIZE);+returnalloc_skb(nlmsg_total_size(payload),flags);}
@@ -653,16 +660,12 @@ static inline int nla_attr_size(int payload)*/staticinlineintnla_total_size(intpayload){-returnNLA_ALIGN(nla_attr_size(payload));-}+size_tlen=NLA_ALIGN(nla_attr_size(payload));-/**-*nla_padlen-lengthofpaddingatthetailofattribute-*@payload:lengthofpayload-*/-staticinlineintnla_padlen(intpayload)-{-returnnla_total_size(payload)-nla_attr_size(payload);+if(!IS_ALIGNED(len,NLA_ATTR_ALIGN))+len=ALIGN(len+NLA_HDRLEN,NLA_ATTR_ALIGN);++returnlen;}/**
@@ -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.
*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.
From: Thomas Graf <tgraf@suug.ch> Date: 2012-12-19 17:20:16
On 12/19/12 at 09:17am, David Laight wrote:
You can't use memcpy() to copy a pointer to a misaligned
structure into an aligned buffer. The compiler assumes
the pointer is aligned and will use instructions that
depend on the alignment.
I am not sure I understand this correctly. Are you saying
that the following does not work on i386?
struct foo {
uint32_t a;
uint64_t b;
};
struct foo buf;
memcpy(&buf, nla_data(attr), nla_len(attr));
printf([...], buf.b);
I think:
1) Alignment is only needed on systems that have 'strict alignment'
requirements (maybe disable for testing?)
Right, what about mixed 32bit/64bit environments?
2) Alignment is only needed for parameters whose size is a
multiple of the alignment (a structure containing a
field that needs 8 byte alignment will always be a multiple
of 8 bytes long).
Good point. I'll fix this in the next iteration of the patch.
3) You need to add NA_HDR_LEN to the write pointer before
determining the size of the pad.
Right, I'm doing this in the patch I proposed. Or are you referring
to something else?
*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.
From: David Laight <hidden> Date: 2012-12-20 09:46:43
On 12/19/12 at 09:17am, David Laight wrote:
quoted
You can't use memcpy() to copy a pointer to a misaligned
structure into an aligned buffer. The compiler assumes
the pointer is aligned and will use instructions that
depend on the alignment.
I am not sure I understand this correctly. Are you saying
that the following does not work on i386?
struct foo {
uint32_t a;
uint64_t b;
};
struct foo buf;
memcpy(&buf, nla_data(attr), nla_len(attr));
printf([...], buf.b);
That will be fine on all systems.
But if, instead, you have:
struct foo buf, *bufp;
bufp = nla_data(attr);
memcpy(&buf, bufp, sizeof buf);
The compiler is allowed to assume that 'bufp' is aligned,
so the copy will be done using 64bit accesses.
(Basically because all you are allowed to do with 'void *'
is cast a point to 'void *', then back to its original type.
So when you cast back from 'void *' the pointer can be assumed
to be aligned.)
This will fault on systems that require strict alignment
of 64bit items.
David
From: David Laight <hidden> Date: 2012-12-20 09:46:43
quoted
I think:
1) Alignment is only needed on systems that have 'strict alignment'
requirements (maybe disable for testing?)
Right, what about mixed 32bit/64bit environments?
Support for i386 user binaries on amd64 kernels
is an entirely different problem!
That, typically, requires the kernel to know that
the application is 32bit and use separate structures
where the 64bit items have the aligned(32) attribute.
David