[PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Nicolas Dichtel]

The goal of this serie is to be able to monitor multicast activities via
rtnetlink.

The main changes are:
 - when user dumps mfc entries it now get all entries, included the unresolved
   cache.
 - kernel sends rtnetlink when it adds/deletes mfc entries.

As usual, the patch against iproute2 will be sent once the patches are included and
net-next merged. I can send it on demand.

 include/linux/inetdevice.h     |   3 +
 include/net/addrconf.h         |   3 +
 include/uapi/linux/netconf.h   |   1 +
 include/uapi/linux/rtnetlink.h |   8 +++
 net/ipv4/devinet.c             |  10 ++-
 net/ipv4/ipmr.c                | 107 +++++++++++++++++++++++++++++--
 net/ipv6/addrconf.c            |  10 ++-
 net/ipv6/ip6mr.c               | 141 +++++++++++++++++++++++++++++++++++------
 8 files changed, 253 insertions(+), 30 deletions(-)

Comments are welcome.

Regards,
Nicolas

[PATCH net-next 1/7] netconf: advertise mc_forwarding status

[Nicolas Dichtel]

This patch advertise the MC_FORWARDING status for IPv4 and IPv6.
This field is readonly, only multicast engine in the kernel updates it.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/linux/inetdevice.h   |  3 +++
 include/net/addrconf.h       |  3 +++
 include/uapi/linux/netconf.h |  1 +
 net/ipv4/devinet.c           | 10 ++++++++--
 net/ipv4/ipmr.c              | 12 ++++++++++++
 net/ipv6/addrconf.c          | 10 ++++++++--
 net/ipv6/ip6mr.c             | 20 ++++++++++++++++++--
 7 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index d032780..a9d8289 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -171,6 +171,9 @@ struct in_ifaddr {
 extern int register_inetaddr_notifier(struct notifier_block *nb);
 extern int unregister_inetaddr_notifier(struct notifier_block *nb);
 
+extern void inet_netconf_notify_devconf(struct net *net, int type, int ifindex,
+					struct ipv4_devconf *devconf);
+
 extern struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref);
 static inline struct net_device *ip_dev_find(struct net *net, __be32 addr)
 {
diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 9e63e76..df4ef94 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -172,6 +172,9 @@ extern bool ipv6_chk_acast_addr(struct net *net, struct net_device *dev,
 extern int register_inet6addr_notifier(struct notifier_block *nb);
 extern int unregister_inet6addr_notifier(struct notifier_block *nb);
 
+extern void inet6_netconf_notify_devconf(struct net *net, int type, int ifindex,
+					 struct ipv6_devconf *devconf);
+
 /**
  * __in6_dev_get - get inet6_dev pointer from netdevice
  * @dev: network device
diff --git a/include/uapi/linux/netconf.h b/include/uapi/linux/netconf.h
index 75dcbc5..64804a7 100644
--- a/include/uapi/linux/netconf.h
+++ b/include/uapi/linux/netconf.h
@@ -13,6 +13,7 @@ enum {
 	NETCONFA_IFINDEX,
 	NETCONFA_FORWARDING,
 	NETCONFA_RP_FILTER,
+	NETCONFA_MC_FORWARDING,
 	__NETCONFA_MAX
 };
 #define NETCONFA_MAX	(__NETCONFA_MAX - 1)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e13183a..cc06a47 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1453,6 +1453,8 @@ static int inet_netconf_msgsize_devconf(int type)
 		size += nla_total_size(4);
 	if (type == -1 || type == NETCONFA_RP_FILTER)
 		size += nla_total_size(4);
+	if (type == -1 || type == NETCONFA_MC_FORWARDING)
+		size += nla_total_size(4);
 
 	return size;
 }
@@ -1485,6 +1487,10 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 	    nla_put_s32(skb, NETCONFA_RP_FILTER,
 			IPV4_DEVCONF(*devconf, RP_FILTER)) < 0)
 		goto nla_put_failure;
+	if ((type == -1 || type == NETCONFA_MC_FORWARDING) &&
+	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
+			IPV4_DEVCONF(*devconf, MC_FORWARDING)) < 0)
+		goto nla_put_failure;
 
 	return nlmsg_end(skb, nlh);
 
@@ -1493,8 +1499,8 @@ nla_put_failure:
 	return -EMSGSIZE;
 }
 
-static void inet_netconf_notify_devconf(struct net *net, int type, int ifindex,
-					struct ipv4_devconf *devconf)
+void inet_netconf_notify_devconf(struct net *net, int type, int ifindex,
+				 struct ipv4_devconf *devconf)
 {
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 58e4160..0c452e3 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -65,6 +65,7 @@
 #include <net/checksum.h>
 #include <net/netlink.h>
 #include <net/fib_rules.h>
+#include <linux/netconf.h>
 
 #if defined(CONFIG_IP_PIMSM_V1) || defined(CONFIG_IP_PIMSM_V2)
 #define CONFIG_IP_PIMSM	1
@@ -582,6 +583,9 @@ static int vif_delete(struct mr_table *mrt, int vifi, int notify,
 	in_dev = __in_dev_get_rtnl(dev);
 	if (in_dev) {
 		IPV4_DEVCONF(in_dev->cnf, MC_FORWARDING)--;
+		inet_netconf_notify_devconf(dev_net(dev),
+					    NETCONFA_MC_FORWARDING,
+					    dev->ifindex, &in_dev->cnf);
 		ip_rt_multicast_event(in_dev);
 	}
 
@@ -772,6 +776,8 @@ static int vif_add(struct net *net, struct mr_table *mrt,
 		return -EADDRNOTAVAIL;
 	}
 	IPV4_DEVCONF(in_dev->cnf, MC_FORWARDING)++;
+	inet_netconf_notify_devconf(net, NETCONFA_MC_FORWARDING, dev->ifindex,
+				    &in_dev->cnf);
 	ip_rt_multicast_event(in_dev);
 
 	/* Fill in the VIF structures */
@@ -1185,6 +1191,9 @@ static void mrtsock_destruct(struct sock *sk)
 	ipmr_for_each_table(mrt, net) {
 		if (sk == rtnl_dereference(mrt->mroute_sk)) {
 			IPV4_DEVCONF_ALL(net, MC_FORWARDING)--;
+			inet_netconf_notify_devconf(net, NETCONFA_MC_FORWARDING,
+						    NETCONFA_IFINDEX_ALL,
+						    net->ipv4.devconf_all);
 			RCU_INIT_POINTER(mrt->mroute_sk, NULL);
 			mroute_clean_tables(mrt);
 		}
@@ -1236,6 +1245,9 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 		if (ret == 0) {
 			rcu_assign_pointer(mrt->mroute_sk, sk);
 			IPV4_DEVCONF_ALL(net, MC_FORWARDING)++;
+			inet_netconf_notify_devconf(net, NETCONFA_MC_FORWARDING,
+						    NETCONFA_IFINDEX_ALL,
+						    net->ipv4.devconf_all);
 		}
 		rtnl_unlock();
 		return ret;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 4b644f6..976543d 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -469,6 +469,8 @@ static int inet6_netconf_msgsize_devconf(int type)
 	/* type -1 is used for ALL */
 	if (type == -1 || type == NETCONFA_FORWARDING)
 		size += nla_total_size(4);
+	if (type == -1 || type == NETCONFA_MC_FORWARDING)
+		size += nla_total_size(4);
 
 	return size;
 }
@@ -496,6 +498,10 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 	if ((type == -1 || type == NETCONFA_FORWARDING) &&
 	    nla_put_s32(skb, NETCONFA_FORWARDING, devconf->forwarding) < 0)
 		goto nla_put_failure;
+	if ((type == -1 || type == NETCONFA_MC_FORWARDING) &&
+	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
+			devconf->mc_forwarding) < 0)
+		goto nla_put_failure;
 
 	return nlmsg_end(skb, nlh);
 
@@ -504,8 +510,8 @@ nla_put_failure:
 	return -EMSGSIZE;
 }
 
-static void inet6_netconf_notify_devconf(struct net *net, int type, int ifindex,
-					 struct ipv6_devconf *devconf)
+void inet6_netconf_notify_devconf(struct net *net, int type, int ifindex,
+				  struct ipv6_devconf *devconf)
 {
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index d7c7e90..efc6d91 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -52,6 +52,7 @@
 #include <linux/netfilter_ipv6.h>
 #include <linux/export.h>
 #include <net/ip6_checksum.h>
+#include <linux/netconf.h>
 
 struct mr6_table {
 	struct list_head	list;
@@ -805,8 +806,12 @@ static int mif6_delete(struct mr6_table *mrt, int vifi, struct list_head *head)
 	dev_set_allmulti(dev, -1);
 
 	in6_dev = __in6_dev_get(dev);
-	if (in6_dev)
+	if (in6_dev) {
 		in6_dev->cnf.mc_forwarding--;
+		inet6_netconf_notify_devconf(dev_net(dev),
+					     NETCONFA_MC_FORWARDING,
+					     dev->ifindex, &in6_dev->cnf);
+	}
 
 	if (v->flags & MIFF_REGISTER)
 		unregister_netdevice_queue(dev, head);
@@ -958,8 +963,12 @@ static int mif6_add(struct net *net, struct mr6_table *mrt,
 	}
 
 	in6_dev = __in6_dev_get(dev);
-	if (in6_dev)
+	if (in6_dev) {
 		in6_dev->cnf.mc_forwarding++;
+		inet6_netconf_notify_devconf(dev_net(dev),
+					     NETCONFA_MC_FORWARDING,
+					     dev->ifindex, &in6_dev->cnf);
+	}
 
 	/*
 	 *	Fill in the VIF structures
@@ -1513,6 +1522,9 @@ static int ip6mr_sk_init(struct mr6_table *mrt, struct sock *sk)
 	if (likely(mrt->mroute6_sk == NULL)) {
 		mrt->mroute6_sk = sk;
 		net->ipv6.devconf_all->mc_forwarding++;
+		inet6_netconf_notify_devconf(net, NETCONFA_MC_FORWARDING,
+					     NETCONFA_IFINDEX_ALL,
+					     net->ipv6.devconf_all);
 	}
 	else
 		err = -EADDRINUSE;
@@ -1535,6 +1547,10 @@ int ip6mr_sk_done(struct sock *sk)
 			write_lock_bh(&mrt_lock);
 			mrt->mroute6_sk = NULL;
 			net->ipv6.devconf_all->mc_forwarding--;
+			inet6_netconf_notify_devconf(net,
+						     NETCONFA_MC_FORWARDING,
+						     NETCONFA_IFINDEX_ALL,
+						     net->ipv6.devconf_all);
 			write_unlock_bh(&mrt_lock);
 
 			mroute_clean_tables(mrt);
-- 
1.8.0.1

[PATCH net-next 2/7] ip6mr: use nla_nest_* helpers

[Nicolas Dichtel]

This patch removes the skb manipulations when nested attributes are added by
using standard helpers.

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

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index efc6d91..653df91 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2119,8 +2119,7 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 {
 	int ct;
 	struct rtnexthop *nhp;
-	u8 *b = skb_tail_pointer(skb);
-	struct rtattr *mp_head;
+	struct nlattr *mp_attr;
 
 	/* If cache is unresolved, don't try to parse IIF and OIF */
 	if (c->mf6c_parent >= MAXMIFS)
@@ -2129,28 +2128,29 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 	if (MIF_EXISTS(mrt, c->mf6c_parent) &&
 	    nla_put_u32(skb, RTA_IIF, mrt->vif6_table[c->mf6c_parent].dev->ifindex) < 0)
 		return -EMSGSIZE;
-
-	mp_head = (struct rtattr *)skb_put(skb, RTA_LENGTH(0));
+	mp_attr = nla_nest_start(skb, RTA_MULTIPATH);
+	if (mp_attr == NULL)
+		return -EMSGSIZE;
 
 	for (ct = c->mfc_un.res.minvif; ct < c->mfc_un.res.maxvif; ct++) {
 		if (MIF_EXISTS(mrt, ct) && c->mfc_un.res.ttls[ct] < 255) {
-			if (skb_tailroom(skb) < RTA_ALIGN(RTA_ALIGN(sizeof(*nhp)) + 4))
-				goto rtattr_failure;
-			nhp = (struct rtnexthop *)skb_put(skb, RTA_ALIGN(sizeof(*nhp)));
+			nhp = nla_reserve_nohdr(skb, sizeof(*nhp));
+			if (nhp == NULL) {
+				nla_nest_cancel(skb, mp_attr);
+				return -EMSGSIZE;
+			}
+
 			nhp->rtnh_flags = 0;
 			nhp->rtnh_hops = c->mfc_un.res.ttls[ct];
 			nhp->rtnh_ifindex = mrt->vif6_table[ct].dev->ifindex;
 			nhp->rtnh_len = sizeof(*nhp);
 		}
 	}
-	mp_head->rta_type = RTA_MULTIPATH;
-	mp_head->rta_len = skb_tail_pointer(skb) - (u8 *)mp_head;
+
+	nla_nest_end(skb, mp_attr);
+
 	rtm->rtm_type = RTN_MULTICAST;
 	return 1;
-
-rtattr_failure:
-	nlmsg_trim(skb, b);
-	return -EMSGSIZE;
 }
 
 int ip6mr_get_route(struct net *net,
-- 
1.8.0.1

[PATCH net-next 3/7] ipmr/ip6mr: advertise mfc stats via rtnetlink

[Nicolas Dichtel]

These statistics can be checked only via /proc/net/ip_mr_cache or
SIOCGETSGCNT[_IN6] and thus only for the table RT_TABLE_DEFAULT.
Advertising them via rtnetlink allows to get statistics for all cache entries,
whatever the table is.

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

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 3dee071..80abe27 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -288,6 +288,7 @@ enum rtattr_type_t {
 	RTA_MP_ALGO, /* no longer used */
 	RTA_TABLE,
 	RTA_MARK,
+	RTA_MFC_STATS,
 	__RTA_MAX
 };
 
@@ -408,6 +409,12 @@ struct rta_session {
 	} u;
 };
 
+struct rta_mfc_stats {
+	__u64	mfcs_packets;
+	__u64	mfcs_bytes;
+	__u64	mfcs_wrong_if;
+};
+
 /****
  *		General form of address family dependent message.
  ****/
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 0c452e3..c5617d6 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -2046,6 +2046,7 @@ static int __ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 	int ct;
 	struct rtnexthop *nhp;
 	struct nlattr *mp_attr;
+	struct rta_mfc_stats mfcs;
 
 	/* If cache is unresolved, don't try to parse IIF and OIF */
 	if (c->mfc_parent >= MAXVIFS)
@@ -2074,6 +2075,12 @@ static int __ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 
 	nla_nest_end(skb, mp_attr);
 
+	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)
+		return -EMSGSIZE;
+
 	rtm->rtm_type = RTN_MULTICAST;
 	return 1;
 }
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 653df91..b744b98 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2120,6 +2120,7 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 	int ct;
 	struct rtnexthop *nhp;
 	struct nlattr *mp_attr;
+	struct rta_mfc_stats mfcs;
 
 	/* If cache is unresolved, don't try to parse IIF and OIF */
 	if (c->mf6c_parent >= MAXMIFS)
@@ -2149,6 +2150,12 @@ static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 
 	nla_nest_end(skb, mp_attr);
 
+	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)
+		return -EMSGSIZE;
+
 	rtm->rtm_type = RTN_MULTICAST;
 	return 1;
 }
-- 
1.8.0.1

[PATCH net-next 4/7] ipmr/ip6mr: report origin of mfc entry into rtnl msg

[Nicolas Dichtel]

A mfc entry can be static or not (added via the mroute_sk socket). The patch
reports MFC_STATIC flag into rtm_protocol by setting rtm_protocol to
RTPROT_STATIC or RTPROT_MROUTED.

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

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 80abe27..33d29ce 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -227,6 +227,7 @@ enum {
 #define RTPROT_XORP	14	/* XORP */
 #define RTPROT_NTK	15	/* Netsukuku */
 #define RTPROT_DHCP	16      /* DHCP client */
+#define RTPROT_MROUTED	17      /* Multicast daemon */
 
 /* rtm_scope
 
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index c5617d6..91782a7 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -2169,7 +2169,10 @@ static int ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 		goto nla_put_failure;
 	rtm->rtm_type     = RTN_MULTICAST;
 	rtm->rtm_scope    = RT_SCOPE_UNIVERSE;
-	rtm->rtm_protocol = RTPROT_UNSPEC;
+	if (c->mfc_flags & MFC_STATIC)
+		rtm->rtm_protocol = RTPROT_STATIC;
+	else
+		rtm->rtm_protocol = RTPROT_MROUTED;
 	rtm->rtm_flags    = 0;
 
 	if (nla_put_be32(skb, RTA_SRC, c->mfc_origin) ||
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index b744b98..e9ef38f 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2249,7 +2249,10 @@ static int ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 	if (nla_put_u32(skb, RTA_TABLE, mrt->id))
 		goto nla_put_failure;
 	rtm->rtm_scope    = RT_SCOPE_UNIVERSE;
-	rtm->rtm_protocol = RTPROT_UNSPEC;
+	if (c->mfc_flags & MFC_STATIC)
+		rtm->rtm_protocol = RTPROT_STATIC;
+	else
+		rtm->rtm_protocol = RTPROT_MROUTED;
 	rtm->rtm_flags    = 0;
 
 	if (nla_put(skb, RTA_SRC, 16, &c->mf6c_origin) ||
-- 
1.8.0.1

[PATCH net-next 5/7] ipmr/ip6mr: allow to get unresolved cache via netlink

[Nicolas Dichtel]

/proc/net/ip[6]_mr_cache allows to get all mfc entries, even if they are put in
the unresolved list (mfc[6]_unres_queue). But only the table RT_TABLE_DEFAULT is
displayed.
This patch adds the parsing of the unresolved list when the dump is made via
rtnetlink, hence each table can be checked.

In IPv6, we set rtm_type in ip6mr_fill_mroute(), because in case of unresolved
mfc __ip6mr_fill_mroute() will not set it. In IPv4, it is already done.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv4/ipmr.c  | 21 ++++++++++++++++++++-
 net/ipv6/ip6mr.c | 22 +++++++++++++++++++++-
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 91782a7..084dac3 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -2154,6 +2154,7 @@ static int ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 {
 	struct nlmsghdr *nlh;
 	struct rtmsg *rtm;
+	int err;
 
 	nlh = nlmsg_put(skb, portid, seq, RTM_NEWROUTE, sizeof(*rtm), NLM_F_MULTI);
 	if (nlh == NULL)
@@ -2178,7 +2179,9 @@ static int ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 	if (nla_put_be32(skb, RTA_SRC, c->mfc_origin) ||
 	    nla_put_be32(skb, RTA_DST, c->mfc_mcastgrp))
 		goto nla_put_failure;
-	if (__ipmr_fill_mroute(mrt, skb, c, rtm) < 0)
+	err = __ipmr_fill_mroute(mrt, skb, c, rtm);
+	/* do not break the dump if cache is unresolved */
+	if (err < 0 && err != -ENOENT)
 		goto nla_put_failure;
 
 	return nlmsg_end(skb, nlh);
@@ -2221,6 +2224,22 @@ next_entry:
 			}
 			e = s_e = 0;
 		}
+		spin_lock_bh(&mfc_unres_lock);
+		list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
+			if (e < s_e)
+				goto next_entry2;
+			if (ipmr_fill_mroute(mrt, skb,
+					     NETLINK_CB(cb->skb).portid,
+					     cb->nlh->nlmsg_seq,
+					     mfc) < 0) {
+				spin_unlock_bh(&mfc_unres_lock);
+				goto done;
+			}
+next_entry2:
+			e++;
+		}
+		spin_unlock_bh(&mfc_unres_lock);
+		e = s_e = 0;
 		s_h = 0;
 next_table:
 		t++;
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index e9ef38f..175270f 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2235,6 +2235,7 @@ static int ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 {
 	struct nlmsghdr *nlh;
 	struct rtmsg *rtm;
+	int err;
 
 	nlh = nlmsg_put(skb, portid, seq, RTM_NEWROUTE, sizeof(*rtm), NLM_F_MULTI);
 	if (nlh == NULL)
@@ -2248,6 +2249,7 @@ static int ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 	rtm->rtm_table    = mrt->id;
 	if (nla_put_u32(skb, RTA_TABLE, mrt->id))
 		goto nla_put_failure;
+	rtm->rtm_type = RTN_MULTICAST;
 	rtm->rtm_scope    = RT_SCOPE_UNIVERSE;
 	if (c->mfc_flags & MFC_STATIC)
 		rtm->rtm_protocol = RTPROT_STATIC;
@@ -2258,7 +2260,9 @@ static int ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 	if (nla_put(skb, RTA_SRC, 16, &c->mf6c_origin) ||
 	    nla_put(skb, RTA_DST, 16, &c->mf6c_mcastgrp))
 		goto nla_put_failure;
-	if (__ip6mr_fill_mroute(mrt, skb, c, rtm) < 0)
+	err = __ip6mr_fill_mroute(mrt, skb, c, rtm);
+	/* do not break the dump if cache is unresolved */
+	if (err < 0 && err != -ENOENT)
 		goto nla_put_failure;
 
 	return nlmsg_end(skb, nlh);
@@ -2301,6 +2305,22 @@ next_entry:
 			}
 			e = s_e = 0;
 		}
+		spin_lock_bh(&mfc_unres_lock);
+		list_for_each_entry(mfc, &mrt->mfc6_unres_queue, list) {
+			if (e < s_e)
+				goto next_entry2;
+			if (ip6mr_fill_mroute(mrt, skb,
+					      NETLINK_CB(cb->skb).portid,
+					      cb->nlh->nlmsg_seq,
+					      mfc) < 0) {
+				spin_unlock_bh(&mfc_unres_lock);
+				goto done;
+			}
+next_entry2:
+			e++;
+		}
+		spin_unlock_bh(&mfc_unres_lock);
+		e = s_e = 0;
 		s_h = 0;
 next_table:
 		t++;
-- 
1.8.0.1

[PATCH net-next 6/7] ipmr: advertise new mfc entries via rtnl

[Nicolas Dichtel]

This patch allows to monitor mfc activities via rtnetlink.
To avoid parsing two times the mfc oifs, we use maxvif to allocate the rtnl
msg, thus we may allocate some superfluous space.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv4/ipmr.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 084dac3..a9454cb 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -134,6 +134,8 @@ static int ipmr_cache_report(struct mr_table *mrt,
 			     struct sk_buff *pkt, vifi_t vifi, int assert);
 static int __ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
 			      struct mfc_cache *c, struct rtmsg *rtm);
+static void mroute_netlink_event(struct mr_table *mrt, struct mfc_cache *mfc,
+				 int cmd);
 static void mroute_clean_tables(struct mr_table *mrt);
 static void ipmr_expire_process(unsigned long arg);
 
@@ -669,6 +671,7 @@ static void ipmr_expire_process(unsigned long arg)
 		}
 
 		list_del(&c->list);
+		mroute_netlink_event(mrt, c, RTM_DELROUTE);
 		ipmr_destroy_unres(mrt, c);
 	}
 
@@ -1026,6 +1029,7 @@ ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi, struct sk_buff *skb)
 
 		atomic_inc(&mrt->cache_resolve_queue_len);
 		list_add(&c->list, &mrt->mfc_unres_queue);
+		mroute_netlink_event(mrt, c, RTM_NEWROUTE);
 
 		if (atomic_read(&mrt->cache_resolve_queue_len) == 1)
 			mod_timer(&mrt->ipmr_expire_timer, c->mfc_un.unres.expires);
@@ -1060,7 +1064,7 @@ static int ipmr_mfc_delete(struct mr_table *mrt, struct mfcctl *mfc)
 		if (c->mfc_origin == mfc->mfcc_origin.s_addr &&
 		    c->mfc_mcastgrp == mfc->mfcc_mcastgrp.s_addr) {
 			list_del_rcu(&c->list);
-
+			mroute_netlink_event(mrt, c, RTM_DELROUTE);
 			ipmr_cache_free(c);
 			return 0;
 		}
@@ -1095,6 +1099,7 @@ static int ipmr_mfc_add(struct net *net, struct mr_table *mrt,
 		if (!mrtsock)
 			c->mfc_flags |= MFC_STATIC;
 		write_unlock_bh(&mrt_lock);
+		mroute_netlink_event(mrt, c, RTM_NEWROUTE);
 		return 0;
 	}
 
@@ -1137,6 +1142,7 @@ static int ipmr_mfc_add(struct net *net, struct mr_table *mrt,
 		ipmr_cache_resolve(net, mrt, uc, c);
 		ipmr_cache_free(uc);
 	}
+	mroute_netlink_event(mrt, c, RTM_NEWROUTE);
 	return 0;
 }
 
@@ -1165,6 +1171,7 @@ static void mroute_clean_tables(struct mr_table *mrt)
 			if (c->mfc_flags & MFC_STATIC)
 				continue;
 			list_del_rcu(&c->list);
+			mroute_netlink_event(mrt, c, RTM_DELROUTE);
 			ipmr_cache_free(c);
 		}
 	}
@@ -1173,6 +1180,7 @@ static void mroute_clean_tables(struct mr_table *mrt)
 		spin_lock_bh(&mfc_unres_lock);
 		list_for_each_entry_safe(c, next, &mrt->mfc_unres_queue, list) {
 			list_del(&c->list);
+			mroute_netlink_event(mrt, c, RTM_DELROUTE);
 			ipmr_destroy_unres(mrt, c);
 		}
 		spin_unlock_bh(&mfc_unres_lock);
@@ -2150,13 +2158,13 @@ int ipmr_get_route(struct net *net, struct sk_buff *skb,
 }
 
 static int ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
-			    u32 portid, u32 seq, struct mfc_cache *c)
+			    u32 portid, u32 seq, struct mfc_cache *c, int cmd)
 {
 	struct nlmsghdr *nlh;
 	struct rtmsg *rtm;
 	int err;
 
-	nlh = nlmsg_put(skb, portid, seq, RTM_NEWROUTE, sizeof(*rtm), NLM_F_MULTI);
+	nlh = nlmsg_put(skb, portid, seq, cmd, sizeof(*rtm), NLM_F_MULTI);
 	if (nlh == NULL)
 		return -EMSGSIZE;
 
@@ -2191,6 +2199,52 @@ nla_put_failure:
 	return -EMSGSIZE;
 }
 
+static size_t mroute_msgsize(bool unresolved, int maxvif)
+{
+	size_t len =
+		NLMSG_ALIGN(sizeof(struct rtmsg))
+		+ nla_total_size(4)	/* RTA_TABLE */
+		+ nla_total_size(4)	/* RTA_SRC */
+		+ nla_total_size(4)	/* RTA_DST */
+		;
+
+	if (!unresolved)
+		len = len
+		      + nla_total_size(4)	/* RTA_IIF */
+		      + nla_total_size(0)	/* RTA_MULTIPATH */
+		      + maxvif * NLA_ALIGN(sizeof(struct rtnexthop))
+						/* RTA_MFC_STATS */
+		      + nla_total_size(sizeof(struct rta_mfc_stats))
+		;
+
+	return len;
+}
+
+static void mroute_netlink_event(struct mr_table *mrt, struct mfc_cache *mfc,
+				 int cmd)
+{
+	struct net *net = read_pnet(&mrt->net);
+	struct sk_buff *skb;
+	int err = -ENOBUFS;
+
+	skb = nlmsg_new(mroute_msgsize(mfc->mfc_parent >= MAXVIFS, mrt->maxvif),
+			GFP_ATOMIC);
+	if (skb == NULL)
+		goto errout;
+
+	err = ipmr_fill_mroute(mrt, skb, 0, 0, mfc, cmd);
+	if (err < 0)
+		goto errout;
+
+	rtnl_notify(skb, net, 0, RTNLGRP_IPV4_MROUTE, NULL, GFP_ATOMIC);
+	return;
+
+errout:
+	kfree_skb(skb);
+	if (err < 0)
+		rtnl_set_sk_err(net, RTNLGRP_IPV4_MROUTE, err);
+}
+
 static int ipmr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct net *net = sock_net(skb->sk);
@@ -2217,7 +2271,7 @@ static int ipmr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
 				if (ipmr_fill_mroute(mrt, skb,
 						     NETLINK_CB(cb->skb).portid,
 						     cb->nlh->nlmsg_seq,
-						     mfc) < 0)
+						     mfc, RTM_NEWROUTE) < 0)
 					goto done;
 next_entry:
 				e++;
@@ -2231,7 +2285,7 @@ next_entry:
 			if (ipmr_fill_mroute(mrt, skb,
 					     NETLINK_CB(cb->skb).portid,
 					     cb->nlh->nlmsg_seq,
-					     mfc) < 0) {
+					     mfc, RTM_NEWROUTE) < 0) {
 				spin_unlock_bh(&mfc_unres_lock);
 				goto done;
 			}
-- 
1.8.0.1

[PATCH net-next 7/7] ip6mr: advertise new mfc entries via rtnl

[Nicolas Dichtel]

This patch allows to monitor mf6c activities via rtnetlink.
To avoid parsing two times the mf6c oifs, we use maxvif to allocate the rtnl
msg, thus we may allocate some superfluous space.

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

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 175270f..26dcdec 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -116,6 +116,8 @@ static int ip6mr_cache_report(struct mr6_table *mrt, struct sk_buff *pkt,
 			      mifi_t mifi, int assert);
 static int __ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
 			       struct mfc6_cache *c, struct rtmsg *rtm);
+static void mr6_netlink_event(struct mr6_table *mrt, struct mfc6_cache *mfc,
+			      int cmd);
 static int ip6mr_rtm_dumproute(struct sk_buff *skb,
 			       struct netlink_callback *cb);
 static void mroute_clean_tables(struct mr6_table *mrt);
@@ -870,6 +872,7 @@ static void ipmr_do_expire_process(struct mr6_table *mrt)
 		}
 
 		list_del(&c->list);
+		mr6_netlink_event(mrt, c, RTM_DELROUTE);
 		ip6mr_destroy_unres(mrt, c);
 	}
 
@@ -1220,6 +1223,7 @@ ip6mr_cache_unresolved(struct mr6_table *mrt, mifi_t mifi, struct sk_buff *skb)
 
 		atomic_inc(&mrt->cache_resolve_queue_len);
 		list_add(&c->list, &mrt->mfc6_unres_queue);
+		mr6_netlink_event(mrt, c, RTM_NEWROUTE);
 
 		ipmr_do_expire_process(mrt);
 	}
@@ -1257,6 +1261,7 @@ static int ip6mr_mfc_delete(struct mr6_table *mrt, struct mf6cctl *mfc)
 			list_del(&c->list);
 			write_unlock_bh(&mrt_lock);
 
+			mr6_netlink_event(mrt, c, RTM_DELROUTE);
 			ip6mr_cache_free(c);
 			return 0;
 		}
@@ -1421,6 +1426,7 @@ static int ip6mr_mfc_add(struct net *net, struct mr6_table *mrt,
 		if (!mrtsock)
 			c->mfc_flags |= MFC_STATIC;
 		write_unlock_bh(&mrt_lock);
+		mr6_netlink_event(mrt, c, RTM_NEWROUTE);
 		return 0;
 	}
 
@@ -1465,6 +1471,7 @@ static int ip6mr_mfc_add(struct net *net, struct mr6_table *mrt,
 		ip6mr_cache_resolve(net, mrt, uc, c);
 		ip6mr_cache_free(uc);
 	}
+	mr6_netlink_event(mrt, c, RTM_NEWROUTE);
 	return 0;
 }
 
@@ -1498,6 +1505,7 @@ static void mroute_clean_tables(struct mr6_table *mrt)
 			list_del(&c->list);
 			write_unlock_bh(&mrt_lock);
 
+			mr6_netlink_event(mrt, c, RTM_DELROUTE);
 			ip6mr_cache_free(c);
 		}
 	}
@@ -1506,6 +1514,7 @@ static void mroute_clean_tables(struct mr6_table *mrt)
 		spin_lock_bh(&mfc_unres_lock);
 		list_for_each_entry_safe(c, next, &mrt->mfc6_unres_queue, list) {
 			list_del(&c->list);
+			mr6_netlink_event(mrt, c, RTM_DELROUTE);
 			ip6mr_destroy_unres(mrt, c);
 		}
 		spin_unlock_bh(&mfc_unres_lock);
@@ -2231,13 +2240,13 @@ int ip6mr_get_route(struct net *net,
 }
 
 static int ip6mr_fill_mroute(struct mr6_table *mrt, struct sk_buff *skb,
-			     u32 portid, u32 seq, struct mfc6_cache *c)
+			     u32 portid, u32 seq, struct mfc6_cache *c, int cmd)
 {
 	struct nlmsghdr *nlh;
 	struct rtmsg *rtm;
 	int err;
 
-	nlh = nlmsg_put(skb, portid, seq, RTM_NEWROUTE, sizeof(*rtm), NLM_F_MULTI);
+	nlh = nlmsg_put(skb, portid, seq, cmd, sizeof(*rtm), NLM_F_MULTI);
 	if (nlh == NULL)
 		return -EMSGSIZE;
 
@@ -2272,6 +2281,52 @@ nla_put_failure:
 	return -EMSGSIZE;
 }
 
+static int mr6_msgsize(bool unresolved, int maxvif)
+{
+	size_t len =
+		NLMSG_ALIGN(sizeof(struct rtmsg))
+		+ nla_total_size(4)	/* RTA_TABLE */
+		+ nla_total_size(sizeof(struct in6_addr))	/* RTA_SRC */
+		+ nla_total_size(sizeof(struct in6_addr))	/* RTA_DST */
+		;
+
+	if (!unresolved)
+		len = len
+		      + nla_total_size(4)	/* RTA_IIF */
+		      + nla_total_size(0)	/* RTA_MULTIPATH */
+		      + maxvif * NLA_ALIGN(sizeof(struct rtnexthop))
+						/* RTA_MFC_STATS */
+		      + nla_total_size(sizeof(struct rta_mfc_stats))
+		;
+
+	return len;
+}
+
+static void mr6_netlink_event(struct mr6_table *mrt, struct mfc6_cache *mfc,
+			      int cmd)
+{
+	struct net *net = read_pnet(&mrt->net);
+	struct sk_buff *skb;
+	int err = -ENOBUFS;
+
+	skb = nlmsg_new(mr6_msgsize(mfc->mf6c_parent >= MAXMIFS, mrt->maxvif),
+			GFP_ATOMIC);
+	if (skb == NULL)
+		goto errout;
+
+	err = ip6mr_fill_mroute(mrt, skb, 0, 0, mfc, cmd);
+	if (err < 0)
+		goto errout;
+
+	rtnl_notify(skb, net, 0, RTNLGRP_IPV6_MROUTE, NULL, GFP_ATOMIC);
+	return;
+
+errout:
+	kfree_skb(skb);
+	if (err < 0)
+		rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE, err);
+}
+
 static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct net *net = sock_net(skb->sk);
@@ -2298,7 +2353,7 @@ static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
 				if (ip6mr_fill_mroute(mrt, skb,
 						      NETLINK_CB(cb->skb).portid,
 						      cb->nlh->nlmsg_seq,
-						      mfc) < 0)
+						      mfc, RTM_NEWROUTE) < 0)
 					goto done;
 next_entry:
 				e++;
@@ -2312,7 +2367,7 @@ next_entry:
 			if (ip6mr_fill_mroute(mrt, skb,
 					      NETLINK_CB(cb->skb).portid,
 					      cb->nlh->nlmsg_seq,
-					      mfc) < 0) {
+					      mfc, RTM_NEWROUTE) < 0) {
 				spin_unlock_bh(&mfc_unres_lock);
 				goto done;
 			}
-- 
1.8.0.1

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue,  4 Dec 2012 12:13:34 +0100

> The goal of this serie is to be able to monitor multicast activities via
> rtnetlink.
> 
> The main changes are:
>  - when user dumps mfc entries it now get all entries, included the unresolved
>    cache.
>  - kernel sends rtnetlink when it adds/deletes mfc entries.
> 
> As usual, the patch against iproute2 will be sent once the patches are included and
> net-next merged. I can send it on demand.

This looks good, applied, thanks Nicolas.

The one thing I worry about are those 64-bit statistics.  I fear that they
not be 64-bit aligned in the final netlink message.  This matters on cpus
that trap on unaligned loads/stores, such as sparc and MIPS.

Can you validate this?

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Nicolas Dichtel]

Le 04/12/2012 19:09, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Tue,  4 Dec 2012 12:13:34 +0100
>
>> The goal of this serie is to be able to monitor multicast activities via
>> rtnetlink.
>>
>> The main changes are:
>>   - when user dumps mfc entries it now get all entries, included the unresolved
>>     cache.
>>   - kernel sends rtnetlink when it adds/deletes mfc entries.
>>
>> As usual, the patch against iproute2 will be sent once the patches are included and
>> net-next merged. I can send it on demand.
>
> This looks good, applied, thanks Nicolas.
>
> The one thing I worry about are those 64-bit statistics.  I fear that they
> not be 64-bit aligned in the final netlink message.  This matters on cpus
> that trap on unaligned loads/stores, such as sparc and MIPS.
>
> Can you validate this?
>
I can have a try on a tile platform. I don't have access to sparc or mips.

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Nicolas Dichtel]

Le 04/12/2012 21:02, Nicolas Dichtel a écrit :
> Le 04/12/2012 19:09, David Miller a écrit :
>> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> Date: Tue,  4 Dec 2012 12:13:34 +0100
>>
>>> The goal of this serie is to be able to monitor multicast activities via
>>> rtnetlink.
>>>
>>> The main changes are:
>>>   - when user dumps mfc entries it now get all entries, included the unresolved
>>>     cache.
>>>   - kernel sends rtnetlink when it adds/deletes mfc entries.
>>>
>>> As usual, the patch against iproute2 will be sent once the patches are
>>> included and
>>> net-next merged. I can send it on demand.
>>
>> This looks good, applied, thanks Nicolas.
>>
>> The one thing I worry about are those 64-bit statistics.  I fear that they
>> not be 64-bit aligned in the final netlink message.  This matters on cpus
>> that trap on unaligned loads/stores, such as sparc and MIPS.
>>
>> Can you validate this?
>>
> I can have a try on a tile platform. I don't have access to sparc or mips.
Hmm, I've read arm instead of mips! So I've tried on mips. Data are aligned on 
32-bit, like for all netlink messages. nla_put_u64() will do the same, as it 
calls nla_put().

And the kernel will only use memcpy() to treat this attribute. Reader will be in 
userland.

RE: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[David Laight]

> >> The one thing I worry about are those 64-bit statistics.  I fear that they
> >> not be 64-bit aligned in the final netlink message.  This matters on cpus
> >> that trap on unaligned loads/stores, such as sparc and MIPS.
> >>
> >> Can you validate this?
> >>
> > I can have a try on a tile platform. I don't have access to sparc or mips.

> Hmm, I've read arm instead of mips! So I've tried on mips. Data are aligned on
> 32-bit, like for all netlink messages. nla_put_u64() will do the same, as it
> calls nla_put().
> 
> And the kernel will only use memcpy() to treat this attribute. Reader will be in
> userland.

Probably worth commenting that the 64bit items might only be 32bit aligned.
Just to stop anyone trying to read/write them with pointer casts.

I think they are currently done with memcpy() - which should be ok.
It might be possibly to optimise by using a structure containing
a 64bit value marked __attribute__((aligned(4))).

	David

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 05 Dec 2012 12:02:50 +0100

> Le 04/12/2012 21:02, Nicolas Dichtel a écrit :
>> I can have a try on a tile platform. I don't have access to sparc or
>> mips.
> Hmm, I've read arm instead of mips! So I've tried on mips. Data are
> aligned on 32-bit, like for all netlink messages. nla_put_u64() will
> do the same, as it calls nla_put().
> 
> And the kernel will only use memcpy() to treat this attribute. Reader
> will be in userland.

Then userland will trap if the 64-bit values are only 32-bit aligned.

That's the problem I'm talking about.

I don't want to export any more unaligned 64-bit values in netlink
messages, it's a complete mess.

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[David Miller]

From: "David Laight" <David.Laight@ACULAB.COM>
Date: Wed, 5 Dec 2012 11:41:33 -0000

> Probably worth commenting that the 64bit items might only be 32bit aligned.
> Just to stop anyone trying to read/write them with pointer casts.

Rather, let's not create this situation at all.

It's totally inappropriate to have special code to handle every single
time we want to put 64-bit values into netlink messages.

We need a real solution to this issue.

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Nicolas Dichtel]

Le 05/12/2012 18:54, David Miller a écrit :
> From: "David Laight" <David.Laight@ACULAB.COM>
> Date: Wed, 5 Dec 2012 11:41:33 -0000
>
>> Probably worth commenting that the 64bit items might only be 32bit aligned.
>> Just to stop anyone trying to read/write them with pointer casts.
>
> Rather, let's not create this situation at all.
>
> It's totally inappropriate to have special code to handle every single
> time we want to put 64-bit values into netlink messages.
>
> We need a real solution to this issue.
>
The easiest way is to update *_ALIGNTO values (maybe we can keep NLMSG_ALIGNTO 
to 4). But I think that many userland apps have these values hardcoded and, the 
most important thing, this may increase size of many netlink messages. Hence we 
need probably to find something better.


diff --git a/include/uapi/linux/netfilter/nfnetlink_compat.h 
b/include/uapi/linux/netfilter/nfnetlink_compat.h
index ffb9503..121e62a 100644
--- a/include/uapi/linux/netfilter/nfnetlink_compat.h
+++ b/include/uapi/linux/netfilter/nfnetlink_compat.h
@@ -33,7 +33,7 @@ struct nfattr {
  #define NFNL_NFA_NEST	0x8000
  #define NFA_TYPE(attr) 	((attr)->nfa_type & 0x7fff)

-#define NFA_ALIGNTO     4
+#define NFA_ALIGNTO     8
  #define NFA_ALIGN(len)	(((len) + NFA_ALIGNTO - 1) & ~(NFA_ALIGNTO - 1))
  #define NFA_OK(nfa,len)	((len) > 0 && (nfa)->nfa_len >= sizeof(struct nfattr) \
  	&& (nfa)->nfa_len <= (len))
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 78d5b8a..66d2a26 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -75,7 +75,7 @@ struct nlmsghdr {
     Check		NLM_F_EXCL
   */

-#define NLMSG_ALIGNTO	4U
+#define NLMSG_ALIGNTO	8U
  #define NLMSG_ALIGN(len) ( ((len)+NLMSG_ALIGNTO-1) & ~(NLMSG_ALIGNTO-1) )
  #define NLMSG_HDRLEN	 ((int) NLMSG_ALIGN(sizeof(struct nlmsghdr)))
  #define NLMSG_LENGTH(len) ((len)+NLMSG_ALIGN(NLMSG_HDRLEN))
@@ -145,7 +145,7 @@ struct nlattr {
  #define NLA_F_NET_BYTEORDER	(1 << 14)
  #define NLA_TYPE_MASK		~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)

-#define NLA_ALIGNTO		4
+#define NLA_ALIGNTO		8
  #define NLA_ALIGN(len)		(((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
  #define NLA_HDRLEN		((int) NLA_ALIGN(sizeof(struct nlattr)))

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 33d29ce..ee898c1 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -146,7 +146,7 @@ struct rtattr {

  /* Macros to handle rtattributes */

-#define RTA_ALIGNTO	4
+#define RTA_ALIGNTO	8
  #define RTA_ALIGN(len) ( ((len)+RTA_ALIGNTO-1) & ~(RTA_ALIGNTO-1) )
  #define RTA_OK(rta,len) ((len) >= (int)sizeof(struct rtattr) && \
  			 (rta)->rta_len >= sizeof(struct rtattr) && \
@@ -322,7 +322,7 @@ struct rtnexthop {

  /* Macros to handle hexthops */

-#define RTNH_ALIGNTO	4
+#define RTNH_ALIGNTO	8
  #define RTNH_ALIGN(len) ( ((len)+RTNH_ALIGNTO-1) & ~(RTNH_ALIGNTO-1) )
  #define RTNH_OK(rtnh,len) ((rtnh)->rtnh_len >= sizeof(struct rtnexthop) && \
  			   ((int)(rtnh)->rtnh_len) <= (len))

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Thomas Graf]

On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
> Le 05/12/2012 18:54, David Miller a écrit :
> >From: "David Laight" <David.Laight@ACULAB.COM>
> >Date: Wed, 5 Dec 2012 11:41:33 -0000
> >
> >>Probably worth commenting that the 64bit items might only be 32bit aligned.
> >>Just to stop anyone trying to read/write them with pointer casts.
> >
> >Rather, let's not create this situation at all.
> >
> >It's totally inappropriate to have special code to handle every single
> >time we want to put 64-bit values into netlink messages.
> >
> >We need a real solution to this issue.
> >
> The easiest way is to update *_ALIGNTO values (maybe we can keep
> NLMSG_ALIGNTO to 4). But I think that many userland apps have these
> values hardcoded and, the most important thing, this may increase
> size of many netlink messages. Hence we need probably to find
> something better.

We can't do this, as you say, ALIGNTO is compiled into all the
binaries.

A simple backwards compatible workaround would be to include an
unknown, empty padding attribute if needed. That would be 4 bytes
in size and could be used to include padding as needed.

We could use nla_type = 0 as it is a reserved value that should
be available in all protocols. All readers (kernel and user space)
must ignore such an attribute just like any other unknown
attribute they encounter.

We could easily extend nla_put_u64() and variants to automatically
include such a padding attribute as needed.

The only situation that I can think of where this would not work
is if we have code like this:

   foo = nla_nest_start();
   for ([..])
      nla_put_u64([...])
   nla_nest_end([...])

and a reader would stupidly do a nla_for_each_attr() in user space
and assume all attributes found must be NLA_U64 without even
checking the length of the attribute.

I would say we take that risk and let such code die horribly.

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Nicolas Dichtel]

Le 06/12/2012 18:49, Thomas Graf a écrit :
> On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
>> Le 05/12/2012 18:54, David Miller a écrit :
>>> From: "David Laight" <David.Laight@ACULAB.COM>
>>> Date: Wed, 5 Dec 2012 11:41:33 -0000
>>>
>>>> Probably worth commenting that the 64bit items might only be 32bit aligned.
>>>> Just to stop anyone trying to read/write them with pointer casts.
>>>
>>> Rather, let's not create this situation at all.
>>>
>>> It's totally inappropriate to have special code to handle every single
>>> time we want to put 64-bit values into netlink messages.
>>>
>>> We need a real solution to this issue.
>>>
>> The easiest way is to update *_ALIGNTO values (maybe we can keep
>> NLMSG_ALIGNTO to 4). But I think that many userland apps have these
>> values hardcoded and, the most important thing, this may increase
>> size of many netlink messages. Hence we need probably to find
>> something better.
>
> We can't do this, as you say, ALIGNTO is compiled into all the
> binaries.
>
> A simple backwards compatible workaround would be to include an
> unknown, empty padding attribute if needed. That would be 4 bytes
> in size and could be used to include padding as needed.
>
> We could use nla_type = 0 as it is a reserved value that should
> be available in all protocols. All readers (kernel and user space)
> must ignore such an attribute just like any other unknown
> attribute they encounter.
>
> We could easily extend nla_put_u64() and variants to automatically
> include such a padding attribute as needed.
>
> The only situation that I can think of where this would not work
> is if we have code like this:
>
>     foo = nla_nest_start();
>     for ([..])
>        nla_put_u64([...])
>     nla_nest_end([...])
>
> and a reader would stupidly do a nla_for_each_attr() in user space
> and assume all attributes found must be NLA_U64 without even
> checking the length of the attribute.
>
> I would say we take that risk and let such code die horribly.
>
Ok, I can work to a patch next week if you want (I will be off until Tuesday).

RE: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[David Laight]

> On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
> > Le 05/12/2012 18:54, David Miller a écrit :
> > >From: "David Laight" <David.Laight@ACULAB.COM>
> > >Date: Wed, 5 Dec 2012 11:41:33 -0000
> > >
> > >>Probably worth commenting that the 64bit items might only be 32bit aligned.
> > >>Just to stop anyone trying to read/write them with pointer casts.
> > >
> > >Rather, let's not create this situation at all.
> > >
> > >It's totally inappropriate to have special code to handle every single
> > >time we want to put 64-bit values into netlink messages.
> > >
> > >We need a real solution to this issue.
> > >
> > The easiest way is to update *_ALIGNTO values (maybe we can keep
> > NLMSG_ALIGNTO to 4). But I think that many userland apps have these
> > values hardcoded and, the most important thing, this may increase
> > size of many netlink messages. Hence we need probably to find
> > something better.
> 
> We can't do this, as you say, ALIGNTO is compiled into all the
> binaries.
> 
> A simple backwards compatible workaround would be to include an
> unknown, empty padding attribute if needed. That would be 4 bytes
> in size and could be used to include padding as needed.

What are you going to align the data with respect to?
I doubt you can assume that the start of the netlink
message itself is 8 byte aligned - so any attempt
to 8 byte align an item is probably doomed to failure.

	David

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Thomas Graf]

On 12/07/12 at 10:38am, David Laight wrote:
> What are you going to align the data with respect to?
> I doubt you can assume that the start of the netlink
> message itself is 8 byte aligned - so any attempt
> to 8 byte align an item is probably doomed to failure.

Correct me if I'm wrong but skb->head will be aligned to
SKB_DATA_ALIGN() which as I understand is guaranted to
be 8 bytse aligned. So we can just add needed padding if
skb->data would not be aligned correctly after the next
netlink attribute header was added.

All user space needs to do is use a receive buffer that
is 8 byte aligned as well and we are good to go.

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Nicolas Dichtel]

Le 06/12/2012 18:49, Thomas Graf a écrit :
> On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
>> Le 05/12/2012 18:54, David Miller a écrit :
>>> From: "David Laight" <David.Laight@ACULAB.COM>
>>> Date: Wed, 5 Dec 2012 11:41:33 -0000
>>>
>>>> Probably worth commenting that the 64bit items might only be 32bit aligned.
>>>> Just to stop anyone trying to read/write them with pointer casts.
>>>
>>> Rather, let's not create this situation at all.
>>>
>>> It's totally inappropriate to have special code to handle every single
>>> time we want to put 64-bit values into netlink messages.
>>>
>>> We need a real solution to this issue.
>>>
>> The easiest way is to update *_ALIGNTO values (maybe we can keep
>> NLMSG_ALIGNTO to 4). But I think that many userland apps have these
>> values hardcoded and, the most important thing, this may increase
>> size of many netlink messages. Hence we need probably to find
>> something better.
>
> We can't do this, as you say, ALIGNTO is compiled into all the
> binaries.
>
> A simple backwards compatible workaround would be to include an
> unknown, empty padding attribute if needed. That would be 4 bytes
> in size and could be used to include padding as needed.
>
> We could use nla_type = 0 as it is a reserved value that should
> be available in all protocols. All readers (kernel and user space)
> must ignore such an attribute just like any other unknown
> attribute they encounter.
>
> We could easily extend nla_put_u64() and variants to automatically
> include such a padding attribute as needed.
In fact, it seems not so easy because most users of nlmsg_new() calculate
the exact needed length, thus if we add an unpredicted attribute, the message 
will be too small.

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Thomas Graf]

On 12/11/12 at 04:03pm, Nicolas Dichtel wrote:
> In fact, it seems not so easy because most users of nlmsg_new() calculate
> the exact needed length, thus if we add an unpredicted attribute,
> the message will be too small.

True, we would either need to fix the calculations by accounting
for an additional 4 bytes for each 64bit arg or just reserve an
additional fixed amount for padding per message in nlmsg_new().

Re: [PATCH net-next 0/7] Allow to monitor multicast cache event via rtnetlink

[Nicolas Dichtel]

Le 11/12/2012 19:40, Thomas Graf a écrit :
> On 12/11/12 at 04:03pm, Nicolas Dichtel wrote:
>> In fact, it seems not so easy because most users of nlmsg_new() calculate
>> the exact needed length, thus if we add an unpredicted attribute,
>> the message will be too small.
>
> True, we would either need to fix the calculations by accounting
> for an additional 4 bytes for each 64bit arg or just reserve an
> additional fixed amount for padding per message in nlmsg_new().
>
I would say that reserving additional space is better, because we also
need to align attributes that contain u64 fields:

struct attribute_foo {
    __u32 bar;
    __u32 bar2;
    __u64 foo;
};

I wonder if it is better to align all attribute on 64-bits or only u64 and
add a new function nla_put_align64() for attribute with u64 fields.

[PATCH] netlink: align attributes on 64-bits

[Nicolas Dichtel]

On 64 bits arch, we must ensure that attributes are always aligned on 64-bits
boundary. We do that by adding attributes of type 0, size 4 (alignment on
32-bits is already done) when needed. Attribute type 0 should be available and
unused in all netlink families.

Some callers of nlmsg_new() calculates the exact length of the attributes they
want to add to their netlink messages. Because we may add some unexpected
attributes type 0, we should take more room for that.

Note that I made the choice to align all kind of netlink attributes (even u8,
u16, ...) to simplify netlink API. Having two sort of nla_put() functions will
certainly be a source of wrong usage. Moreover, it ensures that all existing
code will be fine.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/netlink.h |  9 +++++++++
 lib/nlattr.c          | 11 ++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 9690b0f..aeb9fba 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -492,6 +492,15 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
  */
 static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
 {
+	/* Because attributes may be aligned on 64-bits boundary with fake
+	 * attribute (type 0, size 4 (attributes are 32-bits align by default)),
+	 * an exact payload size cannot be calculated. Hence, we need to reserve
+	 * more space for these attributes.
+	 * 128 is arbitrary: it allows to align up to 32 attributes.
+	 */
+	if (sizeof(void *) > 4 && payload < NLMSG_DEFAULT_SIZE)
+		payload = min(payload + 128, (size_t)NLMSG_DEFAULT_SIZE);
+
 	return alloc_skb(nlmsg_total_size(payload), flags);
 }
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 18eca78..29ace9f 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);
  */
 int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
 {
-	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
+	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), sizeof(void *)) ? 0 : 4;
+
+	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen) + align))
 		return -EMSGSIZE;
 
+	if (align) {
+		/* Goal is to add an attribute with size 4. We know that
+		 * NLA_HDRLEN is 4, hence payload is 0.
+		 */
+		__nla_reserve(skb, 0, 0);
+	}
+
 	__nla_put(skb, attrtype, attrlen, data);
 	return 0;
 }
-- 
1.8.0.1

Re: [PATCH] netlink: align attributes on 64-bits

[Ben Hutchings]

On Fri, 2012-12-14 at 14:16 +0100, Nicolas Dichtel wrote:
> On 64 bits arch, we must ensure that attributes are always aligned on 64-bits
> boundary. We do that by adding attributes of type 0, size 4 (alignment on
> 32-bits is already done) when needed. Attribute type 0 should be available and
> unused in all netlink families.
[...]
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);
>   */
>  int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>  {
> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), sizeof(void *)) ? 0 : 4;

The assumption here is that nothing needs to be aligned to a greater
width than that of a pointer.  However, for most 32-bit architectures
(i386 being an exception) the C ABI requires 64-bit alignment for 64-bit
types.  There may be cases where a mostly 32-bit processor really
requires 64-bit alignment, e.g. to load or save a pair of registers.

Ben.

> +	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen) + align))
>  		return -EMSGSIZE;
>  
> +	if (align) {
> +		/* Goal is to add an attribute with size 4. We know that
> +		 * NLA_HDRLEN is 4, hence payload is 0.
> +		 */
> +		__nla_reserve(skb, 0, 0);
> +	}
> +
>  	__nla_put(skb, attrtype, attrlen, data);
>  	return 0;
>  }

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 14/12/2012 16:49, Ben Hutchings a écrit :
> On Fri, 2012-12-14 at 14:16 +0100, Nicolas Dichtel wrote:
>> On 64 bits arch, we must ensure that attributes are always aligned on 64-bits
>> boundary. We do that by adding attributes of type 0, size 4 (alignment on
>> 32-bits is already done) when needed. Attribute type 0 should be available and
>> unused in all netlink families.
> [...]
>> --- a/lib/nlattr.c
>> +++ b/lib/nlattr.c
>> @@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);
>>    */
>>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>>   {
>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), sizeof(void *)) ? 0 : 4;
>
> The assumption here is that nothing needs to be aligned to a greater
> width than that of a pointer.  However, for most 32-bit architectures
> (i386 being an exception) the C ABI requires 64-bit alignment for 64-bit
> types.  There may be cases where a mostly 32-bit processor really
> requires 64-bit alignment, e.g. to load or save a pair of registers.
Ok, I will wait other comments to send a v2 (which will align these attributes 
for all arch).

Nicolas

RE: [PATCH] netlink: align attributes on 64-bits

[David Laight]

> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), sizeof(void *)) ? 0 : 4;
> +
> +	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen) + align))
>  		return -EMSGSIZE;
> 
> +	if (align) {
> +		/* Goal is to add an attribute with size 4. We know that
> +		 * NLA_HDRLEN is 4, hence payload is 0.
> +		 */
> +		__nla_reserve(skb, 0, 0);
> +	}
> +

Shouldn't the size of the dummy parameter be based on the value
of 'align' - and that be based on the amount of padding needed?

That aligns the write pointer, what guarantees the alignment of
the start of the buffer - so that the reader will find aligned data?

What guarantees that the reader will read the data into an
8-byte aligned buffer.

There is also the lurking issue of items that require more
than 8-byte alignment.
(x86/amd64 requires 16-byte alignment for 16-byte SSE2 regs and
32-byte alignment for the AVX regs.)

Will anyone ever want to put such items into a netlink message?

	David

[PATCH v2] netlink: align attributes on 64-bits

[Nicolas Dichtel]

We must ensure that attributes are always aligned on 64-bits boundary because
some arch may trap when accessing unaligned 64 bits value. We do that by adding
attributes of type 0, size 4 (alignment on 32-bits is already done) when needed.
Attribute type 0 should be available and unused in all netlink families.

Some callers of nlmsg_new() calculates the exact length of the attributes they
want to add to their netlink messages. Because we may add some unexpected
attributes type 0, we should take more room for that.

Note that I made the choice to align all kind of netlink attributes (even u8,
u16, ...) to simplify netlink API. Having two sort of nla_put() functions will
certainly be a source of wrong usage. Moreover, it ensures that all existing
code will be fine.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v2: align attributes on all arch, not only on 64-bits arch

 include/net/netlink.h |  9 +++++++++
 lib/nlattr.c          | 11 ++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 9690b0f..bd9e48f 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -492,6 +492,15 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
  */
 static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
 {
+	/* Because attributes may be aligned on 64-bits boundary with fake
+	 * attribute (type 0, size 4 (attributes are 32-bits align by default)),
+	 * an exact payload size cannot be calculated. Hence, we need to reserve
+	 * more space for these attributes.
+	 * 128 is arbitrary: it allows to align up to 32 attributes.
+	 */
+	if (payload < NLMSG_DEFAULT_SIZE)
+		payload = min(payload + 128, (size_t)NLMSG_DEFAULT_SIZE);
+
 	return alloc_skb(nlmsg_total_size(payload), flags);
 }
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 18eca78..7440a80 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);
  */
 int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
 {
-	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
+	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
+
+	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen) + align))
 		return -EMSGSIZE;
 
+	if (align) {
+		/* Goal is to add an attribute with size 4. We know that
+		 * NLA_HDRLEN is 4, hence payload is 0.
+		 */
+		__nla_reserve(skb, 0, 0);
+	}
+
 	__nla_put(skb, attrtype, attrlen, data);
 	return 0;
 }
-- 
1.8.0.1

Re: [PATCH] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 17/12/2012 10:59, David Laight a écrit :
>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), sizeof(void *)) ? 0 : 4;
>> +
>> +	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen) + align))
>>   		return -EMSGSIZE;
>>
>> +	if (align) {
>> +		/* Goal is to add an attribute with size 4. We know that
>> +		 * NLA_HDRLEN is 4, hence payload is 0.
>> +		 */
>> +		__nla_reserve(skb, 0, 0);
>> +	}
>> +
>
> Shouldn't the size of the dummy parameter be based on the value
> of 'align' - and that be based on the amount of padding needed?
>
Align is 4 or 0. Instead of the comment and 0, I can put 'NLA_HDRLEN - align', 
which will always be 0, because we made this patch because we don't want to 
change values like NLA_HDRLEN, because many user apps have these values 
/structures hardcoded.

> That aligns the write pointer, what guarantees the alignment of
> the start of the buffer - so that the reader will find aligned data?
As Thomas said, skb->head will be aligned, am I wrong?

>
> What guarantees that the reader will read the data into an
> 8-byte aligned buffer.
>
> There is also the lurking issue of items that require more
> than 8-byte alignment.
> (x86/amd64 requires 16-byte alignment for 16-byte SSE2 regs and
> 32-byte alignment for the AVX regs.)
>
> Will anyone ever want to put such items into a netlink message?
>
> 	David

RE: [PATCH v2] netlink: align attributes on 64-bits

[David Laight]

>  int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>  {
> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;

I've just realised where you are adding this!
You only want to add pad if the attribute is a single 64bit item,
not whenever the destination is misaligned.

Eg what happens if you add a 4-byte item after an 8 byte one.

Are there are attributes that consist of a pair of 4 byte values?

...
> +	if (align) {
> +		/* Goal is to add an attribute with size 4. We know that
> +		 * NLA_HDRLEN is 4, hence payload is 0.
> +		 */
> +		__nla_reserve(skb, 0, 0);

One of those zeros should be 'align - 4', then the comment
can be more descriptive.

	David

Re: [PATCH v2] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 17/12/2012 18:06, David Laight a écrit :
>>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>>   {
>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
>
> I've just realised where you are adding this!
> You only want to add pad if the attribute is a single 64bit item,
> not whenever the destination is misaligned.
As said in the commit log, I want to align all attributes. An attribute can be 
like this:

struct foo {
	__u32 bar1;
	__u32 bar2;
	__u64 bar3;
}

nla_put() don't know what is contained in the attribute.

>
> Eg what happens if you add a 4-byte item after an 8 byte one.
>
> Are there are attributes that consist of a pair of 4 byte values?
>
> ...
>> +	if (align) {
>> +		/* Goal is to add an attribute with size 4. We know that
>> +		 * NLA_HDRLEN is 4, hence payload is 0.
>> +		 */
>> +		__nla_reserve(skb, 0, 0);
>
> One of those zeros should be 'align - 4', then the comment
> can be more descriptive.
I thought if you were to research why we use 0, you would know that the first 0 
is the type and the second is the payload size...

RE: [PATCH v2] netlink: align attributes on 64-bits

[David Laight]

> Le 17/12/2012 18:06, David Laight a écrit :
> >>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
> >>   {
> >> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> >> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
> >
> > I've just realised where you are adding this!
> > You only want to add pad if the attribute is a single 64bit item,
> > not whenever the destination is misaligned.
> As said in the commit log, I want to align all attributes. An attribute can be
> like this:
> 
> struct foo {
> 	__u32 bar1;
> 	__u32 bar2;
> 	__u64 bar3;
> }
> 
> nla_put() don't know what is contained in the attribute.

Put there is no need to 8-byte align something whose size isn't a
multiple of 8 bytes.

> > ...
> >> +	if (align) {
> >> +		/* Goal is to add an attribute with size 4. We know that
> >> +		 * NLA_HDRLEN is 4, hence payload is 0.
> >> +		 */
> >> +		__nla_reserve(skb, 0, 0);
> >
> > One of those zeros should be 'align - 4', then the comment
> > can be more descriptive.

> I thought if you were to research why we use 0, you would know that the first 0
> is the type and the second is the payload size...

I can tell that one is the type and the other the size, you've
implied that the 'type+size' actually total 4 bytes.
I don't need to find out which is which!
Now you've told me I'd have written:
	_nla_reserve(skb, 0, align - NLA_HDRLEN);

The compiler could well have tracked the value - so know it is 4.
OTOH you might want to generate the size of 'align' without
using a conditional.

	David

Re: [PATCH v2] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 18/12/2012 10:19, David Laight a écrit :
>> Le 17/12/2012 18:06, David Laight a écrit :
>>>>    int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>>>>    {
>>>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>>>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
>>>
>>> I've just realised where you are adding this!
>>> You only want to add pad if the attribute is a single 64bit item,
>>> not whenever the destination is misaligned.
>> As said in the commit log, I want to align all attributes. An attribute can be
>> like this:
>>
>> struct foo {
>> 	__u32 bar1;
>> 	__u32 bar2;
>> 	__u64 bar3;
>> }
>>
>> nla_put() don't know what is contained in the attribute.
>
> Put there is no need to 8-byte align something whose size isn't a
> multiple of 8 bytes.
Even if you cast the structure in a buffer and read bar3 (without any memcpy 
before)?

>
>>> ...
>>>> +	if (align) {
>>>> +		/* Goal is to add an attribute with size 4. We know that
>>>> +		 * NLA_HDRLEN is 4, hence payload is 0.
>>>> +		 */
>>>> +		__nla_reserve(skb, 0, 0);
>>>
>>> One of those zeros should be 'align - 4', then the comment
>>> can be more descriptive.
>
>> I thought if you were to research why we use 0, you would know that the first 0
>> is the type and the second is the payload size...
>
> I can tell that one is the type and the other the size, you've
> implied that the 'type+size' actually total 4 bytes.
> I don't need to find out which is which!
> Now you've told me I'd have written:
> 	_nla_reserve(skb, 0, align - NLA_HDRLEN);
>
> The compiler could well have tracked the value - so know it is 4.
> OTOH you might want to generate the size of 'align' without
> using a conditional.
Yes, it is the goal ;-)

Re: [PATCH v2] netlink: align attributes on 64-bits

[Thomas Graf]

On 12/17/12 at 05:49pm, Nicolas Dichtel wrote:
> @@ -492,6 +492,15 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
>   */
>  static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
>  {
> +	/* Because attributes may be aligned on 64-bits boundary with fake
> +	 * attribute (type 0, size 4 (attributes are 32-bits align by default)),
> +	 * an exact payload size cannot be calculated. Hence, we need to reserve
> +	 * more space for these attributes.
> +	 * 128 is arbitrary: it allows to align up to 32 attributes.
> +	 */
> +	if (payload < NLMSG_DEFAULT_SIZE)
> +		payload = min(payload + 128, (size_t)NLMSG_DEFAULT_SIZE);

This is doomed to fail eventually. A netlink message may carry
hundreds of attributes eventually. See my suggestion below.

> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 18eca78..7440a80 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);
>   */
>  int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>  {
> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;

This does not look right. In order for the attribute data to be
aligned properly you would need to check skb_tail_pointer(skb) +
NLA_HDRLEN for proper alignment or you end up aligning the
attribute header.

How about we change nla_total_size() to return the size with
needed padding taken into account. That should fix the message
size caluclation problem and we only need to reserve room for
the initial padding to align the very first attribute.

Below is an untested patch that does this. What do you think?

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 9690b0f..7ce8e76 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -114,7 +114,6 @@
  * Attribute Length Calculations:
  *   nla_attr_size(payload)		length of attribute w/o padding
  *   nla_total_size(payload)		length of attribute w/ padding
- *   nla_padlen(payload)		length of padding
  *
  * Attribute Payload Access:
  *   nla_data(nla)			head of attribute payload
@@ -492,6 +491,14 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
  */
 static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
 {
+	/* If an exact size if specified, reserve some additional space to
+	 * align the first attribute, all subsequent attributes should have
+	 * padding accounted for.
+	 */
+	if (payload != NLMSG_DEFAULT_SIZE)
+		payload = min_t(size_t, payload + NLA_ATTR_ALIGN,
+				NLMSG_DEFAULT_SIZE);
+
 	return alloc_skb(nlmsg_total_size(payload), flags);
 }
 
@@ -653,16 +660,12 @@ static inline int nla_attr_size(int payload)
  */
 static inline int nla_total_size(int payload)
 {
-	return NLA_ALIGN(nla_attr_size(payload));
-}
+	size_t len = NLA_ALIGN(nla_attr_size(payload));
 
-/**
- * nla_padlen - length of padding at the tail of attribute
- * @payload: length of payload
- */
-static inline int nla_padlen(int payload)
-{
-	return nla_total_size(payload) - nla_attr_size(payload);
+	if (!IS_ALIGNED(len, NLA_ATTR_ALIGN))
+		len = ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN);
+
+	return len;
 }
 
 /**
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 78d5b8a..1856729 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -149,5 +149,10 @@ struct nlattr {
 #define NLA_ALIGN(len)		(((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
 #define NLA_HDRLEN		((int) NLA_ALIGN(sizeof(struct nlattr)))
 
+/* Padding attribute type, added automatically to align attributes,
+ * must be ignored by readers. */
+#define NLA_PADDING		0
+#define NLA_ATTR_ALIGN		8
+
 
 #endif /* _UAPI__LINUX_NETLINK_H */
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 18eca78..b09473c 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -322,18 +322,36 @@ int nla_strcmp(const struct nlattr *nla, const char *str)
  * Adds a netlink attribute header to a socket buffer and reserves
  * room for the payload but does not copy it.
  *
+ * May add a padding attribute of type NLA_PADDING before the
+ * real attribute to ensure proper alignment.
+ *
  * The caller is responsible to ensure that the skb provides enough
  * tailroom for the attribute header and payload.
  */
 struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
 {
 	struct nlattr *nla;
+	size_t offset;
+
+	offset = (size_t) skb_tail_pointer(skb);
+	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN)) {
+		struct nlattr *pad;
+		size_t padlen;
+
+		padlen = nla_total_size(offset) - offset - NLA_HDRLEN;
+		pad = (struct nlattr *) skb_put(skb, nla_attr_size(padlen));
+		pad->nla_type = 0;
+		pad->nla_len = nla_attr_size(padlen);
+
+		memset((unsigned char *) pad + NLA_HDRLEN, 0, padlen);
+	}
 
-	nla = (struct nlattr *) skb_put(skb, nla_total_size(attrlen));
+	nla = (struct nlattr *) skb_put(skb, NLA_ALIGN(nla_attr_size(attrlen)));
 	nla->nla_type = attrtype;
 	nla->nla_len = nla_attr_size(attrlen);
 
-	memset((unsigned char *) nla + nla->nla_len, 0, nla_padlen(attrlen));
+	memset((unsigned char *) nla + nla->nla_len, 0,
+	       NLA_ALIGN(nla->nla_len) - nla->nla_len);
 
 	return nla;
 }
@@ -360,6 +378,23 @@ void *__nla_reserve_nohdr(struct sk_buff *skb, int attrlen)
 }
 EXPORT_SYMBOL(__nla_reserve_nohdr);
 
+static size_t nla_pre_padlen(struct sk_buff *skb)
+{
+	size_t offset = (size_t) skb_tail_pointer(skb);
+
+	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN))
+		return nla_total_size(offset) - offset - NLA_HDRLEN;
+
+	return 0;
+}
+
+static bool nla_insufficient_space(struct sk_buff *skb, int attrlen)
+{
+	size_t needed = nla_pre_padlen(skb) + nla_total_size(attrlen);
+
+	return (skb_tailroom(skb) < needed);
+}
+
 /**
  * nla_reserve - reserve room for attribute on the skb
  * @skb: socket buffer to reserve room on
@@ -374,7 +409,7 @@ EXPORT_SYMBOL(__nla_reserve_nohdr);
  */
 struct nlattr *nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
 {
-	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
+	if (unlikely(nla_insufficient_space(skb, attrlen)))
 		return NULL;
 
 	return __nla_reserve(skb, attrtype, attrlen);
@@ -450,7 +485,7 @@ EXPORT_SYMBOL(__nla_put_nohdr);
  */
 int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
 {
-	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
+	if (unlikely(nla_insufficient_space(skb, attrlen)))
 		return -EMSGSIZE;
 
 	__nla_put(skb, attrtype, attrlen, data);

Re: [PATCH v2] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 18/12/2012 13:57, Thomas Graf a écrit :
> On 12/17/12 at 05:49pm, Nicolas Dichtel wrote:
>> @@ -492,6 +492,15 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
>>    */
>>   static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
>>   {
>> +	/* Because attributes may be aligned on 64-bits boundary with fake
>> +	 * attribute (type 0, size 4 (attributes are 32-bits align by default)),
>> +	 * an exact payload size cannot be calculated. Hence, we need to reserve
>> +	 * more space for these attributes.
>> +	 * 128 is arbitrary: it allows to align up to 32 attributes.
>> +	 */
>> +	if (payload < NLMSG_DEFAULT_SIZE)
>> +		payload = min(payload + 128, (size_t)NLMSG_DEFAULT_SIZE);
>
> This is doomed to fail eventually. A netlink message may carry
> hundreds of attributes eventually. See my suggestion below.
>
>> diff --git a/lib/nlattr.c b/lib/nlattr.c
>> index 18eca78..7440a80 100644
>> --- a/lib/nlattr.c
>> +++ b/lib/nlattr.c
>> @@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);
>>    */
>>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>>   {
>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
>
> This does not look right. In order for the attribute data to be
> aligned properly you would need to check skb_tail_pointer(skb) +
> NLA_HDRLEN for proper alignment or you end up aligning the
> attribute header.
Good point.

>
> How about we change nla_total_size() to return the size with
> needed padding taken into account. That should fix the message
> size caluclation problem and we only need to reserve room for
> the initial padding to align the very first attribute.
>
> Below is an untested patch that does this. What do you think?
I still have some doubts about the size calculation (see bellow).
For the rest of the patch, it seems ok (except some minor point). I will test it.

>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 9690b0f..7ce8e76 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -114,7 +114,6 @@
>    * Attribute Length Calculations:
>    *   nla_attr_size(payload)		length of attribute w/o padding
>    *   nla_total_size(payload)		length of attribute w/ padding
> - *   nla_padlen(payload)		length of padding
>    *
>    * Attribute Payload Access:
>    *   nla_data(nla)			head of attribute payload
> @@ -492,6 +491,14 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
>    */
>   static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
>   {
> +	/* If an exact size if specified, reserve some additional space to
> +	 * align the first attribute, all subsequent attributes should have
> +	 * padding accounted for.
> +	 */
> +	if (payload != NLMSG_DEFAULT_SIZE)
> +		payload = min_t(size_t, payload + NLA_ATTR_ALIGN,
> +				NLMSG_DEFAULT_SIZE);
> +
>   	return alloc_skb(nlmsg_total_size(payload), flags);
>   }
>
> @@ -653,16 +660,12 @@ static inline int nla_attr_size(int payload)
>    */
>   static inline int nla_total_size(int payload)
>   {
> -	return NLA_ALIGN(nla_attr_size(payload));
> -}
> +	size_t len = NLA_ALIGN(nla_attr_size(payload));
>
> -/**
> - * nla_padlen - length of padding at the tail of attribute
> - * @payload: length of payload
> - */
> -static inline int nla_padlen(int payload)
> -{
> -	return nla_total_size(payload) - nla_attr_size(payload);
> +	if (!IS_ALIGNED(len, NLA_ATTR_ALIGN))
> +		len = ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN);
Two comments:
1/ should it be ALIGN(len, NLA_ATTR_ALIGN)? If we want to add a __u64:
    => nla_attr_size(sizeof(__u64)) = 12
    => NLA_ALIGN(nla_attr_size(sizeof(__u64))) => 12 (= len)
    => ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN) = 0 but it should be 4

2/ Suppose that the attribute is:

   struct foo {
   	__u64 bar1;
   	__u32 bar2;
   }
   => sizeof(struct foo) = 12 (= payload)
   => nla_attr_size(payload) = 16
   => NLA_ALIGN(nla_attr_size(payload)) = 16 (= len)
   => IS_ALIGNED(len, NLA_ATTR_ALIGN) = true
   => extra room is not reserved
   But it's not guaranteed that bar1 is aligned on 8 bytes, only on 4 bytes.

> +
> +	return len;
>   }
>
>   /**
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 78d5b8a..1856729 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -149,5 +149,10 @@ struct nlattr {
>   #define NLA_ALIGN(len)		(((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
>   #define NLA_HDRLEN		((int) NLA_ALIGN(sizeof(struct nlattr)))
>
> +/* Padding attribute type, added automatically to align attributes,
> + * must be ignored by readers. */
> +#define NLA_PADDING		0
> +#define NLA_ATTR_ALIGN		8
> +
>
>   #endif /* _UAPI__LINUX_NETLINK_H */
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 18eca78..b09473c 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -322,18 +322,36 @@ int nla_strcmp(const struct nlattr *nla, const char *str)
>    * Adds a netlink attribute header to a socket buffer and reserves
>    * room for the payload but does not copy it.
>    *
> + * May add a padding attribute of type NLA_PADDING before the
> + * real attribute to ensure proper alignment.
> + *
>    * The caller is responsible to ensure that the skb provides enough
>    * tailroom for the attribute header and payload.
>    */
>   struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
>   {
>   	struct nlattr *nla;
> +	size_t offset;
> +
> +	offset = (size_t) skb_tail_pointer(skb);
> +	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN)) {
With the previous struct foo, this test may be true even if we don't have 
reserved extra room. This test depends on previous attribute.
I think the exact size of the netlink message depends on the order of 
attributes, not only on the attribute itself.
What about taking the assumption that the start will never be aligned and always 
allocating extra room: ALIGN(NLA_ALIGNTO, NLA_ATTR_ALIGN) (= 4)?

> +		struct nlattr *pad;
> +		size_t padlen;
> +
> +		padlen = nla_total_size(offset) - offset - NLA_HDRLEN;
> +		pad = (struct nlattr *) skb_put(skb, nla_attr_size(padlen));
> +		pad->nla_type = 0;
> +		pad->nla_len = nla_attr_size(padlen);
> +
> +		memset((unsigned char *) pad + NLA_HDRLEN, 0, padlen);
> +	}
>
> -	nla = (struct nlattr *) skb_put(skb, nla_total_size(attrlen));
> +	nla = (struct nlattr *) skb_put(skb, NLA_ALIGN(nla_attr_size(attrlen)));
>   	nla->nla_type = attrtype;
>   	nla->nla_len = nla_attr_size(attrlen);
>
> -	memset((unsigned char *) nla + nla->nla_len, 0, nla_padlen(attrlen));
> +	memset((unsigned char *) nla + nla->nla_len, 0,
> +	       NLA_ALIGN(nla->nla_len) - nla->nla_len);
>
>   	return nla;
>   }
> @@ -360,6 +378,23 @@ void *__nla_reserve_nohdr(struct sk_buff *skb, int attrlen)
>   }
>   EXPORT_SYMBOL(__nla_reserve_nohdr);
>
> +static size_t nla_pre_padlen(struct sk_buff *skb)
> +{
> +	size_t offset = (size_t) skb_tail_pointer(skb);
> +
> +	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN))
> +		return nla_total_size(offset) - offset - NLA_HDRLEN;
> +
> +	return 0;
> +}
> +
> +static bool nla_insufficient_space(struct sk_buff *skb, int attrlen)
> +{
> +	size_t needed = nla_pre_padlen(skb) + nla_total_size(attrlen);
If nla_total_size() was right, nla_pre_padlen(skb) should already be included. 
Am I wrong?

> +
> +	return (skb_tailroom(skb) < needed);
> +}
> +
>   /**
>    * nla_reserve - reserve room for attribute on the skb
>    * @skb: socket buffer to reserve room on
> @@ -374,7 +409,7 @@ EXPORT_SYMBOL(__nla_reserve_nohdr);
>    */
>   struct nlattr *nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
>   {
> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	if (unlikely(nla_insufficient_space(skb, attrlen)))
>   		return NULL;
>
>   	return __nla_reserve(skb, attrtype, attrlen);
> @@ -450,7 +485,7 @@ EXPORT_SYMBOL(__nla_put_nohdr);
>    */
>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>   {
> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
> +	if (unlikely(nla_insufficient_space(skb, attrlen)))
>   		return -EMSGSIZE;
>
>   	__nla_put(skb, attrtype, attrlen, data);
>

RE: [PATCH v2] netlink: align attributes on 64-bits

[David Laight]

> 2/ Suppose that the attribute is:
> 
>    struct foo {
>    	__u64 bar1;
>    	__u32 bar2;
>    }
>    => sizeof(struct foo) = 12 (= payload)

That is only true if the host architecture aligns 64bit items
on 32 it boundaries (as i386 does).
Otherwise there are 4 bytes of padding at the end and the
size is 16.

Actually it is worse than that.
Consider the structure:
	struct bar {
		__u32 foo1;
		__u64 foo2;
	}
On i386 it will have size 12 and foo2 will be at offset 4.
On sparc32 (and most 64bit) it will have size 16 with foo2
at offset 8 (and 4 bytes of pad after foo1).

Do these messages move between systems?
If they do then any 64bit items need an explicit alignment
eg tag with __attribute__((aligned(8))) (or aligned(4)).

	David

Re: [PATCH v2] netlink: align attributes on 64-bits

[Thomas Graf]

On 12/18/12 at 05:23pm, Nicolas Dichtel wrote:
> Le 18/12/2012 13:57, Thomas Graf a écrit :
> >-static inline int nla_padlen(int payload)
> >-{
> >-	return nla_total_size(payload) - nla_attr_size(payload);
> >+	if (!IS_ALIGNED(len, NLA_ATTR_ALIGN))
> >+		len = ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN);
> Two comments:
> 1/ should it be ALIGN(len, NLA_ATTR_ALIGN)? If we want to add a __u64:
>    => nla_attr_size(sizeof(__u64)) = 12
>    => NLA_ALIGN(nla_attr_size(sizeof(__u64))) => 12 (= len)
>    => ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN) = 0 but it should be 4

We can't add 1-3 bytes of padding, therefore we need to add
NLA_HDRLEN to len before aligning it to enforce a minimal
padding. We can't hit it right now because 4 byte alignment
of the previous attribute is a given but if we ever change
the alignment it could become an issue and the above should
be bullet proof.

Your example would come out like this:
  nla_attr_size(8) = 12
  ALIGN(12 + 4, 8) = 16

> 2/ Suppose that the attribute is:
> 
>   struct foo {
>   	__u64 bar1;
>   	__u32 bar2;
>   }
>   => sizeof(struct foo) = 12 (= payload)
>   => nla_attr_size(payload) = 16
>   => NLA_ALIGN(nla_attr_size(payload)) = 16 (= len)
>   => IS_ALIGNED(len, NLA_ATTR_ALIGN) = true
>   => extra room is not reserved
>   But it's not guaranteed that bar1 is aligned on 8 bytes, only on 4 bytes.

That's correct, that's why I have added the additional
NLA_ATTR_ALIGN of room in nlmsg_new(). It will account
for the one time padding that is needed before we add
the very first attribute.

If all attributes after that have a size aligned to 8
bytes no padding is needed. Padding will only be needed
again if a struct is missized in which case we reserve
room with the above. Correct?

> >+	offset = (size_t) skb_tail_pointer(skb);
> >+	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN)) {
> With the previous struct foo, this test may be true even if we don't
> have reserved extra room. This test depends on previous attribute.
> I think the exact size of the netlink message depends on the order
> of attributes, not only on the attribute itself.
> What about taking the assumption that the start will never be
> aligned and always allocating extra room: ALIGN(NLA_ALIGNTO,
> NLA_ATTR_ALIGN) (= 4)?

See my explanation above. I think this works. The order does not
matter, the sum of all padding required will always be the same.

> >+static bool nla_insufficient_space(struct sk_buff *skb, int attrlen)
> >+{
> >+	size_t needed = nla_pre_padlen(skb) + nla_total_size(attrlen);
> If nla_total_size() was right, nla_pre_padlen(skb) should already be
> included. Am I wrong?

No, nla_pre_padlen() contains the number of bytes needed to align
skb_tail_pointer() to an alignment of 8. If that is > 0 but the
attribute to follow is already aligned.

The tricky part here is that accounting for padding in
nla_total_size() only works for the sum of all attributes.
It does not account for the specific padding required for the
previous attribute.

Therefore the above check. The above could be changed to
nla_attr_size() theoretically as we don't need space for the
final padding eventually but we checked for space before so I
kept it that way.

I realize it's slightly confusign and needs better documentation
and please double check my thinking :-)

Re: [PATCH v2] netlink: align attributes on 64-bits

[Thomas Graf]

On 12/18/12 at 04:50pm, David Laight wrote:
> > 2/ Suppose that the attribute is:
> > 
> >    struct foo {
> >    	__u64 bar1;
> >    	__u32 bar2;
> >    }
> >    => sizeof(struct foo) = 12 (= payload)
> 
> That is only true if the host architecture aligns 64bit items
> on 32 it boundaries (as i386 does).
> Otherwise there are 4 bytes of padding at the end and the
> size is 16.
> 
> Actually it is worse than that.
> Consider the structure:
> 	struct bar {
> 		__u32 foo1;
> 		__u64 foo2;
> 	}
> On i386 it will have size 12 and foo2 will be at offset 4.
> On sparc32 (and most 64bit) it will have size 16 with foo2
> at offset 8 (and 4 bytes of pad after foo1).

This is a known problem and I can't think of anything
that can be done about it except for memcpy()ing the
data before accessing it.

If you have ideas, I'm more that willing to listen :)

> Do these messages move between systems?
> If they do then any 64bit items need an explicit alignment
> eg tag with __attribute__((aligned(8))) (or aligned(4)).

They don't. Netlink has and will be host bound. It also
uses host byte order for that reason.

Re: [PATCH v2] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 18/12/2012 18:08, Thomas Graf a écrit :
> On 12/18/12 at 05:23pm, Nicolas Dichtel wrote:
>> Le 18/12/2012 13:57, Thomas Graf a écrit :
>>> -static inline int nla_padlen(int payload)
>>> -{
>>> -	return nla_total_size(payload) - nla_attr_size(payload);
>>> +	if (!IS_ALIGNED(len, NLA_ATTR_ALIGN))
>>> +		len = ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN);
>> Two comments:
>> 1/ should it be ALIGN(len, NLA_ATTR_ALIGN)? If we want to add a __u64:
>>     => nla_attr_size(sizeof(__u64)) = 12
>>     => NLA_ALIGN(nla_attr_size(sizeof(__u64))) => 12 (= len)
>>     => ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN) = 0 but it should be 4
>
> We can't add 1-3 bytes of padding, therefore we need to add
> NLA_HDRLEN to len before aligning it to enforce a minimal
> padding. We can't hit it right now because 4 byte alignment
> of the previous attribute is a given but if we ever change
> the alignment it could become an issue and the above should
> be bullet proof.
>
> Your example would come out like this:
>    nla_attr_size(8) = 12
>    ALIGN(12 + 4, 8) = 16
Got it, right.

>
>> 2/ Suppose that the attribute is:
>>
>>    struct foo {
>>    	__u64 bar1;
>>    	__u32 bar2;
>>    }
>>    => sizeof(struct foo) = 12 (= payload)
>>    => nla_attr_size(payload) = 16
>>    => NLA_ALIGN(nla_attr_size(payload)) = 16 (= len)
>>    => IS_ALIGNED(len, NLA_ATTR_ALIGN) = true
>>    => extra room is not reserved
>>    But it's not guaranteed that bar1 is aligned on 8 bytes, only on 4 bytes.
>
> That's correct, that's why I have added the additional
> NLA_ATTR_ALIGN of room in nlmsg_new(). It will account
> for the one time padding that is needed before we add
> the very first attribute.
>
> If all attributes after that have a size aligned to 8
> bytes no padding is needed. Padding will only be needed
> again if a struct is missized in which case we reserve
> room with the above. Correct?
Seems good ;-)

>
>>> +	offset = (size_t) skb_tail_pointer(skb);
>>> +	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN)) {
>> With the previous struct foo, this test may be true even if we don't
>> have reserved extra room. This test depends on previous attribute.
>> I think the exact size of the netlink message depends on the order
>> of attributes, not only on the attribute itself.
>> What about taking the assumption that the start will never be
>> aligned and always allocating extra room: ALIGN(NLA_ALIGNTO,
>> NLA_ATTR_ALIGN) (= 4)?
>
> See my explanation above. I think this works. The order does not
> matter, the sum of all padding required will always be the same.
I will do more test.

>
>>> +static bool nla_insufficient_space(struct sk_buff *skb, int attrlen)
>>> +{
>>> +	size_t needed = nla_pre_padlen(skb) + nla_total_size(attrlen);
>> If nla_total_size() was right, nla_pre_padlen(skb) should already be
>> included. Am I wrong?
>
> No, nla_pre_padlen() contains the number of bytes needed to align
> skb_tail_pointer() to an alignment of 8. If that is > 0 but the
> attribute to follow is already aligned.
>
> The tricky part here is that accounting for padding in
> nla_total_size() only works for the sum of all attributes.
> It does not account for the specific padding required for the
> previous attribute.
>
> Therefore the above check. The above could be changed to
> nla_attr_size() theoretically as we don't need space for the
> final padding eventually but we checked for space before so I
> kept it that way.
>
> I realize it's slightly confusign and needs better documentation
> and please double check my thinking :-)
Ok, you convince me ;-)

RE: [PATCH v2] netlink: align attributes on 64-bits

[David Laight]

> > Consider the structure:
> > 	struct bar {
> > 		__u32 foo1;
> > 		__u64 foo2;
> > 	}
> > On i386 it will have size 12 and foo2 will be at offset 4.
> > On sparc32 (and most 64bit) it will have size 16 with foo2
> > at offset 8 (and 4 bytes of pad after foo1).
> 
> This is a known problem and I can't think of anything
> that can be done about it except for memcpy()ing the
> data before accessing it.

You can't use memcpy() to copy a pointer to a misaligned
structure into an aligned buffer. The compiler assumes
the pointer is aligned and will use instructions that
depend on the alignment.

> If you have ideas, I'm more that willing to listen :)
> ... Netlink has and will be host bound. It also
> uses host byte order for that reason.

I think:
1) Alignment is only needed on systems that have 'strict alignment'
   requirements (maybe disable for testing?)
2) Alignment is only needed for parameters whose size is a
   multiple of the alignment (a structure containing a
   field that needs 8 byte alignment will always be a multiple
   of 8 bytes long).
3) You need to add NA_HDR_LEN to the write pointer before
   determining the size of the pad.

So a structure of three uint32_t will never need aligning.

	David

Re: [PATCH v2] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 18/12/2012 13:57, Thomas Graf a écrit :
> On 12/17/12 at 05:49pm, Nicolas Dichtel wrote:
>> @@ -492,6 +492,15 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
>>    */
>>   static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
>>   {
>> +	/* Because attributes may be aligned on 64-bits boundary with fake
>> +	 * attribute (type 0, size 4 (attributes are 32-bits align by default)),
>> +	 * an exact payload size cannot be calculated. Hence, we need to reserve
>> +	 * more space for these attributes.
>> +	 * 128 is arbitrary: it allows to align up to 32 attributes.
>> +	 */
>> +	if (payload < NLMSG_DEFAULT_SIZE)
>> +		payload = min(payload + 128, (size_t)NLMSG_DEFAULT_SIZE);
>
> This is doomed to fail eventually. A netlink message may carry
> hundreds of attributes eventually. See my suggestion below.
>
>> diff --git a/lib/nlattr.c b/lib/nlattr.c
>> index 18eca78..7440a80 100644
>> --- a/lib/nlattr.c
>> +++ b/lib/nlattr.c
>> @@ -450,9 +450,18 @@ EXPORT_SYMBOL(__nla_put_nohdr);
>>    */
>>   int nla_put(struct sk_buff *skb, int attrtype, int attrlen, const void *data)
>>   {
>> -	if (unlikely(skb_tailroom(skb) < nla_total_size(attrlen)))
>> +	int align = IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8) ? 0 : 4;
>
> This does not look right. In order for the attribute data to be
> aligned properly you would need to check skb_tail_pointer(skb) +
> NLA_HDRLEN for proper alignment or you end up aligning the
> attribute header.
>
> How about we change nla_total_size() to return the size with
> needed padding taken into account. That should fix the message
> size caluclation problem and we only need to reserve room for
> the initial padding to align the very first attribute.
>
> Below is an untested patch that does this. What do you think?
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 9690b0f..7ce8e76 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -114,7 +114,6 @@
>    * Attribute Length Calculations:
>    *   nla_attr_size(payload)		length of attribute w/o padding
>    *   nla_total_size(payload)		length of attribute w/ padding
> - *   nla_padlen(payload)		length of padding
>    *
>    * Attribute Payload Access:
>    *   nla_data(nla)			head of attribute payload
> @@ -492,6 +491,14 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
>    */
>   static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
>   {
> +	/* If an exact size if specified, reserve some additional space to
> +	 * align the first attribute, all subsequent attributes should have
> +	 * padding accounted for.
> +	 */
> +	if (payload != NLMSG_DEFAULT_SIZE)
> +		payload = min_t(size_t, payload + NLA_ATTR_ALIGN,
> +				NLMSG_DEFAULT_SIZE);
> +
>   	return alloc_skb(nlmsg_total_size(payload), flags);
>   }
>
> @@ -653,16 +660,12 @@ static inline int nla_attr_size(int payload)
>    */
>   static inline int nla_total_size(int payload)
>   {
> -	return NLA_ALIGN(nla_attr_size(payload));
> -}
> +	size_t len = NLA_ALIGN(nla_attr_size(payload));
>
> -/**
> - * nla_padlen - length of padding at the tail of attribute
> - * @payload: length of payload
> - */
> -static inline int nla_padlen(int payload)
> -{
> -	return nla_total_size(payload) - nla_attr_size(payload);
> +	if (!IS_ALIGNED(len, NLA_ATTR_ALIGN))
> +		len = ALIGN(len + NLA_HDRLEN, NLA_ATTR_ALIGN);
> +
> +	return len;
>   }
>
>   /**
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 78d5b8a..1856729 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -149,5 +149,10 @@ struct nlattr {
>   #define NLA_ALIGN(len)		(((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
>   #define NLA_HDRLEN		((int) NLA_ALIGN(sizeof(struct nlattr)))
>
> +/* Padding attribute type, added automatically to align attributes,
> + * must be ignored by readers. */
> +#define NLA_PADDING		0
> +#define NLA_ATTR_ALIGN		8
> +
>
>   #endif /* _UAPI__LINUX_NETLINK_H */
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 18eca78..b09473c 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -322,18 +322,36 @@ int nla_strcmp(const struct nlattr *nla, const char *str)
>    * Adds a netlink attribute header to a socket buffer and reserves
>    * room for the payload but does not copy it.
>    *
> + * May add a padding attribute of type NLA_PADDING before the
> + * real attribute to ensure proper alignment.
> + *
>    * The caller is responsible to ensure that the skb provides enough
>    * tailroom for the attribute header and payload.
>    */
>   struct nlattr *__nla_reserve(struct sk_buff *skb, int attrtype, int attrlen)
>   {
>   	struct nlattr *nla;
> +	size_t offset;
> +
> +	offset = (size_t) skb_tail_pointer(skb);
> +	if (!IS_ALIGNED(offset + NLA_HDRLEN, NLA_ATTR_ALIGN)) {
> +		struct nlattr *pad;
> +		size_t padlen;
> +
> +		padlen = nla_total_size(offset) - offset - NLA_HDRLEN;
Here padlen will return 4, which is wrong: padlen + NLA_HDRLEN = 8, alignment is 
the same than before. Here is a proposal fix:

diff --git a/lib/nlattr.c b/lib/nlattr.c
index e4f0329..1556313 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -338,7 +338,10 @@ struct nlattr *__nla_reserve(struct sk_buff *skb, int 
attrtype, int attrlen)
  		struct nlattr *pad;
  		size_t padlen;

-		padlen = nla_total_size(offset) - offset -  NLA_HDRLEN;
+		/* We need to remove NLA_HDRLEN two times: one time for the
+		 * attribute hdr and one time for the pad attribute hdr.
+		 */
+		padlen = nla_total_size(offset) - offset -  2 * NLA_HDRLEN;
  		pad = (struct nlattr *) skb_put(skb, nla_attr_size(padlen));
  		pad->nla_type = 0;
  		pad->nla_len = nla_attr_size(padlen);

With this patch, it seems goods. attribute are always aligned on 8 bytes. Also
I did not notice any problem with size calculation (I try some ip link, ip xfrm, 
ip [m]route).

Do you want to make more tests? Or will your repost the full patch?
I can do it if you don't have time.

Re: [PATCH v2] netlink: align attributes on 64-bits

[Thomas Graf]

On 12/19/12 at 12:22pm, Nicolas Dichtel wrote:
> Here padlen will return 4, which is wrong: padlen + NLA_HDRLEN = 8,
> alignment is the same than before. Here is a proposal fix:
> 
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index e4f0329..1556313 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -338,7 +338,10 @@ struct nlattr *__nla_reserve(struct sk_buff
> *skb, int attrtype, int attrlen)
>  		struct nlattr *pad;
>  		size_t padlen;
> 
> -		padlen = nla_total_size(offset) - offset -  NLA_HDRLEN;
> +		/* We need to remove NLA_HDRLEN two times: one time for the
> +		 * attribute hdr and one time for the pad attribute hdr.
> +		 */
> +		padlen = nla_total_size(offset) - offset -  2 * NLA_HDRLEN;
>  		pad = (struct nlattr *) skb_put(skb, nla_attr_size(padlen));
>  		pad->nla_type = 0;
>  		pad->nla_len = nla_attr_size(padlen);
> 
> With this patch, it seems goods. attribute are always aligned on 8 bytes. Also
> I did not notice any problem with size calculation (I try some ip
> link, ip xfrm, ip [m]route).
> 
> Do you want to make more tests? Or will your repost the full patch?
> I can do it if you don't have time.

Thanks.

I would like to do some testing as well. I do expect some fallout from
this. There is likely some interface abuse that will now be exposed
due to this.

We'll have to wait for the next merge window to open anyway. I'd
consider this a new feature and not a bugfix based on the possible
regression impact it could have.

I'll post a new version of the patch integrating your fix above so
others (especially subsystem maintainers depending on netlink) can run
the patch as well.

Re: [PATCH v2] netlink: align attributes on 64-bits

[Thomas Graf]

On 12/19/12 at 09:17am, David Laight wrote:
> You can't use memcpy() to copy a pointer to a misaligned
> structure into an aligned buffer. The compiler assumes
> the pointer is aligned and will use instructions that
> depend on the alignment.

I am not sure I understand this correctly. Are you saying
that the following does not work on i386?

struct foo {
  uint32_t a;
  uint64_t b;
};

struct foo buf;

memcpy(&buf, nla_data(attr), nla_len(attr));
printf([...], buf.b);


> I think:
> 1) Alignment is only needed on systems that have 'strict alignment'
>    requirements (maybe disable for testing?)

Right, what about mixed 32bit/64bit environments?

> 2) Alignment is only needed for parameters whose size is a
>    multiple of the alignment (a structure containing a
>    field that needs 8 byte alignment will always be a multiple
>    of 8 bytes long).

Good point. I'll fix this in the next iteration of the patch.

> 3) You need to add NA_HDR_LEN to the write pointer before
>    determining the size of the pad.

Right, I'm doing this in the patch I proposed. Or are you referring
to something else?

Re: [PATCH v2] netlink: align attributes on 64-bits

[Nicolas Dichtel]

Le 19/12/2012 18:09, Thomas Graf a écrit :
> On 12/19/12 at 12:22pm, Nicolas Dichtel wrote:
>> Here padlen will return 4, which is wrong: padlen + NLA_HDRLEN = 8,
>> alignment is the same than before. Here is a proposal fix:
>>
>> diff --git a/lib/nlattr.c b/lib/nlattr.c
>> index e4f0329..1556313 100644
>> --- a/lib/nlattr.c
>> +++ b/lib/nlattr.c
>> @@ -338,7 +338,10 @@ struct nlattr *__nla_reserve(struct sk_buff
>> *skb, int attrtype, int attrlen)
>>   		struct nlattr *pad;
>>   		size_t padlen;
>>
>> -		padlen = nla_total_size(offset) - offset -  NLA_HDRLEN;
>> +		/* We need to remove NLA_HDRLEN two times: one time for the
>> +		 * attribute hdr and one time for the pad attribute hdr.
>> +		 */
>> +		padlen = nla_total_size(offset) - offset -  2 * NLA_HDRLEN;
>>   		pad = (struct nlattr *) skb_put(skb, nla_attr_size(padlen));
>>   		pad->nla_type = 0;
>>   		pad->nla_len = nla_attr_size(padlen);
>>
>> With this patch, it seems goods. attribute are always aligned on 8 bytes. Also
>> I did not notice any problem with size calculation (I try some ip
>> link, ip xfrm, ip [m]route).
>>
>> Do you want to make more tests? Or will your repost the full patch?
>> I can do it if you don't have time.
>
> Thanks.
>
> I would like to do some testing as well. I do expect some fallout from
> this. There is likely some interface abuse that will now be exposed
> due to this.
>
> We'll have to wait for the next merge window to open anyway. I'd
> consider this a new feature and not a bugfix based on the possible
> regression impact it could have.
>
> I'll post a new version of the patch integrating your fix above so
> others (especially subsystem maintainers depending on netlink) can run
> the patch as well.
>
Ok, sounds good.

RE: [PATCH v2] netlink: align attributes on 64-bits

[David Laight]

> On 12/19/12 at 09:17am, David Laight wrote:
> > You can't use memcpy() to copy a pointer to a misaligned
> > structure into an aligned buffer. The compiler assumes
> > the pointer is aligned and will use instructions that
> > depend on the alignment.
> 
> I am not sure I understand this correctly. Are you saying
> that the following does not work on i386?
> 
> struct foo {
>   uint32_t a;
>   uint64_t b;
> };
> 
> struct foo buf;
> 
> memcpy(&buf, nla_data(attr), nla_len(attr));
> printf([...], buf.b);

That will be fine on all systems.
But if, instead, you have:
	struct foo buf, *bufp;
	bufp = nla_data(attr);
	memcpy(&buf, bufp, sizeof buf);
The compiler is allowed to assume that 'bufp' is aligned,
so the copy will be done using 64bit accesses.

(Basically because all you are allowed to do with 'void *'
is cast a point to 'void *', then back to its original type.
So when you cast back from 'void *' the pointer can be assumed
to be aligned.)

This will fault on systems that require strict alignment
of 64bit items.

	David

RE: [PATCH v2] netlink: align attributes on 64-bits

[David Laight]

> > I think:
> > 1) Alignment is only needed on systems that have 'strict alignment'
> >    requirements (maybe disable for testing?)
> 
> Right, what about mixed 32bit/64bit environments?

Support for i386 user binaries on amd64 kernels
is an entirely different problem!
That, typically, requires the kernel to know that
the application is 32bit and use separate structures
where the 64bit items have the aligned(32) attribute.

	David