[PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch adds a new RTM_GETSTATS message to query link stats via netlink
from the kernel. RTM_NEWLINK also dumps stats today, but RTM_NEWLINK
returns a lot more than just stats and is expensive in some cases when
frequent polling for stats from userspace is a common operation.

RTM_GETSTATS is an attempt to provide a light weight netlink message
to explicity query only link stats from the kernel on an interface.
The idea is to also keep it extensible so that new kinds of stats can be
added to it in the future.

This patch adds the following attribute for NETDEV stats:
struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
        [IFLA_STATS_LINK_64]  = { .len = sizeof(struct rtnl_link_stats64) },
};

Like any other rtnetlink message, RTM_GETSTATS can be used to get stats of
a single interface or all interfaces with NLM_F_DUMP.

Future possible new types of stat attributes:
link af stats:
    - IFLA_STATS_LINK_IPV6  (nested. for ipv6 stats)
    - IFLA_STATS_LINK_MPLS  (nested. for mpls/mdev stats)
extended stats:
    - IFLA_STATS_LINK_EXTENDED (nested. extended software netdev stats like bridge,
      vlan, vxlan etc)
    - IFLA_STATS_LINK_HW_EXTENDED (nested. extended hardware stats which are
      available via ethtool today)

This patch also declares a filter mask for all stat attributes.
User has to provide a mask of stats attributes to query. filter mask
can be specified in the new hdr 'struct if_stats_msg' for stats messages.
Other important field in the header is the ifindex.

This api can also include attributes for global stats (eg tcp) in the future.
When global stats are included in a stats msg, the ifindex in the header
must be zero. A single stats message cannot contain both global and
netdev specific stats. To easily distinguish them, netdev specific stat
attributes name are prefixed with IFLA_STATS_LINK_

Without any attributes in the filter_mask, no stats will be returned.

This patch has been tested with mofified iproute2 ifstat.

Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
RFC to v1 (apologies for the delay in sending this version out. busy days):
        - Addressed feedback from Dave
                - removed rtnl_link_stats
                - Added hdr struct if_stats_msg to carry ifindex and
                  filter mask
                - new macro IFLA_STATS_FILTER_BIT(ATTR) for filter mask
        - split the ipv6 patch into a separate patch, need some more eyes on it
        - prefix attributes with IFLA_STATS instead of IFLA_LINK_STATS for
          shorter attribute names

v2:
        - move IFLA_STATS_INET6 declaration to the inet6 patch
        - get rid of RTM_DELSTATS
        - mark ipv6 patch RFC. It can be used as an example for
          other AF stats like stats

v3:
        - add required padding to the if_stats_msg structure(suggested by jamal)
        - rename netdev stat attributes with IFLA_STATS_LINK prefix
          so that they are easily distinguishable with global
          stats in the future (after global stats discussion with thomas)
        - get rid of unnecessary copy when getting stats with dev_get_stats
          (suggested by dave)

v4:
        - dropped calcit and af stats from this patch. Will add it
          back when it becomes necessary and with the first af stats
          patch
        - add check for null filter in dump and return -EINVAL:
          this follows rtnl_fdb_dump in returning an error.
          But since netlink_dump does not propagate the error
          to the user, the user will not see an error and
          but will also not see any data. This is consistent with
          other kinds of dumps.

v5:
        - fix selinux nlmsgtab to account for new RTM_*STATS messages

 include/uapi/linux/if_link.h   |  23 +++++++
 include/uapi/linux/rtnetlink.h |   5 ++
 net/core/rtnetlink.c           | 150 +++++++++++++++++++++++++++++++++++++++++
 security/selinux/nlmsgtab.c    |   4 +-
 4 files changed, 181 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bb3a90b..0762f35 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -781,4 +781,27 @@ enum {
 
 #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
 
+/* STATS section */
+
+struct if_stats_msg {
+	__u8  family;
+	__u8  pad1;
+	__u16 pad2;
+	__u32 ifindex;
+	__u32 filter_mask;
+};
+
+/* A stats attribute can be netdev specific or a global stat.
+ * For netdev stats, lets use the prefix IFLA_STATS_LINK_*
+ */
+enum {
+	IFLA_STATS_UNSPEC,
+	IFLA_STATS_LINK_64,
+	__IFLA_STATS_MAX,
+};
+
+#define IFLA_STATS_MAX (__IFLA_STATS_MAX - 1)
+
+#define IFLA_STATS_FILTER_BIT(ATTR)	(1 << (ATTR))
+
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index ca764b5..cc885c4 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -139,6 +139,11 @@ enum {
 	RTM_GETNSID = 90,
 #define RTM_GETNSID RTM_GETNSID
 
+	RTM_NEWSTATS = 92,
+#define RTM_NEWSTATS RTM_NEWSTATS
+	RTM_GETSTATS = 94,
+#define RTM_GETSTATS RTM_GETSTATS
+
 	__RTM_MAX,
 #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
 };
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a75f7e9..fe35102 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3451,6 +3451,153 @@ out:
 	return err;
 }
 
+static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
+			       int type, u32 pid, u32 seq, u32 change,
+			       unsigned int flags, unsigned int filter_mask)
+{
+	struct if_stats_msg *ifsm;
+	struct nlmsghdr *nlh;
+	struct nlattr *attr;
+
+	ASSERT_RTNL();
+
+	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifsm), flags);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	ifsm = nlmsg_data(nlh);
+	ifsm->ifindex = dev->ifindex;
+	ifsm->filter_mask = filter_mask;
+
+	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64)) {
+		struct rtnl_link_stats64 *sp;
+
+		attr = nla_reserve(skb, IFLA_STATS_LINK_64,
+				   sizeof(struct rtnl_link_stats64));
+		if (!attr)
+			goto nla_put_failure;
+
+		sp = nla_data(attr);
+		dev_get_stats(dev, sp);
+	}
+
+	nlmsg_end(skb, nlh);
+
+	return 0;
+
+nla_put_failure:
+	nlmsg_cancel(skb, nlh);
+
+	return -EMSGSIZE;
+}
+
+static const struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
+	[IFLA_STATS_LINK_64]	= { .len = sizeof(struct rtnl_link_stats64) },
+};
+
+static size_t if_nlmsg_stats_size(const struct net_device *dev,
+				  u32 filter_mask)
+{
+	size_t size = 0;
+
+	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64))
+		size += nla_total_size(sizeof(struct rtnl_link_stats64));
+
+	return size;
+}
+
+static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
+{
+	struct net *net = sock_net(skb->sk);
+	struct if_stats_msg *ifsm;
+	struct net_device *dev = NULL;
+	struct sk_buff *nskb;
+	u32 filter_mask;
+	int err;
+
+	ifsm = nlmsg_data(nlh);
+	if (ifsm->ifindex > 0)
+		dev = __dev_get_by_index(net, ifsm->ifindex);
+	else
+		return -EINVAL;
+
+	if (!dev)
+		return -ENODEV;
+
+	filter_mask = ifsm->filter_mask;
+	if (!filter_mask)
+		return -EINVAL;
+
+	nskb = nlmsg_new(if_nlmsg_stats_size(dev, filter_mask), GFP_KERNEL);
+	if (!nskb)
+		return -ENOBUFS;
+
+	err = rtnl_fill_statsinfo(nskb, dev, RTM_NEWSTATS,
+				  NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0,
+				  0, filter_mask);
+	if (err < 0) {
+		/* -EMSGSIZE implies BUG in if_nlmsg_stats_size */
+		WARN_ON(err == -EMSGSIZE);
+		kfree_skb(nskb);
+	} else {
+		err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
+	}
+
+	return err;
+}
+
+static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct net *net = sock_net(skb->sk);
+	struct if_stats_msg *ifsm;
+	int h, s_h;
+	int idx = 0, s_idx;
+	struct net_device *dev;
+	struct hlist_head *head;
+	unsigned int flags = NLM_F_MULTI;
+	u32 filter_mask = 0;
+	int err;
+
+	s_h = cb->args[0];
+	s_idx = cb->args[1];
+
+	cb->seq = net->dev_base_seq;
+
+	ifsm = nlmsg_data(cb->nlh);
+	filter_mask = ifsm->filter_mask;
+	if (!filter_mask)
+		return -EINVAL;
+
+	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
+		idx = 0;
+		head = &net->dev_index_head[h];
+		hlist_for_each_entry(dev, head, index_hlist) {
+			if (idx < s_idx)
+				goto cont;
+			err = rtnl_fill_statsinfo(skb, dev, RTM_NEWSTATS,
+						  NETLINK_CB(cb->skb).portid,
+						  cb->nlh->nlmsg_seq, 0,
+						  flags, filter_mask);
+			/* If we ran out of room on the first message,
+			 * we're in trouble
+			 */
+			WARN_ON((err == -EMSGSIZE) && (skb->len == 0));
+
+			if (err < 0)
+				goto out;
+
+			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
+cont:
+			idx++;
+		}
+	}
+out:
+	cb->args[1] = idx;
+	cb->args[0] = h;
+
+	return skb->len;
+}
+
 /* Process one rtnetlink message. */
 
 static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
@@ -3600,4 +3747,7 @@ void __init rtnetlink_init(void)
 	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, NULL);
 	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, NULL);
 	rtnl_register(PF_BRIDGE, RTM_SETLINK, rtnl_bridge_setlink, NULL, NULL);
+
+	rtnl_register(PF_UNSPEC, RTM_GETSTATS, rtnl_stats_get, rtnl_stats_dump,
+		      NULL);
 }
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index 8495b93..1714633 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -76,6 +76,8 @@ static struct nlmsg_perm nlmsg_route_perms[] =
 	{ RTM_NEWNSID,		NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
 	{ RTM_DELNSID,		NETLINK_ROUTE_SOCKET__NLMSG_READ  },
 	{ RTM_GETNSID,		NETLINK_ROUTE_SOCKET__NLMSG_READ  },
+	{ RTM_NEWSTATS,		NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
+	{ RTM_GETSTATS,		NETLINK_ROUTE_SOCKET__NLMSG_READ  },
 };
 
 static struct nlmsg_perm nlmsg_tcpdiag_perms[] =
@@ -155,7 +157,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
 	switch (sclass) {
 	case SECCLASS_NETLINK_ROUTE_SOCKET:
 		/* RTM_MAX always point to RTM_SETxxxx, ie RTM_NEWxxx + 3 */
-		BUILD_BUG_ON(RTM_MAX != (RTM_NEWNSID + 3));
+		BUILD_BUG_ON(RTM_MAX != (RTM_NEWSTATS + 3));
 		err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms,
 				 sizeof(nlmsg_route_perms));
 		break;
-- 
1.9.1

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Eric Dumazet]

On Mon, 2016-04-18 at 14:10 -0700, Roopa Prabhu wrote:

> +	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64)) {
> +		struct rtnl_link_stats64 *sp;
> +
> +		attr = nla_reserve(skb, IFLA_STATS_LINK_64,
> +				   sizeof(struct rtnl_link_stats64));
> +		if (!attr)
> +			goto nla_put_failure;
> +
> +		sp = nla_data(attr);

Are you sure we have a guarantee that sp is aligned to u64 fields ?

x86 does not care, but some arches would have a potential misalign
access here.


> +		dev_get_stats(dev, sp);
> +	}

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 18 Apr 2016 14:35:56 -0700

> On Mon, 2016-04-18 at 14:10 -0700, Roopa Prabhu wrote:
> 
>> +	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64)) {
>> +		struct rtnl_link_stats64 *sp;
>> +
>> +		attr = nla_reserve(skb, IFLA_STATS_LINK_64,
>> +				   sizeof(struct rtnl_link_stats64));
>> +		if (!attr)
>> +			goto nla_put_failure;
>> +
>> +		sp = nla_data(attr);
> 
> Are you sure we have a guarantee that sp is aligned to u64 fields ?
> 
> x86 does not care, but some arches would have a potential misalign
> access here.

I'll do some testing on sparc and deal with any fallout.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 18 Apr 2016 20:57:55 -0400 (EDT)

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 18 Apr 2016 14:35:56 -0700
> 
>> On Mon, 2016-04-18 at 14:10 -0700, Roopa Prabhu wrote:
>> 
>>> +	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64)) {
>>> +		struct rtnl_link_stats64 *sp;
>>> +
>>> +		attr = nla_reserve(skb, IFLA_STATS_LINK_64,
>>> +				   sizeof(struct rtnl_link_stats64));
>>> +		if (!attr)
>>> +			goto nla_put_failure;
>>> +
>>> +		sp = nla_data(attr);
>> 
>> Are you sure we have a guarantee that sp is aligned to u64 fields ?
>> 
>> x86 does not care, but some arches would have a potential misalign
>> access here.
> 
> I'll do some testing on sparc and deal with any fallout.

Just thinking out loud before I start testing, yeah I think you are
right.  nlmsghdr is 64-bit aligned, but the nlattr header is 32-bit
which will thus make the attribute data area not be aligned.

I think the time has probably come to have a new netlink attribute
format that doesn't have this multi-decade old problem.

There are a lot of bits left in nla_type, we can use one to indicate
that the nlattr struct is another 4 bytes in length in order to
archieve proper alignment of the payload data.

+struct nlattr64 {
+	__u16           nla_len;
+	__u16           nla_type;
+	__u32		nla_pad;
+};
 ...
+#define NLA_F_64BIT_ALIGNED	(1 << 13)
-#define NLA_TYPE_MASK		~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)
+#define NLA_TYPE_MASK		~(NLA_F_NESTED | NLA_F_NET_BYTEORDER |
				  NLA_F_64BIT_ALIGNED)
 ...
#define NLA64_ALIGNTO		8
#define NLA64_ALIGN(len)	(((len) + NLA64_ALIGNTO - 1) & ~(NLA64_ALIGNTO - 1))
#define NLA64_HDRLEN		((int) NLA64_ALIGN(sizeof(struct nlattr64)))

We're going to need some new nla64_*() helpers and code added to some
of the existing ones to test that new bit.

For example, nla_data would now be:

static inline void *nla_data(const struct nlattr *nla)
{
	if (nla->nla_type & NLA_F_64BIT_ALIGNED)
		return (char *) nla + NLA64_HDRLEN;
	else
		return (char *) nla + NLA_HDRLEN;
}

nla_len would be:

static inline int nla_len(const struct nlattr *nla)
{
	int hdrlen = NLA_HDRLEN;

	if (nla->nla_type & NLA_F_64BIT_ALIGNED)
		hdrlen = NLA64_hdrlen;
	return nla->nla_len - hdrlen;
}

etc. etc.

And anyways, I get unaligned accesses without Roopa's changes :-/

davem@patience:~$ ip l l
[3391066.656729] Kernel unaligned access at TPC[7d6c14] loopback_get_stats64+0x74/0xa0
[3391066.672020] Kernel unaligned access at TPC[7d6c18] loopback_get_stats64+0x78/0xa0
[3391066.687282] Kernel unaligned access at TPC[7d6c1c] loopback_get_stats64+0x7c/0xa0
[3391066.702573] Kernel unaligned access at TPC[7d6c20] loopback_get_stats64+0x80/0xa0
[3391066.717858] Kernel unaligned access at TPC[8609dc] dev_get_stats+0x3c/0xe0

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Eric Dumazet]

On Mon, 2016-04-18 at 21:48 -0400, David Miller wrote:

> 
> And anyways, I get unaligned accesses without Roopa's changes :-/
> 
> davem@patience:~$ ip l l
> [3391066.656729] Kernel unaligned access at TPC[7d6c14] loopback_get_stats64+0x74/0xa0
> [3391066.672020] Kernel unaligned access at TPC[7d6c18] loopback_get_stats64+0x78/0xa0
> [3391066.687282] Kernel unaligned access at TPC[7d6c1c] loopback_get_stats64+0x7c/0xa0
> [3391066.702573] Kernel unaligned access at TPC[7d6c20] loopback_get_stats64+0x80/0xa0
> [3391066.717858] Kernel unaligned access at TPC[8609dc] dev_get_stats+0x3c/0xe0

Yes, rtnl_fill_stats() probably has the same mistake.

commit 550bce59baf3f3059cd4ae1e268f08f2d2cb1d5c
Author: Roopa Prabhu <roopa@cumulusnetworks.com>
Date:   Fri Apr 15 20:36:25 2016 -0700

    rtnetlink: rtnl_fill_stats: avoid an unnecssary stats copy
    
    This patch passes netlink attr data ptr directly to dev_get_stats
    thus elimiating a stats copy.
    
    Suggested-by: David Miller <davem@davemloft.net>
    Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[roopa]

On 4/18/16, 5:57 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 18 Apr 2016 14:35:56 -0700
>
>> On Mon, 2016-04-18 at 14:10 -0700, Roopa Prabhu wrote:
>>
>>> +	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64)) {
>>> +		struct rtnl_link_stats64 *sp;
>>> +
>>> +		attr = nla_reserve(skb, IFLA_STATS_LINK_64,
>>> +				   sizeof(struct rtnl_link_stats64));
>>> +		if (!attr)
>>> +			goto nla_put_failure;
>>> +
>>> +		sp = nla_data(attr);
>> Are you sure we have a guarantee that sp is aligned to u64 fields ?
>>
>> x86 does not care, but some arches would have a potential misalign
>> access here.
> I'll do some testing on sparc and deal with any fallout.
Thanks David. I will be happy to respin if you see problems.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

On 4/18/16, 7:22 PM, Eric Dumazet wrote:
> On Mon, 2016-04-18 at 21:48 -0400, David Miller wrote:
>
>> And anyways, I get unaligned accesses without Roopa's changes :-/
>>
>> davem@patience:~$ ip l l
>> [3391066.656729] Kernel unaligned access at TPC[7d6c14] loopback_get_stats64+0x74/0xa0
>> [3391066.672020] Kernel unaligned access at TPC[7d6c18] loopback_get_stats64+0x78/0xa0
>> [3391066.687282] Kernel unaligned access at TPC[7d6c1c] loopback_get_stats64+0x7c/0xa0
>> [3391066.702573] Kernel unaligned access at TPC[7d6c20] loopback_get_stats64+0x80/0xa0
>> [3391066.717858] Kernel unaligned access at TPC[8609dc] dev_get_stats+0x3c/0xe0
> Yes, rtnl_fill_stats() probably has the same mistake.
>
> commit 550bce59baf3f3059cd4ae1e268f08f2d2cb1d5c
> Author: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date:   Fri Apr 15 20:36:25 2016 -0700
>
>     rtnetlink: rtnl_fill_stats: avoid an unnecssary stats copy
>     
>     This patch passes netlink attr data ptr directly to dev_get_stats
>     thus elimiating a stats copy.
>     
>     Suggested-by: David Miller <davem@davemloft.net>
>     Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
>
>
David, if you revert the one in rtnl_fill_stats, i will take care of the dev_get_stats in RTM_GETSTATS in v6.

thanks,
Roopa

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Mon, 18 Apr 2016 14:10:19 -0700

> This patch adds a new RTM_GETSTATS message to query link stats via
> netlink from the kernel. RTM_NEWLINK also dumps stats today, but
> RTM_NEWLINK returns a lot more than just stats and is expensive in
> some cases when frequent polling for stats from userspace is a
> common operation.

I'm holding off on this until we sort out the 64-bit netlink
attribute alignment issue.

Meanwhile, I'll some kind of a fix into the tree for the
rtnl_fill_stats() change so that it doesn't cause unaligned
accesses.

I just tested out a clever idea, where for architectures where
unaligned accesses is a problem, we insert a zero length NOP attribute
before the 64-bit stats.  This makes it properly aligned.  A quick
hack patch just passed testing on my sparc64 box, but I'll go over it
some more.

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bb3a90b..5ffdcb3 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -155,6 +155,7 @@ enum {
 	IFLA_PROTO_DOWN,
 	IFLA_GSO_MAX_SEGS,
 	IFLA_GSO_MAX_SIZE,
+	IFLA_PAD,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a7a3d34..b192576 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1052,6 +1052,15 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 	struct rtnl_link_stats64 *sp;
 	struct nlattr *attr;
 
+	/* Add a zero length NOP attribute so that the nla_data()
+	 * of the IFLA_STATS64 will be 64-bit aligned.
+	 */
+#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
+	attr = nla_reserve(skb, IFLA_PAD, 0);
+	if (!attr)
+		return -EMSGSIZE;
+#endif
+
 	attr = nla_reserve(skb, IFLA_STATS64,
 			   sizeof(struct rtnl_link_stats64));
 	if (!attr)

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Mon, 18 Apr 2016 19:40:08 -0700

> On 4/18/16, 7:22 PM, Eric Dumazet wrote:
>> On Mon, 2016-04-18 at 21:48 -0400, David Miller wrote:
>>
>>> And anyways, I get unaligned accesses without Roopa's changes :-/
>>>
>>> davem@patience:~$ ip l l
>>> [3391066.656729] Kernel unaligned access at TPC[7d6c14] loopback_get_stats64+0x74/0xa0
>>> [3391066.672020] Kernel unaligned access at TPC[7d6c18] loopback_get_stats64+0x78/0xa0
>>> [3391066.687282] Kernel unaligned access at TPC[7d6c1c] loopback_get_stats64+0x7c/0xa0
>>> [3391066.702573] Kernel unaligned access at TPC[7d6c20] loopback_get_stats64+0x80/0xa0
>>> [3391066.717858] Kernel unaligned access at TPC[8609dc] dev_get_stats+0x3c/0xe0
>> Yes, rtnl_fill_stats() probably has the same mistake.
>>
>> commit 550bce59baf3f3059cd4ae1e268f08f2d2cb1d5c
>> Author: Roopa Prabhu <roopa@cumulusnetworks.com>
>> Date:   Fri Apr 15 20:36:25 2016 -0700
>>
>>     rtnetlink: rtnl_fill_stats: avoid an unnecssary stats copy
>>     
>>     This patch passes netlink attr data ptr directly to dev_get_stats
>>     thus elimiating a stats copy.
>>     
>>     Suggested-by: David Miller <davem@davemloft.net>
>>     Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>>     Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>>
>>
> David, if you revert the one in rtnl_fill_stats, i will take care of the dev_get_stats in RTM_GETSTATS in v6.

Don't need to revert, see my idea with the IFLA_PAD attribute. :-)

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 18 Apr 2016 21:48:51 -0400 (EDT)

> I think the time has probably come to have a new netlink attribute
> format that doesn't have this multi-decade old problem.
 ...

Scratch this whole idea, I think the padding attribute works a lot
better and is backwards compatible with every properly written
netlink application.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Eric Dumazet]

On Mon, 2016-04-18 at 23:41 -0400, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Mon, 18 Apr 2016 14:10:19 -0700
> 
> > This patch adds a new RTM_GETSTATS message to query link stats via
> > netlink from the kernel. RTM_NEWLINK also dumps stats today, but
> > RTM_NEWLINK returns a lot more than just stats and is expensive in
> > some cases when frequent polling for stats from userspace is a
> > common operation.
> 
> I'm holding off on this until we sort out the 64-bit netlink
> attribute alignment issue.
> 
> Meanwhile, I'll some kind of a fix into the tree for the
> rtnl_fill_stats() change so that it doesn't cause unaligned
> accesses.

Also it looks like my previous feedback for sctp_get_sctp_info() has
been ignored.

<Quoting http://www.spinics.net/lists/netdev/msg372523.html >

info is not guaranteed to be aligned on 8 bytes.

You need to use put_unaligned()

Check commit ff5d749772018 ("tcp: beware of alignments in
tcp_get_info()") for details.

</quote>

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Eric Dumazet]

On Mon, 2016-04-18 at 23:41 -0400, David Miller wrote:
>  
> +	/* Add a zero length NOP attribute so that the nla_data()
> +	 * of the IFLA_STATS64 will be 64-bit aligned.
> +	 */
> +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
> +	attr = nla_reserve(skb, IFLA_PAD, 0);
> +	if (!attr)
> +		return -EMSGSIZE;
> +#endif

You must do this only if current skb->data alignment is not correct.

(IFLA_ALIAS for example could shift your expectations)

Also you probably want to change if_nlmsg_size() to add
 + nla_total_size(0) /* IFLA_PAD */

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

On 4/18/16, 8:41 PM, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Mon, 18 Apr 2016 14:10:19 -0700
>
>> This patch adds a new RTM_GETSTATS message to query link stats via
>> netlink from the kernel. RTM_NEWLINK also dumps stats today, but
>> RTM_NEWLINK returns a lot more than just stats and is expensive in
>> some cases when frequent polling for stats from userspace is a
>> common operation.
> I'm holding off on this until we sort out the 64-bit netlink
> attribute alignment issue.
sure,
>
> Meanwhile, I'll some kind of a fix into the tree for the
> rtnl_fill_stats() change so that it doesn't cause unaligned
> accesses.
>
> I just tested out a clever idea, where for architectures where
> unaligned accesses is a problem, we insert a zero length NOP attribute
> before the 64-bit stats.  This makes it properly aligned.  A quick
> hack patch just passed testing on my sparc64 box, but I'll go over it
> some more.
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index bb3a90b..5ffdcb3 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -155,6 +155,7 @@ enum {
>  	IFLA_PROTO_DOWN,
>  	IFLA_GSO_MAX_SEGS,
>  	IFLA_GSO_MAX_SIZE,
> +	IFLA_PAD,
>  	__IFLA_MAX
>  };
>  
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index a7a3d34..b192576 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1052,6 +1052,15 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
>  	struct rtnl_link_stats64 *sp;
>  	struct nlattr *attr;
>  
> +	/* Add a zero length NOP attribute so that the nla_data()
> +	 * of the IFLA_STATS64 will be 64-bit aligned.
> +	 */
> +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
> +	attr = nla_reserve(skb, IFLA_PAD, 0);
> +	if (!attr)
> +		return -EMSGSIZE;
> +#endif
> +
>  	attr = nla_reserve(skb, IFLA_STATS64,
>  			   sizeof(struct rtnl_link_stats64));
>  	if (!attr)
>
that is cleaver :)

thanks!

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 18 Apr 2016 21:32:04 -0700

> On Mon, 2016-04-18 at 23:41 -0400, David Miller wrote:
>>  
>> +	/* Add a zero length NOP attribute so that the nla_data()
>> +	 * of the IFLA_STATS64 will be 64-bit aligned.
>> +	 */
>> +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
>> +	attr = nla_reserve(skb, IFLA_PAD, 0);
>> +	if (!attr)
>> +		return -EMSGSIZE;
>> +#endif
> 
> You must do this only if current skb->data alignment is not correct.

I'll put an assertion there if it makes you happy. :-)

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Nicolas Dichtel]

Le 19/04/2016 05:41, David Miller a écrit :
[snip]
> I just tested out a clever idea, where for architectures where
> unaligned accesses is a problem, we insert a zero length NOP attribute
> before the 64-bit stats.  This makes it properly aligned.  A quick
> hack patch just passed testing on my sparc64 box, but I'll go over it
> some more.
We saw also problems with architectures like tilera.

Just for reminder, there was a first attempt by Thomas to make this patch
generic: http://patchwork.ozlabs.org/patch/207097/

But we finally discover that some netlink API use the attribute '0'.
Ideally, it would be great to 'tell' to the libnetlink the attribute used for
padding, so that every attribute can be aligned on 8 if needed, but I don't see
a way to do it without changing the whole libnetlink API (and thus all users).
Or could we done the opposite: using Thomas patch by default and introducing a
new API in the libnetlink for netlink messages where the attribute '0' is used?
Some were identified in this patch:
31e20bad8d58 ("diag: warn about missing first netlink attribute").

I like this idea, because it will fix all users, not only rtnl, but the risk is
to forget someone.

Any thoughts?


Regards,
Nicolas

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Nicolas Dichtel]

+ selinux maintainers

Le 18/04/2016 23:10, Roopa Prabhu a écrit :
[snip]
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index 8495b93..1714633 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -76,6 +76,8 @@ static struct nlmsg_perm nlmsg_route_perms[] =
>   	{ RTM_NEWNSID,		NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>   	{ RTM_DELNSID,		NETLINK_ROUTE_SOCKET__NLMSG_READ  },
>   	{ RTM_GETNSID,		NETLINK_ROUTE_SOCKET__NLMSG_READ  },
> +	{ RTM_NEWSTATS,		NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
I would say it's NETLINK_ROUTE_SOCKET__NLMSG_READ, not WRITE. This command is
only sent by the kernel, not by the userland.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Johannes Berg]

On Mon, 2016-04-18 at 23:52 -0400, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 18 Apr 2016 21:48:51 -0400 (EDT)
> 
> > I think the time has probably come to have a new netlink attribute
> > format that doesn't have this multi-decade old problem.
>  ...
> 
> Scratch this whole idea, I think the padding attribute works a lot
> better and is backwards compatible with every properly written
> netlink application.

This talk about attribute flags reminded me about something else.

At netconf, we talked about how awkward it can be that one doesn't know
if an attribute was accepted by the kernel or simply ignored because
it's not supported (older kernel version).

I considered (and Emmanuel has started to cook up a patch for it)
adding a flag here meaning "reject if not parsed" (or so).

Now, I realize that with all the existing netlink commands one can't
actually rely on that (since it requires some infrastructure), but new
commands in existing families and also entirely new generic netlink
families would allow let userspace rely on such a thing.

Thoughts?

johannes

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Emmanuel Grumbach]

On Tue, Apr 19, 2016 at 1:09 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2016-04-18 at 23:52 -0400, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Mon, 18 Apr 2016 21:48:51 -0400 (EDT)
>>
>> > I think the time has probably come to have a new netlink attribute
>> > format that doesn't have this multi-decade old problem.
>>  ...
>>
>> Scratch this whole idea, I think the padding attribute works a lot
>> better and is backwards compatible with every properly written
>> netlink application.
>
> This talk about attribute flags reminded me about something else.
>
> At netconf, we talked about how awkward it can be that one doesn't know
> if an attribute was accepted by the kernel or simply ignored because
> it's not supported (older kernel version).
>
> I considered (and Emmanuel has started to cook up a patch for it)
> adding a flag here meaning "reject if not parsed" (or so).
>
> Now, I realize that with all the existing netlink commands one can't
> actually rely on that (since it requires some infrastructure), but new
> commands in existing families and also entirely new generic netlink
> families would allow let userspace rely on such a thing.
>
> Thoughts?


FWIW:

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 2a5dbcc..2dfd46d 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -663,6 +663,15 @@ static inline int nla_type(const struct nlattr *nla)
 }

 /**
+ * nla_must_parse - returns true if the attribute must be parsed
+ * @nla: netlink attribute
+ */
+static inline bool nla_must_parse(const struct nlattr *nla)
+{
+       return nla->nla_type & NLA_F_NET_MUST_PARSE;
+}
+
+/**
  * nla_data - head of payload
  * @nla: netlink attribute
  */
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 1a85940..e81a8d4 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -165,17 +165,21 @@ struct nlattr {

 /*
  * nla_type (16 bits)
- * +---+---+-------------------------------+
- * | N | O | Attribute Type                |
- * +---+---+-------------------------------+
+ * +---+---+---+----------------------------+
+ * | N | O | M | Attribute Type             |
+ * +---+---+---+----------------------------+
  * N := Carries nested attributes
  * O := Payload stored in network byte order
+ * M := Attribute can't be ignored
  *
  * Note: The N and O flag are mutually exclusive.
  */
 #define NLA_F_NESTED           (1 << 15)
 #define NLA_F_NET_BYTEORDER    (1 << 14)
-#define NLA_TYPE_MASK          ~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)
+#define NLA_F_NET_MUST_PARSE   (1 << 13)
+#define NLA_TYPE_MASK          ~(NLA_F_NESTED |        \
+                                 NLA_F_NET_BYTEORDER | \
+                                 NLA_F_NET_MUST_PARSE)

 #define NLA_ALIGNTO            4
 #define NLA_ALIGN(len)         (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
diff --git a/lib/nlattr.c b/lib/nlattr.c
index f5907d2..fa15e6f 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -198,6 +198,10 @@ int nla_parse(struct nlattr **tb, int maxtype,
const struct nlattr *head,
                        }

                        tb[type] = (struct nlattr *)nla;
+               } else if (nla_must_parse(nla)) {
+                       err = -EINVAL;
+                       goto errout;
                }
        }

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 19 Apr 2016 09:45:32 +0200

> But we finally discover that some netlink API use the attribute '0'.

I explicitly did not use attribute zero, and instead made a new IFLA_*
attribute exactly to avoid compatability problems.

Every netlink attribute space that needs to deal with this padding issue
will need to add such a new attribute.

It is the only safe way to deal with this problem.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 19 Apr 2016 12:09:03 +0200

> At netconf, we talked about how awkward it can be that one doesn't know
> if an attribute was accepted by the kernel or simply ignored because
> it's not supported (older kernel version).
> 
> I considered (and Emmanuel has started to cook up a patch for it)
> adding a flag here meaning "reject if not parsed" (or so).
> 
> Now, I realize that with all the existing netlink commands one can't
> actually rely on that (since it requires some infrastructure), but new
> commands in existing families and also entirely new generic netlink
> families would allow let userspace rely on such a thing.

I like this nlattr flag idea, it's opt-in and any tool can be updated
to use the new facility.

I'd be willing the backport the nlattr flag bit change to all stable
releases as well.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 19 Apr 2016 01:03:16 -0400 (EDT)

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 18 Apr 2016 21:32:04 -0700
> 
>> On Mon, 2016-04-18 at 23:41 -0400, David Miller wrote:
>>>  
>>> +	/* Add a zero length NOP attribute so that the nla_data()
>>> +	 * of the IFLA_STATS64 will be 64-bit aligned.
>>> +	 */
>>> +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
>>> +	attr = nla_reserve(skb, IFLA_PAD, 0);
>>> +	if (!attr)
>>> +		return -EMSGSIZE;
>>> +#endif
>> 
>> You must do this only if current skb->data alignment is not correct.
> 
> I'll put an assertion there if it makes you happy. :-)

Actually, you are right, as usual.

Here is the final patch I'm about to push out, thanks a lot Eric.

Roopa, please adjust your GETSTATS patch as needed (I think you need
to adjust the SELinux table entry as well) and we can integrate that
too.

====================
[PATCH] net: Align IFLA_STATS64 attributes properly on architectures that need it.

Since the nlattr header is 4 bytes in size, it can cause the netlink
attribute payload to not be 8-byte aligned.

This is particularly troublesome for IFLA_STATS64 which contains 64-bit
statistic values.

Solve this by creating a dummy IFLA_PAD attribute which has a payload
which is zero bytes in size.  When HAVE_EFFICIENT_UNALIGNED_ACCESS is
false, we insert an IFLA_PAD attribute into the netlink response when
necessary such that the IFLA_STATS64 payload will be properly aligned.

With help and suggestions from Eric Dumazet.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bb3a90b..5ffdcb3 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -155,6 +155,7 @@ enum {
 	IFLA_PROTO_DOWN,
 	IFLA_GSO_MAX_SEGS,
 	IFLA_GSO_MAX_SIZE,
+	IFLA_PAD,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a7a3d34..198ca2c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -878,6 +878,9 @@ 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(MAX_ADDR_LEN) /* IFLA_ADDRESS */
 	       + nla_total_size(MAX_ADDR_LEN) /* IFLA_BROADCAST */
@@ -1052,6 +1055,22 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 	struct rtnl_link_stats64 *sp;
 	struct nlattr *attr;
 
+#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
+
 	attr = nla_reserve(skb, IFLA_STATS64,
 			   sizeof(struct rtnl_link_stats64));
 	if (!attr)
-- 
2.4.1

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Eric Dumazet]

On Tue, 2016-04-19 at 14:31 -0400, David Miller wrote:

> 
> Here is the final patch I'm about to push out, thanks a lot Eric.
> 
> Roopa, please adjust your GETSTATS patch as needed (I think you need
> to adjust the SELinux table entry as well) and we can integrate that
> too.
> 
> ====================
> [PATCH] net: Align IFLA_STATS64 attributes properly on architectures that need it.
> 
> Since the nlattr header is 4 bytes in size, it can cause the netlink
> attribute payload to not be 8-byte aligned.
> 
> This is particularly troublesome for IFLA_STATS64 which contains 64-bit
> statistic values.
> 
> Solve this by creating a dummy IFLA_PAD attribute which has a payload
> which is zero bytes in size.  When HAVE_EFFICIENT_UNALIGNED_ACCESS is
> false, we insert an IFLA_PAD attribute into the netlink response when
> necessary such that the IFLA_STATS64 payload will be properly aligned.
> 
> With help and suggestions from Eric Dumazet.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  include/uapi/linux/if_link.h |  1 +
>  net/core/rtnetlink.c         | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)

LGTM, thanks David ;)

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Eric Dumazet]

On Tue, 2016-04-19 at 14:31 -0400, David Miller wrote:

> +#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

Since we want to use this in other places, we could define a helper.

nla_align_64bit(skb, attribute)  or something.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

On 4/19/16, 11:31 AM, David Miller wrote:

[snip]
>
> Here is the final patch I'm about to push out, thanks a lot Eric.
>
> Roopa, please adjust your GETSTATS patch as needed (I think you need
> to adjust the SELinux table entry as well) and we can integrate that
> too.
ok, will do. one thing though, for GETSTATS, if I need a pad attribute like IFLA_PAD,
I will need to add a new stats attribute IFLA_STATS_PAD and burn a bit for it in filter_mask too.
In which case, I am wondering if we should live with the copy. I will take any suggestions here.

I had adjusted the SELinux table entries for v5. I will check again and make sure it is right for v6.

>
> ====================
> [PATCH] net: Align IFLA_STATS64 attributes properly on architectures that need it.
>
> Since the nlattr header is 4 bytes in size, it can cause the netlink
> attribute payload to not be 8-byte aligned.
>
> This is particularly troublesome for IFLA_STATS64 which contains 64-bit
> statistic values.
>
> Solve this by creating a dummy IFLA_PAD attribute which has a payload
> which is zero bytes in size.  When HAVE_EFFICIENT_UNALIGNED_ACCESS is
> false, we insert an IFLA_PAD attribute into the netlink response when
> necessary such that the IFLA_STATS64 payload will be properly aligned.
>
> With help and suggestions from Eric Dumazet.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
[snip]


Thanks David.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Nicolas Dichtel]

Le 19/04/2016 20:47, Eric Dumazet a écrit :
> On Tue, 2016-04-19 at 14:31 -0400, David Miller wrote:
>
>> +#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
>
> 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()

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Johannes Berg]

On Tue, 2016-04-19 at 14:23 -0400, David Miller wrote:
> 
> I like this nlattr flag idea, it's opt-in and any tool can be updated
> to use the new facility.

Right.

> I'd be willing the backport the nlattr flag bit change to all stable
> releases as well.

I'm not really convinced that helps much - tools still can't really
rely on the kernel supporting it.

One thing that might work is that a tool might say it only wants to
support kernels that have this change (assuming we backport it to
enough kernels etc.); in that case the tool could add some absolutely
must-have information (like the IFINDEX or whatever, depends on the
command) with the new flag, this would get rejected since unpatched
kernels wouldn't understand the flag and wouldn't find that must-have
information.

Nevertheless, I think it's most reliable with new netlink commands that
are known to be available only on kernels understand and treating the
flag correctly.

johannes

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Paul Moore]

On Tue, Apr 19, 2016 at 4:26 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> + selinux maintainers
>
> Le 18/04/2016 23:10, Roopa Prabhu a écrit :
> [snip]
>>
>> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
>> index 8495b93..1714633 100644
>> --- a/security/selinux/nlmsgtab.c
>> +++ b/security/selinux/nlmsgtab.c
>> @@ -76,6 +76,8 @@ static struct nlmsg_perm nlmsg_route_perms[] =
>>         { RTM_NEWNSID,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>>         { RTM_DELNSID,          NETLINK_ROUTE_SOCKET__NLMSG_READ  },
>>         { RTM_GETNSID,          NETLINK_ROUTE_SOCKET__NLMSG_READ  },
>> +       { RTM_NEWSTATS,         NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>
> I would say it's NETLINK_ROUTE_SOCKET__NLMSG_READ, not WRITE. This command
> is only sent by the kernel, not by the userland.

From what I could tell from the patch description, it looks like
RTM_NEWSTATS only dumps stats to userspace and doesn't alter the state
of the kernel, is that correct?  If so, then yes, NLMSG__READ is the
right SELinux permission.  However, if RTM_NEWSTATS does alter the
state/configuration of the kernel then we should use NLMSG__WRITE.

-- 
paul moore
www.paul-moore.com

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

On 4/19/16, 12:55 PM, Paul Moore wrote:
> On Tue, Apr 19, 2016 at 4:26 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> + selinux maintainers
>>
>> Le 18/04/2016 23:10, Roopa Prabhu a écrit :
>> [snip]
>>> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
>>> index 8495b93..1714633 100644
>>> --- a/security/selinux/nlmsgtab.c
>>> +++ b/security/selinux/nlmsgtab.c
>>> @@ -76,6 +76,8 @@ static struct nlmsg_perm nlmsg_route_perms[] =
>>>         { RTM_NEWNSID,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>>>         { RTM_DELNSID,          NETLINK_ROUTE_SOCKET__NLMSG_READ  },
>>>         { RTM_GETNSID,          NETLINK_ROUTE_SOCKET__NLMSG_READ  },
>>> +       { RTM_NEWSTATS,         NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>> I would say it's NETLINK_ROUTE_SOCKET__NLMSG_READ, not WRITE. This command
>> is only sent by the kernel, not by the userland.
> From what I could tell from the patch description, it looks like
> RTM_NEWSTATS only dumps stats to userspace and doesn't alter the state
> of the kernel, is that correct?  If so, then yes, NLMSG__READ is the
> right SELinux permission.  However, if RTM_NEWSTATS does alter the
> state/configuration of the kernel then we should use NLMSG__WRITE.
>
okay, will change it to READ in the next version,

thanks.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Tue, 19 Apr 2016 12:05:00 -0700

> ok, will do. one thing though, for GETSTATS, if I need a pad
> attribute like IFLA_PAD, I will need to add a new stats attribute
> IFLA_STATS_PAD and burn a bit for it in filter_mask too.  In which
> case, I am wondering if we should live with the copy. I will take
> any suggestions here.

I don't think the copy is appropriate, especially if the existing full
link state dump gets away without it.  We're adding this facility for
performance reasons after all.

You have several options to avoid wasting filter mask space. For
example, you could use IFLA_STATS_UNSPEC, which should be OK since
only new applications will use these.

Or you could make IFLA_STATS_PAD the first attribute, and define the
filter mask as relative to it.  Ie. IFLA_STATS_LINK_64 uses bit
(IFLA_STATS_LINK_64 - IFLA_STATS_PAD), etc.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Miller]

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.

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>
---
 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));
-- 
2.1.0

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[David Ahern]

On 4/19/16 1:41 PM, Johannes Berg wrote:
> On Tue, 2016-04-19 at 14:23 -0400, David Miller wrote:
>>
>> I like this nlattr flag idea, it's opt-in and any tool can be updated
>> to use the new facility.
>
> Right.
>
>> I'd be willing the backport the nlattr flag bit change to all stable
>> releases as well.
>
> I'm not really convinced that helps much - tools still can't really
> rely on the kernel supporting it.
>
> One thing that might work is that a tool might say it only wants to
> support kernels that have this change (assuming we backport it to
> enough kernels etc.); in that case the tool could add some absolutely
> must-have information (like the IFINDEX or whatever, depends on the
> command) with the new flag, this would get rejected since unpatched
> kernels wouldn't understand the flag and wouldn't find that must-have
> information.
>
> Nevertheless, I think it's most reliable with new netlink commands that
> are known to be available only on kernels understand and treating the
> flag correctly.

The kernel can set a flag in the response that it acknowledges the new 
attribute/flag. I did that for filtering neigh dumps -- 21fdd092acc7.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

On 4/19/16, 3:49 PM, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Tue, 19 Apr 2016 12:05:00 -0700
>
>> ok, will do. one thing though, for GETSTATS, if I need a pad
>> attribute like IFLA_PAD, I will need to add a new stats attribute
>> IFLA_STATS_PAD and burn a bit for it in filter_mask too.  In which
>> case, I am wondering if we should live with the copy. I will take
>> any suggestions here.
> I don't think the copy is appropriate, especially if the existing full
> link state dump gets away without it.  We're adding this facility for
> performance reasons after all.
>
> You have several options to avoid wasting filter mask space. For
> example, you could use IFLA_STATS_UNSPEC, which should be OK since
> only new applications will use these.
>
> Or you could make IFLA_STATS_PAD the first attribute, and define the
> filter mask as relative to it.  Ie. IFLA_STATS_LINK_64 uses bit
> (IFLA_STATS_LINK_64 - IFLA_STATS_PAD), etc.
ok ack, makes sense.

thanks,
Roopa

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Roopa Prabhu]

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!

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Johannes Berg]

On Tue, 2016-04-19 at 19:53 -0600, David Ahern wrote:
> 
> The kernel can set a flag in the response that it acknowledges the
> new  attribute/flag. I did that for filtering neigh dumps --
> 21fdd092acc7.
> 

Hm, that works, but I think it requires writing extra code, which I was
kinda trying to avoid. With the patch that Emmanuel wrote, we can
restrict the changes to just nla_parse().

Anyway, I think we just have to document the behaviour very precisely,
and userspace can make its own decisions.

Essentially, apps will have a number of choices:

1) Use the new attribute flag only with commands known to have been
   added after the kernel support was added.

2) Use the new attribute flag with some required attribute for
   existing commands, so that older kernel will not find the required
   attribute and will reject the operation entirely.
   May or may not fall back to trying the operation again without the
   flag.

3) Simply use the new flag and do unexpected things on kernels not
   supporting the rejection mechanism - not much worse than today in
   many cases.

I guess we'll write a proper commit message and send the patch.

johannes

[PATCH net-next 0/4] libnl: enhance API to ease 64bit alignment for attribute

[Nicolas Dichtel]

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

[PATCH net-next 1/4] netlink: fix test alignment in nla_align_64bit()

[Nicolas Dichtel]

IS_ALIGN() returns true when the alignment is as expected. The pad
attribute should be added only when the alignment is not 8.

Fixes: 35c5845957c7 ("net: Add helpers for 64-bit aligning netlink attributes.")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/netlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index e644b3489acf..694caac31d2c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1245,7 +1245,7 @@ static inline int nla_validate_nested(const struct nlattr *start, int maxtype,
 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)) {
+	if (!IS_ALIGNED((unsigned long)skb->data, 8)) {
 		struct nlattr *attr = nla_reserve(skb, padattr, 0);
 		if (!attr)
 			return -EMSGSIZE;
-- 
2.4.2

[PATCH net-next 2/4] libnl: add more helpers to align attribute on 64-bit

[Nicolas Dichtel]

Add use it to align IFLA_STATS64.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/netlink.h |   8 ++++
 lib/nlattr.c          | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++
 net/core/rtnetlink.c  |   9 +----
 3 files changed, 117 insertions(+), 7 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 694caac31d2c..c01863b49787 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -244,13 +244,21 @@ int nla_memcpy(void *dest, const struct nlattr *src, int count);
 int nla_memcmp(const struct nlattr *nla, const void *data, size_t size);
 int nla_strcmp(const struct nlattr *nla, const char *str);
 struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen);
+struct nlattr *__nla_reserve_64bit(struct sk_buff *skb, int attrtype,
+				   int attrlen, int padattr);
 void *__nla_reserve_nohdr(struct sk_buff *skb, int attrlen);
 struct nlattr *nla_reserve(struct sk_buff *skb, int attrtype, int attrlen);
+struct nlattr *nla_reserve_64bit(struct sk_buff *skb, int attrtype,
+				 int attrlen, int padattr);
 void *nla_reserve_nohdr(struct sk_buff *skb, int attrlen);
 void __nla_put(struct sk_buff *skb, int attrtype, int attrlen,
 	       const void *data);
+void __nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		     const void *data, int padattr);
 void __nla_put_nohdr(struct sk_buff *skb, int attrlen, const void *data);
 int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data);
+int nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		  const void *data, int padattr);
 int nla_put_nohdr(struct sk_buff *skb, int attrlen, const void *data);
 int nla_append(struct sk_buff *skb, int attrlen, const void *data);
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index f5907d23272d..cc311b8b6ff0 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -355,6 +355,31 @@ struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
 EXPORT_SYMBOL(__nla_reserve);
 
 /**
+ * __nla_reserve_64bit - reserve room for attribute on the skb and align it
+ * @skb: socket buffer to reserve room on
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ *
+ * Adds a netlink attribute header to a socket buffer and reserves
+ * room for the payload but does not copy it. It also ensure that this
+ * attribute will be 64-bit aign.
+ *
+ * The caller is responsible to ensure that the skb provides enough
+ * tailroom for the attribute header and payload.
+ */
+struct nlattr *__nla_reserve_64bit(struct sk_buff *skb, int attrtype,
+				   int attrlen, int padattr)
+{
+#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
+	if (!IS_ALIGNED((unsigned long)skb->data, 8))
+		nla_align_64bit(skb, padattr);
+#endif
+
+	return __nla_reserve(skb, attrtype, attrlen);
+}
+EXPORT_SYMBOL(__nla_reserve_64bit);
+
+/**
  * __nla_reserve_nohdr - reserve room for attribute without header
  * @skb: socket buffer to reserve room on
  * @attrlen: length of attribute payload
@@ -397,6 +422,38 @@ struct nlattr *nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
 EXPORT_SYMBOL(nla_reserve);
 
 /**
+ * nla_reserve_64bit - reserve room for attribute on the skb and align it
+ * @skb: socket buffer to reserve room on
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ *
+ * Adds a netlink attribute header to a socket buffer and reserves
+ * room for the payload but does not copy it. It also ensure that this
+ * attribute will be 64-bit aign.
+ *
+ * Returns NULL if the tailroom of the skb is insufficient to store
+ * the attribute header and payload.
+ */
+struct nlattr *nla_reserve_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+				 int padattr)
+{
+	size_t len;
+
+#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
+	if (!IS_ALIGNED((unsigned long)skb->data, 8))
+		len = nla_total_size_64bit(attrlen);
+	else
+#endif
+		len = nla_total_size(attrlen);
+
+	if (unlikely(skb_tailroom(skb) < len))
+		return NULL;
+
+	return __nla_reserve_64bit(skb, attrtype, attrlen, padattr);
+}
+EXPORT_SYMBOL(nla_reserve_64bit);
+
+/**
  * nla_reserve_nohdr - reserve room for attribute without header
  * @skb: socket buffer to reserve room on
  * @attrlen: length of attribute payload
@@ -436,6 +493,26 @@ void __nla_put(struct sk_buff *skb, int attrtype, int attrlen,
 EXPORT_SYMBOL(__nla_put);
 
 /**
+ * __nla_put_64bit - Add a netlink attribute to a socket buffer and align it
+ * @skb: socket buffer to add attribute to
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ * @data: head of attribute payload
+ *
+ * The caller is responsible to ensure that the skb provides enough
+ * tailroom for the attribute header and payload.
+ */
+void __nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		     const void *data, int padattr)
+{
+	struct nlattr *nla;
+
+	nla = __nla_reserve_64bit(skb, attrtype, attrlen, padattr);
+	memcpy(nla_data(nla), data, attrlen);
+}
+EXPORT_SYMBOL(__nla_put_64bit);
+
+/**
  * __nla_put_nohdr - Add a netlink attribute without header
  * @skb: socket buffer to add attribute to
  * @attrlen: length of attribute payload
@@ -474,6 +551,36 @@ int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
 EXPORT_SYMBOL(nla_put);
 
 /**
+ * nla_put_64bit - Add a netlink attribute to a socket buffer and align it
+ * @skb: socket buffer to add attribute to
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ * @data: head of attribute payload
+ *
+ * Returns -EMSGSIZE if the tailroom of the skb is insufficient to store
+ * the attribute header and payload.
+ */
+int nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		  const void *data, int padattr)
+{
+	size_t len;
+
+#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
+	if (!IS_ALIGNED((unsigned long)skb->data, 8))
+		len = nla_total_size_64bit(attrlen);
+	else
+#endif
+		len = nla_total_size(attrlen);
+
+	if (unlikely(skb_tailroom(skb) < len))
+		return -EMSGSIZE;
+
+	__nla_put_64bit(skb, attrtype, attrlen, data, padattr);
+	return 0;
+}
+EXPORT_SYMBOL(nla_put_64bit);
+
+/**
  * nla_put_nohdr - Add a netlink attribute without header
  * @skb: socket buffer to add attribute to
  * @attrlen: length of attribute payload
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d3694a13c85a..24cd21273383 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1051,14 +1051,9 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 {
 	struct rtnl_link_stats64 *sp;
 	struct nlattr *attr;
-	int err;
-
-	err = nla_align_64bit(skb, IFLA_PAD);
-	if (err)
-		return err;
 
-	attr = nla_reserve(skb, IFLA_STATS64,
-			   sizeof(struct rtnl_link_stats64));
+	attr = nla_reserve_64bit(skb, IFLA_STATS64,
+				 sizeof(struct rtnl_link_stats64), IFLA_PAD);
 	if (!attr)
 		return -EMSGSIZE;
 
-- 
2.4.2

[PATCH net-next 3/4] ipmr: align RTA_MFC_STATS on 64-bit

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/uapi/linux/rtnetlink.h | 1 +
 net/ipv4/ipmr.c                | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index ca764b5da86d..02baa5281bbf 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -312,6 +312,7 @@ enum rtattr_type_t {
 	RTA_ENCAP_TYPE,
 	RTA_ENCAP,
 	RTA_EXPIRES,
+	RTA_PAD,
 	__RTA_MAX
 };
 
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 395e2814a46d..21a38e296fe2 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -2104,7 +2104,7 @@ static int __ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 	mfcs.mfcs_packets = c->mfc_un.res.pkt;
 	mfcs.mfcs_bytes = c->mfc_un.res.bytes;
 	mfcs.mfcs_wrong_if = c->mfc_un.res.wrong_if;
-	if (nla_put(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs) < 0)
+	if (nla_put_64bit(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs, RTA_PAD) < 0)
 		return -EMSGSIZE;
 
 	rtm->rtm_type = RTN_MULTICAST;
@@ -2237,7 +2237,7 @@ static size_t mroute_msgsize(bool unresolved, int maxvif)
 		      + nla_total_size(0)	/* RTA_MULTIPATH */
 		      + maxvif * NLA_ALIGN(sizeof(struct rtnexthop))
 						/* RTA_MFC_STATS */
-		      + nla_total_size(sizeof(struct rta_mfc_stats))
+		      + nla_total_size_64bit(sizeof(struct rta_mfc_stats))
 		;
 
 	return len;
-- 
2.4.2

[PATCH net-next 4/4] ip6mr: align RTA_MFC_STATS on 64-bit

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/ip6mr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index a10e77103c88..bf678324fd52 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2268,7 +2268,7 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 	mfcs.mfcs_packets = c->mfc_un.res.pkt;
 	mfcs.mfcs_bytes = c->mfc_un.res.bytes;
 	mfcs.mfcs_wrong_if = c->mfc_un.res.wrong_if;
-	if (nla_put(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs) < 0)
+	if (nla_put_64bit(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs, RTA_PAD) < 0)
 		return -EMSGSIZE;
 
 	rtm->rtm_type = RTN_MULTICAST;
@@ -2411,7 +2411,7 @@ static int mr6_msgsize(bool unresolved, int maxvif)
 		      + nla_total_size(0)	/* RTA_MULTIPATH */
 		      + maxvif * NLA_ALIGN(sizeof(struct rtnexthop))
 						/* RTA_MFC_STATS */
-		      + nla_total_size(sizeof(struct rta_mfc_stats))
+		      + nla_total_size_64bit(sizeof(struct rta_mfc_stats))
 		;
 
 	return len;
-- 
2.4.2

Re: [PATCH net-next 1/4] netlink: fix test alignment in nla_align_64bit()

[Eric Dumazet]

On Wed, 2016-04-20 at 10:57 +0200, Nicolas Dichtel wrote:
> IS_ALIGN() returns true when the alignment is as expected. The pad
> attribute should be added only when the alignment is not 8.
> 
> Fixes: 35c5845957c7 ("net: Add helpers for 64-bit aligning netlink attributes.")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  include/net/netlink.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index e644b3489acf..694caac31d2c 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -1245,7 +1245,7 @@ static inline int nla_validate_nested(const struct nlattr *start, int maxtype,
>  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)) {
> +	if (!IS_ALIGNED((unsigned long)skb->data, 8)) {
>  		struct nlattr *attr = nla_reserve(skb, padattr, 0);
>  		if (!attr)
>  			return -EMSGSIZE;

This is silly.

How have you tested your patch exactly ?

I guess David should have copied his original comment here.

-        * 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.

Re: [PATCH net-next 1/4] netlink: fix test alignment in nla_align_64bit()

[Nicolas Dichtel]

Le 20/04/2016 11:33, Eric Dumazet a écrit :
[snip]
> How have you tested your patch exactly ?
As stated in the cover letter, I didn't test it.

>
> I guess David should have copied his original comment here.
>
> -        * 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.
>
>
I knew I was missing something, thanks for the explanation.

All other patches of this series need to be updated, I will do it if there is
no other comment.

Re: [PATCH net-next 1/4] netlink: fix test alignment in nla_align_64bit()

[Eric Dumazet]

On Wed, 2016-04-20 at 11:44 +0200, Nicolas Dichtel wrote:
> Le 20/04/2016 11:33, Eric Dumazet a écrit :
> [snip]
> > How have you tested your patch exactly ?
> As stated in the cover letter, I didn't test it.


You certainly can test this, by tweaking HAVE_EFFICIENT_UNALIGNED_ACCESS
and adding another assertion in the code.

By testing it you would have caught a real bug, since David incorrectly
used HAVE_EFFICIENT_UNALIGNED_ACCESS instead of
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS

;)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index e644b3489acf..ea6872633a92 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1244,7 +1244,7 @@ static inline int nla_validate_nested(const struct nlattr *start, int maxtype,
  */
 static inline int nla_align_64bit(struct sk_buff *skb, int padattr)
 {
-#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
 	if (IS_ALIGNED((unsigned long)skb->data, 8)) {
 		struct nlattr *attr = nla_reserve(skb, padattr, 0);
 		if (!attr)
@@ -1261,7 +1261,7 @@ static inline int nla_align_64bit(struct sk_buff *skb, int padattr)
 static inline int nla_total_size_64bit(int payload)
 {
 	return NLA_ALIGN(nla_attr_size(payload))
-#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
 		+ NLA_ALIGN(nla_attr_size(0))
 #endif
 		;

Re: [PATCH net-next 1/4] netlink: fix test alignment in nla_align_64bit()

[Nicolas Dichtel]

Le 20/04/2016 11:57, Eric Dumazet a écrit :
> On Wed, 2016-04-20 at 11:44 +0200, Nicolas Dichtel wrote:
>> Le 20/04/2016 11:33, Eric Dumazet a écrit :
>> [snip]
>>> How have you tested your patch exactly ?
>> As stated in the cover letter, I didn't test it.
>
>
> You certainly can test this, by tweaking HAVE_EFFICIENT_UNALIGNED_ACCESS
> and adding another assertion in the code.
>
> By testing it you would have caught a real bug, since David incorrectly
> used HAVE_EFFICIENT_UNALIGNED_ACCESS instead of
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
>
> ;)
Héhé, good catch :)

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Jiri Benc]

On Wed, 20 Apr 2016 09:32:20 +0200, Johannes Berg wrote:
> 2) Use the new attribute flag with some required attribute for
>    existing commands, so that older kernel will not find the required
>    attribute and will reject the operation entirely.
>    May or may not fall back to trying the operation again without the
>    flag.

This is basically what I submitted half a year ago. See:
http://thread.gmane.org/gmane.linux.network/382850

 Jiri

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Johannes Berg]

On Wed, 2016-04-20 at 14:48 +0200, Jiri Benc wrote:
> On Wed, 20 Apr 2016 09:32:20 +0200, Johannes Berg wrote:
> > 
> > 2) Use the new attribute flag with some required attribute for
> >    existing commands, so that older kernel will not find the
> > required
> >    attribute and will reject the operation entirely.
> >    May or may not fall back to trying the operation again without
> > the
> >    flag.
> This is basically what I submitted half a year ago. See:
> http://thread.gmane.org/gmane.linux.network/382850
> 

That looks like a *huge* patchset though - whereas my proposal really
required only what Emmanuel sent in this thread. It did make some
assumptions, for example that any attribute lower than the "maxtype"
argument to nla_parse() was understood. [1]

Looks like you have this on a per-message basis. I thought it was
better on an attribute basis because that's really where the issue is.

You can still detect it with the per-attribute flag approach as I
described in (2) - if, for your lwtunnel example, you could specify the
flag on the RTA_ENCAP attribute, without which no lwtunnel can be
created (if I understand the code correctly.)

johannes



[1] for example, if I have three attributes:
enum attrs {__unused, A, B, C};

and the policy

policy = {
	[A] = { .type = NLA_U32 },
	[C] = { .type = NLA_U8 },
}

and then do

nla_parse(tb, 3, msg, msg_len, &policy)

it would assume that "B" is valid. Since this policy is equivalent to
the policy with
	[B] = { .type = NLA_BINARY }

(minimum length 0) we could also reject anything that has type=len=0 in
the policy, if the NLA_F_NET_MUST_PARSE flag is set in the nla_type.

This would likely be the right approach for most netlink families,
since they usually don't have holes that they actually care about -
I've yet to see any attribute that's not specified at all in the policy
but used anyway, normally you want some level of checking, and indicate
that by using { .type = NLA_BINARY } - but other things are possible.

johannes

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Jiri Benc]

On Wed, 20 Apr 2016 15:17:08 +0200, Johannes Berg wrote:
> Looks like you have this on a per-message basis. I thought it was
> better on an attribute basis because that's really where the issue is.

No problem. I'm not that happy with my patchset myself. Just wanted to
point it out in case it's useful.

 Jiri

[PATCH net-next] net: fix HAVE_EFFICIENT_UNALIGNED_ACCESS typos

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

HAVE_EFFICIENT_UNALIGNED_ACCESS needs CONFIG_ prefix.

Also add a comment in nla_align_64bit() explaining we have
to add a padding if current skb->data is aligned, as it
certainly can be confusing.

Fixes: 35c5845957c7 ("net: Add helpers for 64-bit aligning netlink attributes.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/netlink.h |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index e644b3489acf..cf95df1fa14b 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1238,18 +1238,21 @@ static inline int nla_validate_nested(const struct nlattr *start, int maxtype,
  * 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.
+ * CONFIG_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;
-	}
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	/* 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 next attribute, will make nla_data()
+	 * 8-byte aligned.
+	 */
+	if (IS_ALIGNED((unsigned long)skb->data, 8) &&
+	    !nla_reserve(skb, padattr, 0))
+		return -EMSGSIZE;
 #endif
 	return 0;
 }
@@ -1261,7 +1264,7 @@ static inline int nla_align_64bit(struct sk_buff *skb, int padattr)
 static inline int nla_total_size_64bit(int payload)
 {
 	return NLA_ALIGN(nla_attr_size(payload))
-#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
 		+ NLA_ALIGN(nla_attr_size(0))
 #endif
 		;

Re: [PATCH net-next] net: fix HAVE_EFFICIENT_UNALIGNED_ACCESS typos

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 20 Apr 2016 07:31:31 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> HAVE_EFFICIENT_UNALIGNED_ACCESS needs CONFIG_ prefix.
> 
> Also add a comment in nla_align_64bit() explaining we have
> to add a padding if current skb->data is aligned, as it
> certainly can be confusing.
> 
> Fixes: 35c5845957c7 ("net: Add helpers for 64-bit aligning netlink attributes.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.

Re: [PATCH net-next v5] rtnetlink: add new RTM_GETSTATS message to dump link stats

[Johannes Berg]

On Wed, 2016-04-20 at 15:34 +0200, Jiri Benc wrote:
> On Wed, 20 Apr 2016 15:17:08 +0200, Johannes Berg wrote:
> > 
> > Looks like you have this on a per-message basis. I thought it was
> > better on an attribute basis because that's really where the issue
> > is.
> No problem. I'm not that happy with my patchset myself. Just wanted
> to point it out in case it's useful.

Yeah, I looked at it, but I think it ended up a bit too complicated
really.

It does have slightly more validation in some sense, but I don't really
think that justifies the complexity?

No matter what, we'll always have to deal with the problem of not
having this capability on older kernels. One way to work around it
would be to add a new NLM_F_REQUEST2 flag, since the kernel currently
requires having NLM_F_REQUEST set, NLM_F_REQUEST2 messages would be
rejected by existing kernels. Dunno if it's really worth it though, I
suspect that family/command-specific detection will work in practically
all cases.

johannes

[PATCH net-next v2 0/4] libnl: enhance API to ease 64bit alignment for attribute

[Nicolas Dichtel]

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

[PATCH net-next v2 1/4] libnl: add more helpers to align attributes on 64-bit

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/netlink.h | 39 +++++++++++++++-----
 lib/nlattr.c          | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 8 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 3c1fd92a52c8..6f51a8a06498 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -244,13 +244,21 @@ int nla_memcpy(void *dest, const struct nlattr *src, int count);
 int nla_memcmp(const struct nlattr *nla, const void *data, size_t size);
 int nla_strcmp(const struct nlattr *nla, const char *str);
 struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen);
+struct nlattr *__nla_reserve_64bit(struct sk_buff *skb, int attrtype,
+				   int attrlen, int padattr);
 void *__nla_reserve_nohdr(struct sk_buff *skb, int attrlen);
 struct nlattr *nla_reserve(struct sk_buff *skb, int attrtype, int attrlen);
+struct nlattr *nla_reserve_64bit(struct sk_buff *skb, int attrtype,
+				 int attrlen, int padattr);
 void *nla_reserve_nohdr(struct sk_buff *skb, int attrlen);
 void __nla_put(struct sk_buff *skb, int attrtype, int attrlen,
 	       const void *data);
+void __nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		     const void *data, int padattr);
 void __nla_put_nohdr(struct sk_buff *skb, int attrlen, const void *data);
 int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data);
+int nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		  const void *data, int padattr);
 int nla_put_nohdr(struct sk_buff *skb, int attrlen, const void *data);
 int nla_append(struct sk_buff *skb, int attrlen, const void *data);
 
@@ -1231,6 +1239,27 @@ static inline int nla_validate_nested(const struct nlattr *start, int maxtype,
 }
 
 /**
+ * nla_need_padding_for_64bit - test 64-bit alignment of the next attribute
+ * @skb: socket buffer the message is stored in
+ *
+ * Return true if padding is needed to align the next attribute (nla_data()) to
+ * a 64-bit aligned area.
+ */
+static inline bool nla_need_padding_for_64bit(struct sk_buff *skb)
+{
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	/* The nlattr header is 4 bytes in size, that's why we test
+	 * if the skb->data _is_ aligned.  A NOP attribute, plus
+	 * nlattr header for next attribute, will make nla_data()
+	 * 8-byte aligned.
+	 */
+	if (IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8))
+		return true;
+#endif
+	return false;
+}
+
+/**
  * 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
@@ -1244,16 +1273,10 @@ static inline int nla_validate_nested(const struct nlattr *start, int maxtype,
  */
 static inline int nla_align_64bit(struct sk_buff *skb, int padattr)
 {
-#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-	/* 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 next attribute, will make nla_data()
-	 * 8-byte aligned.
-	 */
-	if (IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) &&
+	if (nla_need_padding_for_64bit(skb) &&
 	    !nla_reserve(skb, padattr, 0))
 		return -EMSGSIZE;
-#endif
+
 	return 0;
 }
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index f5907d23272d..2b82f1e2ebc2 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -355,6 +355,29 @@ struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
 EXPORT_SYMBOL(__nla_reserve);
 
 /**
+ * __nla_reserve_64bit - reserve room for attribute on the skb and align it
+ * @skb: socket buffer to reserve room on
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ *
+ * Adds a netlink attribute header to a socket buffer and reserves
+ * room for the payload but does not copy it. It also ensure that this
+ * attribute will be 64-bit aign.
+ *
+ * The caller is responsible to ensure that the skb provides enough
+ * tailroom for the attribute header and payload.
+ */
+struct nlattr *__nla_reserve_64bit(struct sk_buff *skb, int attrtype,
+				   int attrlen, int padattr)
+{
+	if (nla_need_padding_for_64bit(skb))
+		nla_align_64bit(skb, padattr);
+
+	return __nla_reserve(skb, attrtype, attrlen);
+}
+EXPORT_SYMBOL(__nla_reserve_64bit);
+
+/**
  * __nla_reserve_nohdr - reserve room for attribute without header
  * @skb: socket buffer to reserve room on
  * @attrlen: length of attribute payload
@@ -397,6 +420,35 @@ struct nlattr *nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
 EXPORT_SYMBOL(nla_reserve);
 
 /**
+ * nla_reserve_64bit - reserve room for attribute on the skb and align it
+ * @skb: socket buffer to reserve room on
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ *
+ * Adds a netlink attribute header to a socket buffer and reserves
+ * room for the payload but does not copy it. It also ensure that this
+ * attribute will be 64-bit aign.
+ *
+ * Returns NULL if the tailroom of the skb is insufficient to store
+ * the attribute header and payload.
+ */
+struct nlattr *nla_reserve_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+				 int padattr)
+{
+	size_t len;
+
+	if (nla_need_padding_for_64bit(skb))
+		len = nla_total_size_64bit(attrlen);
+	else
+		len = nla_total_size(attrlen);
+	if (unlikely(skb_tailroom(skb) < len))
+		return NULL;
+
+	return __nla_reserve_64bit(skb, attrtype, attrlen, padattr);
+}
+EXPORT_SYMBOL(nla_reserve_64bit);
+
+/**
  * nla_reserve_nohdr - reserve room for attribute without header
  * @skb: socket buffer to reserve room on
  * @attrlen: length of attribute payload
@@ -436,6 +488,26 @@ void __nla_put(struct sk_buff *skb, int attrtype, int attrlen,
 EXPORT_SYMBOL(__nla_put);
 
 /**
+ * __nla_put_64bit - Add a netlink attribute to a socket buffer and align it
+ * @skb: socket buffer to add attribute to
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ * @data: head of attribute payload
+ *
+ * The caller is responsible to ensure that the skb provides enough
+ * tailroom for the attribute header and payload.
+ */
+void __nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		     const void *data, int padattr)
+{
+	struct nlattr *nla;
+
+	nla = __nla_reserve_64bit(skb, attrtype, attrlen, padattr);
+	memcpy(nla_data(nla), data, attrlen);
+}
+EXPORT_SYMBOL(__nla_put_64bit);
+
+/**
  * __nla_put_nohdr - Add a netlink attribute without header
  * @skb: socket buffer to add attribute to
  * @attrlen: length of attribute payload
@@ -474,6 +546,33 @@ int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
 EXPORT_SYMBOL(nla_put);
 
 /**
+ * nla_put_64bit - Add a netlink attribute to a socket buffer and align it
+ * @skb: socket buffer to add attribute to
+ * @attrtype: attribute type
+ * @attrlen: length of attribute payload
+ * @data: head of attribute payload
+ *
+ * Returns -EMSGSIZE if the tailroom of the skb is insufficient to store
+ * the attribute header and payload.
+ */
+int nla_put_64bit(struct sk_buff *skb, int attrtype, int attrlen,
+		  const void *data, int padattr)
+{
+	size_t len;
+
+	if (nla_need_padding_for_64bit(skb))
+		len = nla_total_size_64bit(attrlen);
+	else
+		len = nla_total_size(attrlen);
+	if (unlikely(skb_tailroom(skb) < len))
+		return -EMSGSIZE;
+
+	__nla_put_64bit(skb, attrtype, attrlen, data, padattr);
+	return 0;
+}
+EXPORT_SYMBOL(nla_put_64bit);
+
+/**
  * nla_put_nohdr - Add a netlink attribute without header
  * @skb: socket buffer to add attribute to
  * @attrlen: length of attribute payload
-- 
2.4.2

[PATCH net-next v2 2/4] rtnl: use the new API to align IFLA_STATS*

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 4a47a9aceb1d..5ec059d52823 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1051,14 +1051,9 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 {
 	struct rtnl_link_stats64 *sp;
 	struct nlattr *attr;
-	int err;
-
-	err = nla_align_64bit(skb, IFLA_PAD);
-	if (err)
-		return err;
 
-	attr = nla_reserve(skb, IFLA_STATS64,
-			   sizeof(struct rtnl_link_stats64));
+	attr = nla_reserve_64bit(skb, IFLA_STATS64,
+				 sizeof(struct rtnl_link_stats64), IFLA_PAD);
 	if (!attr)
 		return -EMSGSIZE;
 
@@ -3469,17 +3464,10 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 
 	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64)) {
 		struct rtnl_link_stats64 *sp;
-		int err;
-
-		/* if necessary, add a zero length NOP attribute so that
-		 * IFLA_STATS_LINK_64 will be 64-bit aligned
-		 */
-		err = nla_align_64bit(skb, IFLA_STATS_UNSPEC);
-		if (err)
-			goto nla_put_failure;
 
-		attr = nla_reserve(skb, IFLA_STATS_LINK_64,
-				   sizeof(struct rtnl_link_stats64));
+		attr = nla_reserve_64bit(skb, IFLA_STATS_LINK_64,
+					 sizeof(struct rtnl_link_stats64),
+					 IFLA_STATS_UNSPEC);
 		if (!attr)
 			goto nla_put_failure;
 
-- 
2.4.2

[PATCH net-next v2 3/4] ipmr: align RTA_MFC_STATS on 64-bit

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/uapi/linux/rtnetlink.h | 1 +
 net/ipv4/ipmr.c                | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index cc885c4e9065..a94e0b69c769 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -317,6 +317,7 @@ enum rtattr_type_t {
 	RTA_ENCAP_TYPE,
 	RTA_ENCAP,
 	RTA_EXPIRES,
+	RTA_PAD,
 	__RTA_MAX
 };
 
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 395e2814a46d..21a38e296fe2 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -2104,7 +2104,7 @@ static int __ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 	mfcs.mfcs_packets = c->mfc_un.res.pkt;
 	mfcs.mfcs_bytes = c->mfc_un.res.bytes;
 	mfcs.mfcs_wrong_if = c->mfc_un.res.wrong_if;
-	if (nla_put(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs) < 0)
+	if (nla_put_64bit(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs, RTA_PAD) < 0)
 		return -EMSGSIZE;
 
 	rtm->rtm_type = RTN_MULTICAST;
@@ -2237,7 +2237,7 @@ static size_t mroute_msgsize(bool unresolved, int maxvif)
 		      + nla_total_size(0)	/* RTA_MULTIPATH */
 		      + maxvif * NLA_ALIGN(sizeof(struct rtnexthop))
 						/* RTA_MFC_STATS */
-		      + nla_total_size(sizeof(struct rta_mfc_stats))
+		      + nla_total_size_64bit(sizeof(struct rta_mfc_stats))
 		;
 
 	return len;
-- 
2.4.2

[PATCH net-next v2 4/4] ip6mr: align RTA_MFC_STATS on 64-bit

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/ip6mr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index a10e77103c88..bf678324fd52 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2268,7 +2268,7 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 	mfcs.mfcs_packets = c->mfc_un.res.pkt;
 	mfcs.mfcs_bytes = c->mfc_un.res.bytes;
 	mfcs.mfcs_wrong_if = c->mfc_un.res.wrong_if;
-	if (nla_put(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs) < 0)
+	if (nla_put_64bit(skb, RTA_MFC_STATS, sizeof(mfcs), &mfcs, RTA_PAD) < 0)
 		return -EMSGSIZE;
 
 	rtm->rtm_type = RTN_MULTICAST;
@@ -2411,7 +2411,7 @@ static int mr6_msgsize(bool unresolved, int maxvif)
 		      + nla_total_size(0)	/* RTA_MULTIPATH */
 		      + maxvif * NLA_ALIGN(sizeof(struct rtnexthop))
 						/* RTA_MFC_STATS */
-		      + nla_total_size(sizeof(struct rta_mfc_stats))
+		      + nla_total_size_64bit(sizeof(struct rta_mfc_stats))
 		;
 
 	return len;
-- 
2.4.2

Re: [PATCH net-next v2 0/4] libnl: enhance API to ease 64bit alignment for attribute

[David Miller]

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.

Re: [PATCH net-next v2 0/4] libnl: enhance API to ease 64bit alignment for attribute

[Nicolas Dichtel]

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

Re: [PATCH net-next v2 0/4] libnl: enhance API to ease 64bit alignment for attribute

[David Miller]

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.