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

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.

I really dislike this use though.

And that's really convoluted.

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

(same thing happens again in this file)

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:

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

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