| Message ID | 20160419.195009.1052027353987244150.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 4/19/16, 4:50 PM, David Miller wrote: > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Tue, 19 Apr 2016 21:08:21 +0200 > >> Le 19/04/2016 20:47, Eric Dumazet a écrit : >>> Since we want to use this in other places, we could define a helper. >>> >>> nla_align_64bit(skb, attribute) or something. >> Yes, with the corresponding nla_total_size_64bit() > Good, idea, committed the following: > > Roopa, please use these helpers in your RTM_GETSTATS patch. will do. > > Thank you. > > ==================== > [PATCH] net: Add helpers for 64-bit aligning netlink attributes. > > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com> > Suggested-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > these look really nice. Thanks!
Here is a proposal to add more helpers in the libnetlink to manage 64-bit alignment issues. Note that this series was only tested on x86. The first patch is a fix (bug seen by code review only unless I've missed something). The second patch adds helpers and uses it for IFLA_STATS64. The last two patches use the new API to align mcast stats. We could also add helpers for nla_put_u64() and its variants. include/net/netlink.h | 10 +++- include/uapi/linux/rtnetlink.h | 1 + lib/nlattr.c | 107 +++++++++++++++++++++++++++++++++++++++++ net/core/rtnetlink.c | 9 +--- net/ipv4/ipmr.c | 4 +- net/ipv6/ip6mr.c | 4 +- 6 files changed, 123 insertions(+), 12 deletions(-) Comments are welcomed, Regards, Nicolas
Here is a proposal to add more helpers in the libnetlink to manage 64-bit alignment issues. Note that this series was only tested on x86 by tweeking CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and adding some traces. The first patch adds helpers for 64bit alignment and other patches use them. We could also add helpers for nla_put_u64() and its variants if needed. v1 -> v2: - remove patch #1 - split patch #2 (now #1 and #2) - add nla_need_padding_for_64bit() include/net/netlink.h | 39 +++++++++++++---- include/uapi/linux/rtnetlink.h | 1 + lib/nlattr.c | 99 ++++++++++++++++++++++++++++++++++++++++++ net/core/rtnetlink.c | 22 +++------- net/ipv4/ipmr.c | 4 +- net/ipv6/ip6mr.c | 4 +- 6 files changed, 140 insertions(+), 29 deletions(-) Comments are welcomed, Regards, Nicolas
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Thu, 21 Apr 2016 18:58:23 +0200 > Here is a proposal to add more helpers in the libnetlink to manage 64-bit > alignment issues. > Note that this series was only tested on x86 by tweeking > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and adding some traces. > > The first patch adds helpers for 64bit alignment and other patches > use them. > > We could also add helpers for nla_put_u64() and its variants if needed. > > v1 -> v2: > - remove patch #1 > - split patch #2 (now #1 and #2) > - add nla_need_padding_for_64bit() I like it, nice work Nicolas. Applied to net-next. I did a quick scan and the following jumped out at me as cases we need to fix up as well: 1) xfrm_user 2) tcp_info 3) taskstats 4) pkt_{cls,sched} 5) openvswitch etc. Most of these are statistic cases just like all of the existing ones we have fixed so far.
Le 21/04/2016 20:28, David Miller a écrit : > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Thu, 21 Apr 2016 18:58:23 +0200 > >> Here is a proposal to add more helpers in the libnetlink to manage 64-bit >> alignment issues. >> Note that this series was only tested on x86 by tweeking >> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and adding some traces. >> >> The first patch adds helpers for 64bit alignment and other patches >> use them. >> >> We could also add helpers for nla_put_u64() and its variants if needed. >> >> v1 -> v2: >> - remove patch #1 >> - split patch #2 (now #1 and #2) >> - add nla_need_padding_for_64bit() > > I like it, nice work Nicolas. Thank you. > > Applied to net-next. > > I did a quick scan and the following jumped out at me as cases we need > to fix up as well: Did you grep something or just catch this by code review? > > 1) xfrm_user > 2) tcp_info > 3) taskstats > 4) pkt_{cls,sched} > 5) openvswitch > etc. > > Most of these are statistic cases just like all of the existing ones > we have fixed so far. Yes, I will follow on this topic. There are also a bunch of nla_put_[u|be|le]64(): $ git grep -w "nla_put_.\{1,2\}64" net/ | wc -l 118 $ git grep -w "nla_put_.\{1,2\}64" | wc -l 172
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Fri, 22 Apr 2016 00:00:09 +0200 > Le 21/04/2016 20:28, David Miller a écrit : >> >> Applied to net-next. >> >> I did a quick scan and the following jumped out at me as cases we need >> to fix up as well: > Did you grep something or just catch this by code review? It was a "grep and check" kind of thing, yes.
==================== [PATCH] net: Add helpers for 64-bit aligning netlink attributes. Suggested-by: Eric Dumazet <eric.dumazet@gmail.com> Suggested-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Signed-off-by: David S. Miller <davem@davemloft.net> --- include/net/netlink.h | 37 +++++++++++++++++++++++++++++++++++++ net/core/rtnetlink.c | 24 +++++------------------- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/include/net/netlink.h b/include/net/netlink.h index 0e31727..e644b34 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -1231,6 +1231,43 @@ static inline int nla_validate_nested(const struct nlattr *start, int maxtype, } /** + * nla_align_64bit - 64-bit align the nla_data() of next attribute + * @skb: socket buffer the message is stored in + * @padattr: attribute type for the padding + * + * Conditionally emit a padding netlink attribute in order to make + * the next attribute we emit have a 64-bit aligned nla_data() area. + * This will only be done in architectures which do not have + * HAVE_EFFICIENT_UNALIGNED_ACCESS defined. + * + * Returns zero on success or a negative error code. + */ +static inline int nla_align_64bit(struct sk_buff *skb, int padattr) +{ +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS + if (IS_ALIGNED((unsigned long)skb->data, 8)) { + struct nlattr *attr = nla_reserve(skb, padattr, 0); + if (!attr) + return -EMSGSIZE; + } +#endif + return 0; +} + +/** + * nla_total_size_64bit - total length of attribute including padding + * @payload: length of payload + */ +static inline int nla_total_size_64bit(int payload) +{ + return NLA_ALIGN(nla_attr_size(payload)) +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS + + NLA_ALIGN(nla_attr_size(0)) +#endif + ; +} + +/** * nla_for_each_attr - iterate over a stream of attributes * @pos: loop counter, set to current attribute * @head: head of attribute stream diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 198ca2c..d3694a1 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -878,10 +878,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev, + nla_total_size(IFNAMSIZ) /* IFLA_QDISC */ + nla_total_size(sizeof(struct rtnl_link_ifmap)) + nla_total_size(sizeof(struct rtnl_link_stats)) -#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS - + nla_total_size(0) /* IFLA_PAD */ -#endif - + nla_total_size(sizeof(struct rtnl_link_stats64)) + + nla_total_size_64bit(sizeof(struct rtnl_link_stats64)) + nla_total_size(MAX_ADDR_LEN) /* IFLA_ADDRESS */ + nla_total_size(MAX_ADDR_LEN) /* IFLA_BROADCAST */ + nla_total_size(4) /* IFLA_TXQLEN */ @@ -1054,22 +1051,11 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb, { struct rtnl_link_stats64 *sp; struct nlattr *attr; + int err; -#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS - /* IF necessary, add a zero length NOP attribute so that the - * nla_data() of the IFLA_STATS64 will be 64-bit aligned. - * - * The nlattr header is 4 bytes in size, that's why we test - * if the skb->data _is_ aligned. This NOP attribute, plus - * nlattr header for IFLA_STATS64, will make nla_data() 8-byte - * aligned. - */ - if (IS_ALIGNED((unsigned long)skb->data, 8)) { - attr = nla_reserve(skb, IFLA_PAD, 0); - if (!attr) - return -EMSGSIZE; - } -#endif + err = nla_align_64bit(skb, IFLA_PAD); + if (err) + return err; attr = nla_reserve(skb, IFLA_STATS64, sizeof(struct rtnl_link_stats64));