xfrm: add x86 CONFIG_COMPAT support

xfrm: add x86 CONFIG_COMPAT support

[Florian Westphal]

At the moment it is not possible to use the xfrm netlink interface on
x86_64 with a 32bit userland.

The problem exists because a few structures, e.g. struct xfrm_usersa_info,
have different sizes in user/kernelspace (3 byte padding on x86, 7
byte on x86_64) due to different alignment requirements of "u64".

The following patch set aims to resolve this.

The first two patches add necessary CONFIG_COMPAT_NETLINK_MESSAGES
infrastructure to the netlink in/output path.

Patch 3 is a refactoring patch to split functionality (especially
nlmsg allocation and adding data to the nlmsg) in order to
re-use code and ease review.

Patch 4 adds CONFIG_COMPAT_FOR_U64_ALIGNMENT support to xfrm.

Patch 5 sets CMSG_MSG_COMPAT in sock_aio_write(), depending on
is_compat_task().  If patch 5 is deemed too ugly, just pretend it
doesn't exist; userspace can be patched to use sendmsg instead of
write() to avoid this problem.

 include/linux/netlink.h  |    1
 net/netlink/af_netlink.c |   11
 net/socket.c             |    4
 net/xfrm/Kconfig         |    1
 net/xfrm/xfrm_user.c     |  524 ++++++++++++++++++++++++++++++++++++++++++-----
 5 files changed, 488 insertions(+), 53 deletions(-)

[PATCH 1/5] netlink: append NLMSG_DONE to compatskb, too

[Florian Westphal]

From: Florian Westphal <fwestphal@astaro.com>

modules using netlink may supply a 2nd skb, (via frag_list)
that contains an alternative data set meant for applications
using 32bit compatibility mode.

In such a case, netlink_recvmsg will use this 2nd skb instead of the
original one.

Without this patch, such compat applications will retrieve
all netlink dump data, but will then get an unexpected EOF.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netlink/af_netlink.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4c5972b..4910031 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1698,6 +1698,14 @@ static int netlink_dump(struct sock *sk)
 
 	memcpy(nlmsg_data(nlh), &len, sizeof(len));
 
+#ifdef CONFIG_COMPAT_NETLINK_MESSAGES
+	if (unlikely(skb_shinfo(skb)->frag_list)) {
+		nlh = nlmsg_put_answer(skb_shinfo(skb)->frag_list, cb,
+					NLMSG_DONE, sizeof(len), NLM_F_MULTI);
+		if (nlh)
+			memcpy(nlmsg_data(nlh), &len, sizeof(len));
+	}
+#endif
 	if (sk_filter(sk, skb))
 		kfree_skb(skb);
 	else {
-- 
1.6.3.3

[PATCH 2/5] netlink: store MSG_CMSG_COMPAT flag in netlink_skb_parms

[Florian Westphal]

This allows the netlink processing context to determine if the data
needs any 32 bit fixups.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netlink.h  |    1 +
 net/netlink/af_netlink.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index fde27c0..8b30c01 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -164,6 +164,7 @@ struct netlink_skb_parms {
 	__u32			loginuid;	/* Login (audit) uid */
 	__u32			sessionid;	/* Session id (audit) */
 	__u32			sid;		/* SELinux security id */
+	__u8			msg_compat;	/* Message needs 32bit fixups */
 };
 
 #define NETLINK_CB(skb)		(*(struct netlink_skb_parms*)&((skb)->cb))
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4910031..5ff97cf 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1328,6 +1328,9 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
 	NETLINK_CB(skb).dst_group = dst_group;
 	NETLINK_CB(skb).loginuid = audit_get_loginuid(current);
 	NETLINK_CB(skb).sessionid = audit_get_sessionid(current);
+#ifdef CONFIG_COMPAT_NETLINK_MESSAGES
+	NETLINK_CB(skb).msg_compat = !!(msg->msg_flags & MSG_CMSG_COMPAT);
+#endif
 	security_task_getsecid(current, &(NETLINK_CB(skb).sid));
 	memcpy(NETLINK_CREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
 
-- 
1.6.3.3

Re: [PATCH 2/5] netlink: store MSG_CMSG_COMPAT flag in netlink_skb_parms

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

On Mon, 2010-02-15 at 17:46 +0100, Florian Westphal wrote:
> This allows the netlink processing context to determine if the data
> needs any 32 bit fixups.

> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -164,6 +164,7 @@ struct netlink_skb_parms {
>  	__u32			loginuid;	/* Login (audit) uid */
>  	__u32			sessionid;	/* Session id (audit) */
>  	__u32			sid;		/* SELinux security id */
> +	__u8			msg_compat;	/* Message needs 32bit fixups */

bool?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 3/5] xfrm: split nlmsg allocation and data copying

[Florian Westphal]

To support 32bit userland with different u64 alignment requirements
than a 64bit kernel (COMPAT_FOR_U64_ALIGNMENT), it is
necessary to prepare messages containing affected structures
twice: once in the format expected by 64bit listeners, one
in the format expected by 32bit applications.

In order to minimize copy & pasting and re-use existing
code where possible, split nlmsg allocation and data copying.

Also, replace foo(..., sizeof(*structure)) with

len = sizeof(*structure);
foo(..., len);

so len can be made conditional if we are preparing a compat message.
This will be done in a followup-patch.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/xfrm/xfrm_user.c |  156 ++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 114 insertions(+), 42 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 943c871..bda5c15 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -692,17 +692,16 @@ nla_put_failure:
 	return -EMSGSIZE;
 }
 
-static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
+static int copy_one_state(struct sk_buff *skb, struct xfrm_state *x, struct xfrm_dump_info *sp)
 {
-	struct xfrm_dump_info *sp = ptr;
 	struct sk_buff *in_skb = sp->in_skb;
-	struct sk_buff *skb = sp->out_skb;
 	struct xfrm_usersa_info *p;
 	struct nlmsghdr *nlh;
+	size_t len = sizeof(*p);
 	int err;
 
 	nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
-			XFRM_MSG_NEWSA, sizeof(*p), sp->nlmsg_flags);
+			XFRM_MSG_NEWSA, len, sp->nlmsg_flags);
 	if (nlh == NULL)
 		return -EMSGSIZE;
 
@@ -720,6 +719,14 @@ nla_put_failure:
 	return err;
 }
 
+static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
+{
+	struct xfrm_dump_info *sp = ptr;
+	struct sk_buff *skb = sp->out_skb;
+	int ret = copy_one_state(skb, x, sp);
+	return ret;
+}
+
 static int xfrm_dump_sa_done(struct netlink_callback *cb)
 {
 	struct xfrm_state_walk *walk = (struct xfrm_state_walk *) &cb->args[1];
@@ -1345,16 +1352,15 @@ static inline int copy_to_user_policy_type(u8 type, struct sk_buff *skb)
 }
 #endif
 
-static int dump_one_policy(struct xfrm_policy *xp, int dir, int count, void *ptr)
+static int copy_one_policy(struct sk_buff *skb, struct xfrm_policy *xp, int dir, struct xfrm_dump_info *sp)
 {
-	struct xfrm_dump_info *sp = ptr;
 	struct xfrm_userpolicy_info *p;
 	struct sk_buff *in_skb = sp->in_skb;
-	struct sk_buff *skb = sp->out_skb;
 	struct nlmsghdr *nlh;
+	size_t len = sizeof(*p);
 
 	nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
-			XFRM_MSG_NEWPOLICY, sizeof(*p), sp->nlmsg_flags);
+			XFRM_MSG_NEWPOLICY, len, sp->nlmsg_flags);
 	if (nlh == NULL)
 		return -EMSGSIZE;
 
@@ -1375,6 +1381,14 @@ nlmsg_failure:
 	return -EMSGSIZE;
 }
 
+static int dump_one_policy(struct xfrm_policy *xp, int dir, int count, void *ptr)
+{
+	struct xfrm_dump_info *sp = ptr;
+	struct sk_buff *skb = sp->out_skb;
+	int ret = copy_one_policy(skb, xp, dir, sp);
+	return ret;
+}
+
 static int xfrm_dump_policy_done(struct netlink_callback *cb)
 {
 	struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1];
@@ -1695,7 +1709,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct xfrm_user_polexpire *up = nlmsg_data(nlh);
 	struct xfrm_userpolicy_info *p = &up->pol;
 	u8 type = XFRM_POLICY_TYPE_MAIN;
-	int err = -ENOENT;
+	int hard, err = -ENOENT;
 
 	err = copy_from_user_policy_type(&type, attrs);
 	if (err)
@@ -1733,7 +1747,8 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	read_unlock(&xp->lock);
 	err = 0;
-	if (up->hard) {
+	hard = up->hard;
+	if (hard) {
 		uid_t loginuid = NETLINK_CB(skb).loginuid;
 		uid_t sessionid = NETLINK_CB(skb).sessionid;
 		u32 sid = NETLINK_CB(skb).sid;
@@ -1744,7 +1759,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 		// reset the timers here?
 		printk("Dont know what to do with soft policy expire\n");
 	}
-	km_policy_expired(xp, p->dir, up->hard, current->pid);
+	km_policy_expired(xp, p->dir, hard, current->pid);
 
 out:
 	xfrm_pol_put(xp);
@@ -1756,7 +1771,7 @@ static int xfrm_add_sa_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 {
 	struct net *net = sock_net(skb->sk);
 	struct xfrm_state *x;
-	int err;
+	int hard, err;
 	struct xfrm_user_expire *ue = nlmsg_data(nlh);
 	struct xfrm_usersa_info *p = &ue->state;
 
@@ -1770,9 +1785,10 @@ static int xfrm_add_sa_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = -EINVAL;
 	if (x->km.state != XFRM_STATE_VALID)
 		goto out;
-	km_state_expired(x, ue->hard, current->pid);
+	hard = ue->hard;
+	km_state_expired(x, hard, current->pid);
 
-	if (ue->hard) {
+	if (hard) {
 		uid_t loginuid = NETLINK_CB(skb).loginuid;
 		uid_t sessionid = NETLINK_CB(skb).sessionid;
 		u32 sid = NETLINK_CB(skb).sid;
@@ -2256,26 +2272,35 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x)
 	return l;
 }
 
-static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
+static int xfrm_notify_sa_len(struct xfrm_state *x, const struct km_event *c)
 {
-	struct net *net = xs_net(x);
-	struct xfrm_usersa_info *p;
-	struct xfrm_usersa_id *id;
-	struct nlmsghdr *nlh;
-	struct sk_buff *skb;
 	int len = xfrm_sa_len(x);
-	int headlen;
+	int headlen = sizeof(struct xfrm_usersa_info);
 
-	headlen = sizeof(*p);
 	if (c->event == XFRM_MSG_DELSA) {
 		len += nla_total_size(headlen);
-		headlen = sizeof(*id);
+		headlen = sizeof(struct xfrm_usersa_id);
 	}
 	len += NLMSG_ALIGN(headlen);
 
-	skb = nlmsg_new(len, GFP_ATOMIC);
-	if (skb == NULL)
-		return -ENOMEM;
+	return len;
+}
+
+static int xfrm_notify_sa_headlen(const struct km_event *c)
+{
+	if (c->event == XFRM_MSG_DELSA)
+		return sizeof(struct xfrm_usersa_id);
+	return sizeof(struct xfrm_usersa_info);
+}
+
+static int copy_to_user_xfrm_notify_sa(struct sk_buff *skb,
+                                       struct xfrm_state *x, struct km_event *c)
+{
+	struct xfrm_usersa_info *p;
+	struct xfrm_usersa_id *id;
+	struct nlmsghdr *nlh;
+	int sizeof_xfrm_usersa_info = sizeof(*p);
+	int headlen = xfrm_notify_sa_headlen(c);
 
 	nlh = nlmsg_put(skb, c->pid, c->seq, c->event, headlen, 0);
 	if (nlh == NULL)
@@ -2291,7 +2316,7 @@ static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
 		id->family = x->props.family;
 		id->proto = x->id.proto;
 
-		attr = nla_reserve(skb, XFRMA_SA, sizeof(*p));
+		attr = nla_reserve(skb, XFRMA_SA, sizeof_xfrm_usersa_info);
 		if (attr == NULL)
 			goto nla_put_failure;
 
@@ -2302,6 +2327,25 @@ static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
 		goto nla_put_failure;
 
 	nlmsg_end(skb, nlh);
+	return 0;
+nla_put_failure:
+	/* Somebody screwed up with xfrm_sa_len! */
+	WARN_ON(1);
+	return -1;
+}
+
+static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
+{
+	struct sk_buff *skb;
+	struct net *net = xs_net(x);
+	int len = xfrm_notify_sa_len(x, c);
+
+	skb = nlmsg_new(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+
+	if (copy_to_user_xfrm_notify_sa(skb, x, c))
+		goto nla_put_failure;
 
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_SA, GFP_ATOMIC);
 
@@ -2349,10 +2393,11 @@ static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 			 int dir)
 {
 	struct xfrm_user_acquire *ua;
+	size_t len = sizeof(*ua);
 	struct nlmsghdr *nlh;
 	__u32 seq = xfrm_get_acqseq();
 
-	nlh = nlmsg_put(skb, 0, 0, XFRM_MSG_ACQUIRE, sizeof(*ua), 0);
+	nlh = nlmsg_put(skb, 0, 0, XFRM_MSG_ACQUIRE, len, 0);
 	if (nlh == NULL)
 		return -EMSGSIZE;
 
@@ -2468,10 +2513,11 @@ static int build_polexpire(struct sk_buff *skb, struct xfrm_policy *xp,
 			   int dir, struct km_event *c)
 {
 	struct xfrm_user_polexpire *upe;
+	size_t len = sizeof(*upe);
 	struct nlmsghdr *nlh;
 	int hard = c->data.hard;
 
-	nlh = nlmsg_put(skb, c->pid, 0, XFRM_MSG_POLEXPIRE, sizeof(*upe), 0);
+	nlh = nlmsg_put(skb, c->pid, 0, XFRM_MSG_POLEXPIRE, len, 0);
 	if (nlh == NULL)
 		return -EMSGSIZE;
 
@@ -2507,27 +2553,35 @@ static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, struct km_eve
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_EXPIRE, GFP_ATOMIC);
 }
 
-static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *c)
+static int xfrm_notify_policy_len(struct xfrm_policy *xp, struct km_event *c)
 {
-	struct net *net = xp_net(xp);
-	struct xfrm_userpolicy_info *p;
-	struct xfrm_userpolicy_id *id;
-	struct nlmsghdr *nlh;
-	struct sk_buff *skb;
 	int len = nla_total_size(sizeof(struct xfrm_user_tmpl) * xp->xfrm_nr);
-	int headlen;
+	int headlen = sizeof(struct xfrm_userpolicy_info);
 
-	headlen = sizeof(*p);
 	if (c->event == XFRM_MSG_DELPOLICY) {
 		len += nla_total_size(headlen);
-		headlen = sizeof(*id);
+		headlen = sizeof(struct xfrm_userpolicy_id);
 	}
 	len += userpolicy_type_attrsize();
 	len += NLMSG_ALIGN(headlen);
+	return len;
+}
 
-	skb = nlmsg_new(len, GFP_ATOMIC);
-	if (skb == NULL)
-		return -ENOMEM;
+static int xfrm_notify_policy_headlen(const struct km_event *c)
+{
+	if (c->event == XFRM_MSG_DELPOLICY)
+		return sizeof(struct xfrm_userpolicy_id);
+	return sizeof(struct xfrm_userpolicy_info);
+}
+
+static int copy_to_user_xfrm_notify_policy(struct sk_buff *skb, int dir,
+                                           struct xfrm_policy *xp, struct km_event *c)
+{
+	struct xfrm_userpolicy_info *p;
+	struct xfrm_userpolicy_id *id;
+	struct nlmsghdr *nlh;
+	int sizeof_xfrm_userpolicy_info = sizeof(*p);
+	int headlen = xfrm_notify_policy_headlen(c);
 
 	nlh = nlmsg_put(skb, c->pid, c->seq, c->event, headlen, 0);
 	if (nlh == NULL)
@@ -2545,7 +2599,7 @@ static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *
 		else
 			memcpy(&id->sel, &xp->selector, sizeof(id->sel));
 
-		attr = nla_reserve(skb, XFRMA_POLICY, sizeof(*p));
+		attr = nla_reserve(skb, XFRMA_POLICY, sizeof_xfrm_userpolicy_info);
 		if (attr == NULL)
 			goto nlmsg_failure;
 
@@ -2560,6 +2614,24 @@ static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *
 
 	nlmsg_end(skb, nlh);
 
+	return 0;
+
+nlmsg_failure:
+	return -1;
+}
+
+static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+	struct net *net = xp_net(xp);
+	struct sk_buff *skb;
+	int len = xfrm_notify_policy_len(xp, c);
+
+	skb = nlmsg_new(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	if (copy_to_user_xfrm_notify_policy(skb, dir, xp, c))
+		goto nlmsg_failure;
+
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_POLICY, GFP_ATOMIC);
 
 nlmsg_failure:
-- 
1.6.3.3

[PATCH 4/5] xfrm: CONFIG_COMPAT support for x86 architecture

[Florian Westphal]

on x86_64, struct xfrm_userpolicy_info/usersa_info have four
additional bytes of padding at the end compared to x86.

Thus, when calling nlmsg_parse(.., sizeof(struct xfrm_userpolicy_info),
trailing attributes are not parsed correctly by the kernel when
the message was sent from an x86 32bit task.

Furthermore, those structures are contained inside
a few other structures, e.g. struct xfrm_user_acquire.

When dealing with incoming data from userland,
those structures need special treatment in the "userland is 32bit"
case.

When sending data to userland, it is sent in both 32bit and native
format.

Errors when building the compat message are not visisble to user
space; data will then be sent without the compat payload.

refer to 1dacc76d0014a034b8aca14237c127d7c19d7726
(net/compat/wext: send different messages to compat tasks) for
more information on netlink compat handling.

Signed-off-by: Florian Westphal <fwestphal@astaro.com>
---
 net/xfrm/Kconfig     |    1 +
 net/xfrm/xfrm_user.c |  372 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 360 insertions(+), 13 deletions(-)

diff --git a/net/xfrm/Kconfig b/net/xfrm/Kconfig
index 6d08167..0b30357 100644
--- a/net/xfrm/Kconfig
+++ b/net/xfrm/Kconfig
@@ -9,6 +9,7 @@ config XFRM
 config XFRM_USER
 	tristate "Transformation user configuration interface"
 	depends on INET && XFRM
+	select WANT_COMPAT_NETLINK_MESSAGES if COMPAT_FOR_U64_ALIGNMENT
 	---help---
 	  Support for Transformation(XFRM) user configuration interface
 	  like IPsec used by native Linux tools.
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index bda5c15..5794fa3 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -10,6 +10,7 @@
  *
  */
 
+#include <linux/compat.h>
 #include <linux/crypto.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
@@ -31,6 +32,161 @@
 #include <linux/in6.h>
 #endif
 
+#ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT
+struct compat_xfrm_lifetime_cfg {
+	compat_u64 soft_byte_limit, hard_byte_limit;
+	compat_u64 soft_packet_limit, hard_packet_limit;
+	compat_u64 soft_add_expires_seconds, hard_add_expires_seconds;
+	compat_u64 soft_use_expires_seconds, hard_use_expires_seconds;
+};
+
+struct compat_xfrm_lifetime_cur {
+	compat_u64 bytes, packets, add_time, use_time;
+};
+
+struct compat_xfrm_userpolicy_info {
+	struct xfrm_selector sel;
+	struct compat_xfrm_lifetime_cfg lft;
+	struct compat_xfrm_lifetime_cur curlft;
+	u32 priority, index;
+	u8 dir, action, flags, share;
+	/* 4 bytes additional padding on 64bit */
+};
+
+struct compat_xfrm_usersa_info {
+	struct xfrm_selector sel;
+	struct xfrm_id id;
+	xfrm_address_t saddr;
+	struct compat_xfrm_lifetime_cfg lft;
+	struct compat_xfrm_lifetime_cur curlft;
+	struct xfrm_stats stats;
+	u32 seq, reqid;
+	u16 family;
+	u8 mode, replay_window, flags;
+	/* 4 bytes additional padding on 64bit */
+};
+
+struct compat_xfrm_user_acquire {
+	struct xfrm_id id;
+	xfrm_address_t saddr;
+	struct xfrm_selector sel;
+	struct compat_xfrm_userpolicy_info policy;
+	/* 4 bytes additional padding on 64bit */
+	u32 aalgos, ealgos, calgos, seq;
+};
+
+struct compat_xfrm_userspi_info {
+	struct compat_xfrm_usersa_info info;
+	/* 4 bytes additional padding on 64bit */
+	u32 min, max;
+};
+
+struct compat_xfrm_user_expire {
+	struct compat_xfrm_usersa_info state;
+	/* 4 bytes additional padding on 64bit */
+	u8 hard;
+};
+
+struct compat_xfrm_user_polexpire {
+	struct compat_xfrm_userpolicy_info pol;
+	/* 4 bytes additional padding on 64bit */
+	u8 hard;
+};
+
+static bool xfrm_msg_compat(const struct sk_buff *skb)
+{
+	return unlikely(!!NETLINK_CB(skb).msg_compat);
+}
+
+static bool xfrm_add_compatskb(struct sk_buff *skb,
+                               unsigned int len, gfp_t gfp)
+{
+	struct sk_buff *compatskb = nlmsg_new(len, gfp);
+
+	WARN_ON(skb_shinfo(skb)->frag_list);
+
+	if (compatskb)
+		NETLINK_CB(compatskb).msg_compat = 1;
+
+	skb_shinfo(skb)->frag_list = compatskb;
+
+	return compatskb != NULL;
+}
+
+static struct sk_buff *xfrm_get_compatskb(struct sk_buff *skb)
+{
+	return skb_shinfo(skb)->frag_list;
+}
+#else
+static inline bool xfrm_msg_compat(const struct sk_buff *skb)
+{
+	return false;
+}
+
+static inline bool xfrm_add_compatskb(struct sk_buff *skb,
+                                      unsigned int len, gfp_t gfp)
+{
+	return false;
+}
+static inline void compat_xfrm_notify_sa(struct sk_buff *skb,
+                                         struct xfrm_state *x, struct km_event *c)
+{
+}
+
+/*
+ * avoids #ifdefs all over the place. Use of these must be conditional via
+ * xfrm_add_compatskb/xfrm_get_compatskb so compiler can remove branches.
+ */
+#define compat_xfrm_user_expire     xfrm_user_expire
+#define compat_xfrm_user_acquire    xfrm_user_acquire
+#define compat_xfrm_user_polexpire  xfrm_user_polexpire
+#define compat_xfrm_userpolicy_info xfrm_userpolicy_info
+#define compat_xfrm_usersa_info     xfrm_usersa_info
+#define compat_xfrm_userspi_info    xfrm_userspi_info
+
+#endif
+
+/*
+ * userspace size of some structures is smaller by this amount
+ * due to different u64 alignment on x86 platform.
+ *
+ * Some of the structures need to use the compat_* structure definition
+ * when accessing certain members.
+ */
+static const u8 xfrm_msg_min_compat_pad[XFRM_NR_MSGTYPES] = {
+	[XFRM_MSG_NEWSA       - XFRM_MSG_BASE] = 4, /* padding at end */
+	[XFRM_MSG_NEWPOLICY   - XFRM_MSG_BASE] = 4, /* padding at end */
+
+	[XFRM_MSG_ALLOCSPI    - XFRM_MSG_BASE] = 4, /* access might need compat_ */
+	[XFRM_MSG_ACQUIRE     - XFRM_MSG_BASE] = 4, /* access might need compat_ */
+	[XFRM_MSG_EXPIRE      - XFRM_MSG_BASE] = 4, /* access might need compat_ */
+	[XFRM_MSG_UPDPOLICY   - XFRM_MSG_BASE] = 4, /* padding at end */
+	[XFRM_MSG_UPDSA       - XFRM_MSG_BASE] = 4, /* padding at end */
+	[XFRM_MSG_POLEXPIRE   - XFRM_MSG_BASE] = 4, /* access might need compat_ */
+};
+
+static int get_user_expire_hard(const struct sk_buff *skb,
+                                const struct xfrm_user_expire *ue)
+{
+	if (xfrm_msg_compat(skb)) {
+		const struct compat_xfrm_user_expire *cmpt;
+		cmpt = (const struct compat_xfrm_user_expire*) ue;
+		return cmpt->hard;
+	}
+	return ue->hard;
+}
+
+static int get_user_polexpire_hard(const struct sk_buff *skb,
+                                   const struct xfrm_user_polexpire *ue)
+{
+	if (xfrm_msg_compat(skb)) {
+		const struct compat_xfrm_user_polexpire *cmpt;
+		cmpt = (const struct compat_xfrm_user_polexpire*) ue;
+		return cmpt->hard;
+	}
+	return ue->hard;
+}
+
 static inline int aead_len(struct xfrm_algo_aead *alg)
 {
 	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);
@@ -700,6 +856,9 @@ static int copy_one_state(struct sk_buff *skb, struct xfrm_state *x, struct xfrm
 	size_t len = sizeof(*p);
 	int err;
 
+	if (xfrm_msg_compat(skb))
+		len = sizeof(struct compat_xfrm_usersa_info);
+
 	nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
 			XFRM_MSG_NEWSA, len, sp->nlmsg_flags);
 	if (nlh == NULL)
@@ -724,6 +883,13 @@ static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
 	struct xfrm_dump_info *sp = ptr;
 	struct sk_buff *skb = sp->out_skb;
 	int ret = copy_one_state(skb, x, sp);
+#ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT
+	if (ret == 0) {
+		skb = xfrm_get_compatskb(skb);
+		if (skb)
+			copy_one_state(skb, x, sp);
+	}
+#endif
 	return ret;
 }
 
@@ -753,6 +919,8 @@ static int xfrm_dump_sa(struct sk_buff *skb, struct netlink_callback *cb)
 		xfrm_state_walk_init(walk, 0);
 	}
 
+	xfrm_add_compatskb(skb, NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+
 	(void) xfrm_state_walk(net, walk, dump_one_state, &info);
 
 	return skb->len;
@@ -944,6 +1112,29 @@ static int verify_userspi_info(struct xfrm_userspi_info *p)
 	return 0;
 }
 
+static int compat_verify_userspi_info(struct xfrm_userspi_info *p __maybe_unused)
+{
+	struct compat_xfrm_userspi_info *compat;
+
+	compat = (struct compat_xfrm_userspi_info  *) p;
+
+	switch (p->info.id.proto) {
+	case IPPROTO_AH:
+	case IPPROTO_ESP:
+		break;
+	case IPPROTO_COMP:
+		if (compat->max >= 0x10000)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (compat->min > compat->max)
+		return -EINVAL;
+	return 0;
+}
+
 static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 		struct nlattr **attrs)
 {
@@ -956,7 +1147,10 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 	int err;
 
 	p = nlmsg_data(nlh);
-	err = verify_userspi_info(p);
+	if (xfrm_msg_compat(skb))
+		err = compat_verify_userspi_info(p);
+	else
+		err = verify_userspi_info(p);
 	if (err)
 		goto out_noput;
 
@@ -980,8 +1174,14 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = -ENOENT;
 	if (x == NULL)
 		goto out_noput;
+	if (xfrm_msg_compat(skb)) {
+		struct compat_xfrm_userspi_info *compat;
+		compat = (struct compat_xfrm_userspi_info *) p;
+		err = xfrm_alloc_spi(x, compat->min, compat->max);
+	} else {
+		err = xfrm_alloc_spi(x, p->min, p->max);
+	}
 
-	err = xfrm_alloc_spi(x, p->min, p->max);
 	if (err)
 		goto out;
 
@@ -1359,6 +1559,9 @@ static int copy_one_policy(struct sk_buff *skb, struct xfrm_policy *xp, int dir,
 	struct nlmsghdr *nlh;
 	size_t len = sizeof(*p);
 
+	if (xfrm_msg_compat(skb))
+		len = sizeof(struct compat_xfrm_userpolicy_info);
+
 	nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
 			XFRM_MSG_NEWPOLICY, len, sp->nlmsg_flags);
 	if (nlh == NULL)
@@ -1386,6 +1589,13 @@ static int dump_one_policy(struct xfrm_policy *xp, int dir, int count, void *ptr
 	struct xfrm_dump_info *sp = ptr;
 	struct sk_buff *skb = sp->out_skb;
 	int ret = copy_one_policy(skb, xp, dir, sp);
+#ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT
+	if (ret == 0) {
+		skb = xfrm_get_compatskb(skb);
+		if (skb)
+			copy_one_policy(skb, xp, dir, sp);
+	}
+#endif
 	return ret;
 }
 
@@ -1416,6 +1626,8 @@ static int xfrm_dump_policy(struct sk_buff *skb, struct netlink_callback *cb)
 		xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY);
 	}
 
+	xfrm_add_compatskb(skb, NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+
 	(void) xfrm_policy_walk(net, walk, dump_one_policy, &info);
 
 	return skb->len;
@@ -1747,7 +1959,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	read_unlock(&xp->lock);
 	err = 0;
-	hard = up->hard;
+	hard = get_user_polexpire_hard(skb, up);
 	if (hard) {
 		uid_t loginuid = NETLINK_CB(skb).loginuid;
 		uid_t sessionid = NETLINK_CB(skb).sessionid;
@@ -1785,7 +1997,7 @@ static int xfrm_add_sa_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = -EINVAL;
 	if (x->km.state != XFRM_STATE_VALID)
 		goto out;
-	hard = ue->hard;
+	hard = get_user_expire_hard(skb, ue);
 	km_state_expired(x, hard, current->pid);
 
 	if (hard) {
@@ -1802,6 +2014,23 @@ out:
 	return err;
 }
 
+static void acquire_extract_algos(const struct sk_buff *skb,
+                                  struct xfrm_tmpl *t,
+                                  const struct xfrm_user_acquire *ua)
+{
+	if (!xfrm_msg_compat(skb)) {
+		t->aalgos = ua->aalgos;
+		t->ealgos = ua->ealgos;
+		t->calgos = ua->calgos;
+	} else {
+		const struct compat_xfrm_user_acquire *compat_ua;
+		compat_ua = (const struct compat_xfrm_user_acquire *) ua;
+		t->aalgos = compat_ua->aalgos;
+		t->ealgos = compat_ua->ealgos;
+		t->calgos = compat_ua->calgos;
+	}
+}
+
 static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 		struct nlattr **attrs)
 {
@@ -1839,9 +2068,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 		x->props.mode = t->mode;
 		x->props.reqid = t->reqid;
 		x->props.family = ut->family;
-		t->aalgos = ua->aalgos;
-		t->ealgos = ua->ealgos;
-		t->calgos = ua->calgos;
+		acquire_extract_algos(skb, t, ua);
 		err = km_query(x, t, xp);
 
 	}
@@ -2122,6 +2349,13 @@ static struct xfrm_link {
 	[XFRM_MSG_GETSPDINFO  - XFRM_MSG_BASE] = { .doit = xfrm_get_spdinfo   },
 };
 
+static int xfrm_msg_hdrlen(const struct sk_buff *skb, int type)
+{
+	if (xfrm_msg_compat(skb))
+		return xfrm_msg_min[type] - xfrm_msg_min_compat_pad[type];
+	return xfrm_msg_min[type];
+}
+
 static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
 	struct net *net = sock_net(skb->sk);
@@ -2149,7 +2383,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		return netlink_dump_start(net->xfrm.nlsk, skb, nlh, link->dump, link->done);
 	}
 
-	err = nlmsg_parse(nlh, xfrm_msg_min[type], attrs, XFRMA_MAX,
+	err = nlmsg_parse(nlh, xfrm_msg_hdrlen(skb, type), attrs, XFRMA_MAX,
 			  xfrma_policy);
 	if (err < 0)
 		return err;
@@ -2188,6 +2422,27 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, struct km_eve
 	return nlmsg_end(skb, nlh);
 }
 
+static inline size_t compat_xfrm_expire_msgsize(void)
+{
+	return NLMSG_ALIGN(sizeof(struct compat_xfrm_user_expire));
+}
+
+static void compat_build_expire(struct sk_buff *skb, struct xfrm_state *x, struct km_event *c)
+{
+	struct compat_xfrm_user_expire *compat_ue;
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put(skb, c->pid, 0, XFRM_MSG_EXPIRE, sizeof(*compat_ue), 0);
+	if (nlh == NULL)
+		return;
+
+	compat_ue = nlmsg_data(nlh);
+	copy_to_user_state(x, (struct xfrm_usersa_info *) &compat_ue->state);
+	compat_ue->hard = (c->data.hard != 0) ? 1 : 0;
+
+	nlmsg_end(skb, nlh);
+}
+
 static int xfrm_exp_state_notify(struct xfrm_state *x, struct km_event *c)
 {
 	struct net *net = xs_net(x);
@@ -2200,6 +2455,9 @@ static int xfrm_exp_state_notify(struct xfrm_state *x, struct km_event *c)
 	if (build_expire(skb, x, c) < 0)
 		BUG();
 
+	if (xfrm_add_compatskb(skb, compat_xfrm_expire_msgsize(), GFP_ATOMIC))
+		compat_build_expire(skb_shinfo(skb)->frag_list, x, c);
+
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_EXPIRE, GFP_ATOMIC);
 }
 
@@ -2293,6 +2551,13 @@ static int xfrm_notify_sa_headlen(const struct km_event *c)
 	return sizeof(struct xfrm_usersa_info);
 }
 
+static int compat_xfrm_notify_sa_headlen(const struct km_event *c)
+{
+	if (c->event == XFRM_MSG_DELSA)
+		return sizeof(struct xfrm_usersa_id);
+	return sizeof(struct compat_xfrm_usersa_info);
+}
+
 static int copy_to_user_xfrm_notify_sa(struct sk_buff *skb,
                                        struct xfrm_state *x, struct km_event *c)
 {
@@ -2302,6 +2567,11 @@ static int copy_to_user_xfrm_notify_sa(struct sk_buff *skb,
 	int sizeof_xfrm_usersa_info = sizeof(*p);
 	int headlen = xfrm_notify_sa_headlen(c);
 
+	if (xfrm_msg_compat(skb)) {
+		headlen = compat_xfrm_notify_sa_headlen(c);
+		sizeof_xfrm_usersa_info = sizeof(struct compat_xfrm_usersa_info);
+	}
+
 	nlh = nlmsg_put(skb, c->pid, c->seq, c->event, headlen, 0);
 	if (nlh == NULL)
 		goto nla_put_failure;
@@ -2347,6 +2617,9 @@ static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
 	if (copy_to_user_xfrm_notify_sa(skb, x, c))
 		goto nla_put_failure;
 
+	if (xfrm_add_compatskb(skb, len, GFP_ATOMIC))
+		copy_to_user_xfrm_notify_sa(skb_shinfo(skb)->frag_list, x, c);
+
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_SA, GFP_ATOMIC);
 
 nla_put_failure:
@@ -2388,6 +2661,32 @@ static inline size_t xfrm_acquire_msgsize(struct xfrm_state *x,
 	       + userpolicy_type_attrsize();
 }
 
+static inline size_t compat_xfrm_acquire_msgsize(struct xfrm_state *x,
+					  struct xfrm_policy *xp)
+{
+	return NLMSG_ALIGN(sizeof(struct compat_xfrm_user_acquire))
+	       + nla_total_size(sizeof(struct xfrm_user_tmpl) * xp->xfrm_nr)
+	       + nla_total_size(xfrm_user_sec_ctx_size(x->security))
+	       + userpolicy_type_attrsize();
+}
+
+static void acquire_copy_algos(const struct sk_buff *skb,
+                               struct xfrm_user_acquire *ua,
+                               const struct xfrm_tmpl *xt)
+{
+	if (!xfrm_msg_compat(skb)) {
+		ua->aalgos = xt->aalgos;
+		ua->ealgos = xt->ealgos;
+		ua->calgos = xt->calgos;
+	} else {
+		struct compat_xfrm_user_acquire *compat_ua;
+		compat_ua = (struct compat_xfrm_user_acquire *) ua;
+		compat_ua->aalgos = xt->aalgos;
+		compat_ua->ealgos = xt->ealgos;
+		compat_ua->calgos = xt->calgos;
+	}
+}
+
 static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 			 struct xfrm_tmpl *xt, struct xfrm_policy *xp,
 			 int dir)
@@ -2396,6 +2695,10 @@ static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 	size_t len = sizeof(*ua);
 	struct nlmsghdr *nlh;
 	__u32 seq = xfrm_get_acqseq();
+	struct compat_xfrm_user_acquire *compat_ua;
+
+	if (xfrm_msg_compat(skb))
+		len = sizeof(*compat_ua);
 
 	nlh = nlmsg_put(skb, 0, 0, XFRM_MSG_ACQUIRE, len, 0);
 	if (nlh == NULL)
@@ -2406,10 +2709,14 @@ static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 	memcpy(&ua->saddr, &x->props.saddr, sizeof(ua->saddr));
 	memcpy(&ua->sel, &x->sel, sizeof(ua->sel));
 	copy_to_user_policy(xp, &ua->policy, dir);
-	ua->aalgos = xt->aalgos;
-	ua->ealgos = xt->ealgos;
-	ua->calgos = xt->calgos;
-	ua->seq = x->km.seq = seq;
+	acquire_copy_algos(skb, ua, xt);
+
+	if (xfrm_msg_compat(skb)) {
+		compat_ua = nlmsg_data(nlh);
+		compat_ua->seq = x->km.seq = seq;
+	} else {
+		ua->seq = x->km.seq = seq;
+	}
 
 	if (copy_to_user_tmpl(xp, skb) < 0)
 		goto nlmsg_failure;
@@ -2438,6 +2745,9 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
 	if (build_acquire(skb, x, xt, xp, dir) < 0)
 		BUG();
 
+	if (xfrm_add_compatskb(skb, compat_xfrm_acquire_msgsize(x, xp), GFP_ATOMIC))
+		build_acquire(skb_shinfo(skb)->frag_list, x, xt, xp, dir);
+
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_ACQUIRE, GFP_ATOMIC);
 }
 
@@ -2501,6 +2811,14 @@ static struct xfrm_policy *xfrm_compile_policy(struct sock *sk, int opt,
 	return xp;
 }
 
+static inline size_t compat_xfrm_polexpire_msgsize(struct xfrm_policy *xp)
+{
+	return NLMSG_ALIGN(sizeof(struct compat_xfrm_user_polexpire))
+	       + nla_total_size(sizeof(struct xfrm_user_tmpl) * xp->xfrm_nr)
+	       + nla_total_size(xfrm_user_sec_ctx_size(xp->security))
+	       + userpolicy_type_attrsize();
+}
+
 static inline size_t xfrm_polexpire_msgsize(struct xfrm_policy *xp)
 {
 	return NLMSG_ALIGN(sizeof(struct xfrm_user_polexpire))
@@ -2516,6 +2834,10 @@ static int build_polexpire(struct sk_buff *skb, struct xfrm_policy *xp,
 	size_t len = sizeof(*upe);
 	struct nlmsghdr *nlh;
 	int hard = c->data.hard;
+	struct compat_xfrm_user_polexpire *upe_cmpt;
+
+	if (xfrm_msg_compat(skb))
+		len = sizeof(*upe_cmpt);
 
 	nlh = nlmsg_put(skb, c->pid, 0, XFRM_MSG_POLEXPIRE, len, 0);
 	if (nlh == NULL)
@@ -2529,7 +2851,12 @@ static int build_polexpire(struct sk_buff *skb, struct xfrm_policy *xp,
 		goto nlmsg_failure;
 	if (copy_to_user_policy_type(xp->type, skb) < 0)
 		goto nlmsg_failure;
-	upe->hard = !!hard;
+	if (xfrm_msg_compat(skb)) {
+		upe_cmpt = nlmsg_data(nlh);
+		upe_cmpt->hard = !!hard;
+	} else {
+		upe->hard = !!hard;
+	}
 
 	return nlmsg_end(skb, nlh);
 
@@ -2550,6 +2877,9 @@ static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, struct km_eve
 	if (build_polexpire(skb, xp, dir, c) < 0)
 		BUG();
 
+	if (xfrm_add_compatskb(skb, compat_xfrm_polexpire_msgsize(xp), GFP_ATOMIC))
+		build_polexpire(skb_shinfo(skb)->frag_list, xp, dir, c);
+
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_EXPIRE, GFP_ATOMIC);
 }
 
@@ -2574,6 +2904,13 @@ static int xfrm_notify_policy_headlen(const struct km_event *c)
 	return sizeof(struct xfrm_userpolicy_info);
 }
 
+static int compat_xfrm_notify_policy_headlen(const struct km_event *c)
+{
+	if (c->event == XFRM_MSG_DELPOLICY)
+		return sizeof(struct xfrm_userpolicy_id);
+	return sizeof(struct compat_xfrm_userpolicy_info);
+}
+
 static int copy_to_user_xfrm_notify_policy(struct sk_buff *skb, int dir,
                                            struct xfrm_policy *xp, struct km_event *c)
 {
@@ -2583,6 +2920,11 @@ static int copy_to_user_xfrm_notify_policy(struct sk_buff *skb, int dir,
 	int sizeof_xfrm_userpolicy_info = sizeof(*p);
 	int headlen = xfrm_notify_policy_headlen(c);
 
+	if (xfrm_msg_compat(skb)) {
+		sizeof_xfrm_userpolicy_info = sizeof(struct compat_xfrm_userpolicy_info);
+		headlen = compat_xfrm_notify_policy_headlen(c);
+	}
+
 	nlh = nlmsg_put(skb, c->pid, c->seq, c->event, headlen, 0);
 	if (nlh == NULL)
 		goto nlmsg_failure;
@@ -2632,6 +2974,10 @@ static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *
 	if (copy_to_user_xfrm_notify_policy(skb, dir, xp, c))
 		goto nlmsg_failure;
 
+	if (xfrm_add_compatskb(skb, len, GFP_ATOMIC))
+		copy_to_user_xfrm_notify_policy(skb_shinfo(skb)->frag_list,
+						dir, xp, c);
+
 	return nlmsg_multicast(net->xfrm.nlsk, skb, 0, XFRMNLGRP_POLICY, GFP_ATOMIC);
 
 nlmsg_failure:
-- 
1.6.3.3

Re: [PATCH 4/5] xfrm: CONFIG_COMPAT support for x86 architecture

[Johannes Berg]

On Mon, 2010-02-15 at 17:46 +0100, Florian Westphal wrote:

> +static bool xfrm_msg_compat(const struct sk_buff *skb)
> +{
> +	return unlikely(!!NETLINK_CB(skb).msg_compat);
> +}

The !! seems pointless but that's not why I'm quoting this here.

I think this function should only be used in the input path.

> +static bool xfrm_add_compatskb(struct sk_buff *skb,
> +                               unsigned int len, gfp_t gfp)
> +{

I think this should return the new skb, and I don't think it should set
the msg_compat flag in the NETLINK_CB.

> +static struct sk_buff *xfrm_get_compatskb(struct sk_buff *skb)
> +{

making this no longer needed.

> +/*
> + * avoids #ifdefs all over the place. Use of these must be
> conditional via
> + * xfrm_add_compatskb/xfrm_get_compatskb so compiler can remove
> branches.
> + */
> +#define compat_xfrm_user_expire     xfrm_user_expire
> +#define compat_xfrm_user_acquire    xfrm_user_acquire
> +#define compat_xfrm_user_polexpire  xfrm_user_polexpire
> +#define compat_xfrm_userpolicy_info xfrm_userpolicy_info
> +#define compat_xfrm_usersa_info     xfrm_usersa_info
> +#define compat_xfrm_userspi_info    xfrm_userspi_info

Good thing I didn't have to deal with compat _input_ in wireless since I
had killed the netlink input code :))

> +#endif
> +
> +/*
> + * userspace size of some structures is smaller by this amount
> + * due to different u64 alignment on x86 platform.
> + *
> + * Some of the structures need to use the compat_* structure
> definition
> + * when accessing certain members.
> + */
> +static const u8 xfrm_msg_min_compat_pad[XFRM_NR_MSGTYPES] = {
> +	[XFRM_MSG_NEWSA       - XFRM_MSG_BASE] = 4, /* padding at end */
> +	[XFRM_MSG_NEWPOLICY   - XFRM_MSG_BASE] = 4, /* padding at end */
> +
> +	[XFRM_MSG_ALLOCSPI    - XFRM_MSG_BASE] = 4, /* access might need
> compat_ */
> +	[XFRM_MSG_ACQUIRE     - XFRM_MSG_BASE] = 4, /* access might need
> compat_ */
> +	[XFRM_MSG_EXPIRE      - XFRM_MSG_BASE] = 4, /* access might need
> compat_ */
> +	[XFRM_MSG_UPDPOLICY   - XFRM_MSG_BASE] = 4, /* padding at end */
> +	[XFRM_MSG_UPDSA       - XFRM_MSG_BASE] = 4, /* padding at end */
> +	[XFRM_MSG_POLEXPIRE   - XFRM_MSG_BASE] = 4, /* access might need
> compat_ */
> +};

Shouldn't that be some sizeof() trickery instead of hardcoding 4?

> +static int get_user_expire_hard(const struct sk_buff *skb,
> +                                const struct xfrm_user_expire *ue)
> +{
> +	if (xfrm_msg_compat(skb)) {
> +		const struct compat_xfrm_user_expire *cmpt;
> +		cmpt = (const struct compat_xfrm_user_expire*) ue;
> +		return cmpt->hard;
> +	}
> +	return ue->hard;
> +}

That looks good to me, good use of the compat helper.

> @@ -700,6 +856,9 @@ static int copy_one_state(struct sk_buff *skb,
> struct xfrm_state *x, struct xfrm
>  	size_t len = sizeof(*p);
>  	int err;
>  
> +	if (xfrm_msg_compat(skb))
> +		len = sizeof(struct compat_xfrm_usersa_info);
> +

I really dislike this use though.

> @@ -724,6 +883,13 @@ static int dump_one_state(struct xfrm_state *x,
> int count, void *ptr)
>  	struct xfrm_dump_info *sp = ptr;
>  	struct sk_buff *skb = sp->out_skb;
>  	int ret = copy_one_state(skb, x, sp);
> +#ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT
> +	if (ret == 0) {
> +		skb = xfrm_get_compatskb(skb);
> +		if (skb)
> +			copy_one_state(skb, x, sp);
> +	}
> +#endif
>  	return ret;
>  }

And that's really convoluted.

> @@ -753,6 +919,8 @@ static int xfrm_dump_sa(struct sk_buff *skb,
> struct netlink_callback *cb)
>  		xfrm_state_walk_init(walk, 0);
>  	}
>  
> +	xfrm_add_compatskb(skb, NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +
>  	(void) xfrm_state_walk(net, walk, dump_one_state, &info);
>  
>  	return skb->len;

And this just seems wrong -- doesn't that fill _only_ the compat skb
here then??

(same thing happens again in this file)

> @@ -2200,6 +2455,9 @@ static int xfrm_exp_state_notify(struct
> xfrm_state *x, struct km_event *c)
>  	if (build_expire(skb, x, c) < 0)
>  		BUG();
>  
> +	if (xfrm_add_compatskb(skb, compat_xfrm_expire_msgsize(), GFP_ATOMIC))
> +		compat_build_expire(skb_shinfo(skb)->frag_list, x, c);

This is a good model, although I'd change it to be like this:

if (cskb = xfrm_add_compatskb(skb, ...))
compat_build_expire(cskb, x, c);

And I think the code would be a lot clearer if the other user were like

build_foo(skb, ..., false /* not compat */);
if ((cskb = xfrm_add_compatskb(skb, ...)))
build_foo(cskb, ..., true /* compat */);

instead of all the trickery with setting msg_compat on the output path
and then checking it again in the fill functions.

So for example:

> @@ -2302,6 +2567,11 @@ static int copy_to_user_xfrm_notify_sa(struct
> sk_buff *skb,
>  	int sizeof_xfrm_usersa_info = sizeof(*p);
>  	int headlen = xfrm_notify_sa_headlen(c);
>  
> +	if (xfrm_msg_compat(skb)) {
> +		headlen = compat_xfrm_notify_sa_headlen(c);
> +		sizeof_xfrm_usersa_info = sizeof(struct compat_xfrm_usersa_info);
> +	}

That'd get a compat parameter, and use it here instead of the
xfrm_msg_compat(skb) check.

> 	nlh = nlmsg_put(skb, c->pid, c->seq, c->event, headlen, 0);
>  	if (nlh == NULL)
>  		goto nla_put_failure;
> @@ -2347,6 +2617,9 @@ static int xfrm_notify_sa(struct xfrm_state *x,
> struct km_event *c)
>  	if (copy_to_user_xfrm_notify_sa(skb, x, c))
>  		goto nla_put_failure;
>  
> +	if (xfrm_add_compatskb(skb, len, GFP_ATOMIC))
> +		copy_to_user_xfrm_notify_sa(skb_shinfo(skb)->frag_list, x, c);

and instead of touching frag_list here, you'd pass the result of
xfrm_add_compatskb().

That way, the use of xfrm_msg_compat() is restricted to the input path,
which I think is much clearer.

johannes 

Re: [PATCH 4/5] xfrm: CONFIG_COMPAT support for x86 architecture

[Florian Westphal]

Johannes Berg <johannes@sipsolutions.net> wrote:
> > +static bool xfrm_msg_compat(const struct sk_buff *skb)
> > +{
> > +	return unlikely(!!NETLINK_CB(skb).msg_compat);
> > +}
> 
> The !! seems pointless but that's not why I'm quoting this here.
> 
> I think this function should only be used in the input path.

Fair enough.  I'll see about introducing "bool compat" arguments
to the functions that currently use this helper in the output path.

> > +static bool xfrm_add_compatskb(struct sk_buff *skb,
> > +                               unsigned int len, gfp_t gfp)
> > +{
> 
> I think this should return the new skb, and I don't think it should set
> the msg_compat flag in the NETLINK_CB.

Right, that would no longer be necessary if xfrm_msg_compat() is
restricted to the input path.

> > +static struct sk_buff *xfrm_get_compatskb(struct sk_buff *skb)
> > +{
> 
> making this no longer needed.
> 

[..]
> Shouldn't that be some sizeof() trickery instead of hardcoding 4?

I'll do that. For x86/x86_64 the delta is always 4 (or 0), though.

> > @@ -700,6 +856,9 @@ static int copy_one_state(struct sk_buff *skb,
> > struct xfrm_state *x, struct xfrm
> >  	size_t len = sizeof(*p);
> >  	int err;
> >  
> > +	if (xfrm_msg_compat(skb))
> > +		len = sizeof(struct compat_xfrm_usersa_info);
> > +
> 
> I really dislike this use though.

Alright, I will add a "bool is_compat" argument instead.

> > @@ -724,6 +883,13 @@ static int dump_one_state(struct xfrm_state *x,
> > int count, void *ptr)
> >  	struct xfrm_dump_info *sp = ptr;
> >  	struct sk_buff *skb = sp->out_skb;
> >  	int ret = copy_one_state(skb, x, sp);
> > +#ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT
> > +	if (ret == 0) {
> > +		skb = xfrm_get_compatskb(skb);
> > +		if (skb)
> > +			copy_one_state(skb, x, sp);
> > +	}
> > +#endif
> >  	return ret;
> >  }
> 
> And that's really convoluted.

dump_one_state() is called from xfrm_state_walk(), so it has to prepare
both native and compat skb.

The "ifdef" is not necessary; I added it because gcc 4.3.4 did
no longer inline copy_one_state() without them in the
COMPAT_FOR_U64_ALIGNMENT=n case.

> > @@ -753,6 +919,8 @@ static int xfrm_dump_sa(struct sk_buff *skb,
> > struct netlink_callback *cb)
> >  		xfrm_state_walk_init(walk, 0);
> >  	}
> >  
> > +	xfrm_add_compatskb(skb, NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +
> >  	(void) xfrm_state_walk(net, walk, dump_one_state, &info);
> >  
> >  	return skb->len;
> 
> And this just seems wrong -- doesn't that fill _only_ the compat skb
> here then??

No, dump_one_state() calls copy_one_state() twice (once for
skb, once for ->frag_list.

> > @@ -2200,6 +2455,9 @@ static int xfrm_exp_state_notify(struct
> > xfrm_state *x, struct km_event *c)
> >  	if (build_expire(skb, x, c) < 0)
> >  		BUG();
> >  
> > +	if (xfrm_add_compatskb(skb, compat_xfrm_expire_msgsize(), GFP_ATOMIC))
> > +		compat_build_expire(skb_shinfo(skb)->frag_list, x, c);
> 
> This is a good model, although I'd change it to be like this:
> 
> if (cskb = xfrm_add_compatskb(skb, ...))
> compat_build_expire(cskb, x, c);

good point, I'll change this accordingly.

> And I think the code would be a lot clearer if the other user were like
> 
> build_foo(skb, ..., false /* not compat */);
> if ((cskb = xfrm_add_compatskb(skb, ...)))
> build_foo(cskb, ..., true /* compat */);
> 
> instead of all the trickery with setting msg_compat on the output path
> and then checking it again in the fill functions.

Thanks a lot for spending time on reviewing this; I'll make the changes
you suggested.

Florian

[PATCH 5/5] net: sock_aio_write: set CMSG_MSG_COMPAT flag if is_compat_task

[Florian Westphal]

From: Florian Westphal <fwestphal@astaro.com>

some programs (e.g. pluto ike daemon), send netlink data to the
kernel via write().

So far, the CMSG_MSG_COMPAT flag is not set in this case, which
results in the kernel interpreting the netlink data the wrong way.

An alternative solution is to switch userspace to sendmsg() when
using netlink sockets.

Signed-off-by: Florian Westphal <fwestphal@astaro.com>
---
 net/socket.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 769c386..e6a6a67 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -837,6 +837,10 @@ static ssize_t do_sock_write(struct msghdr *msg, struct kiocb *iocb,
 	msg->msg_iov = (struct iovec *)iov;
 	msg->msg_iovlen = nr_segs;
 	msg->msg_flags = (file->f_flags & O_NONBLOCK) ? MSG_DONTWAIT : 0;
+#ifdef CONFIG_COMPAT
+	if (is_compat_task())
+		msg->msg_flags |= MSG_CMSG_COMPAT;
+#endif
 	if (sock->type == SOCK_SEQPACKET)
 		msg->msg_flags |= MSG_EOR;
 
-- 
1.6.3.3