[PATCH] tipc: revert some previously ABI breaking changes to the user space tipc interface

[PATCH] tipc: revert some previously ABI breaking changes to the user space tipc interface

[nhorman]

Based on discussions with Leandro Lucarella and Jon Maloy, it appears we've
managed to break the TIPC user space ABI.  While it would be really nice to have
some level of consistency in regards to on the wire byte order, we can't do that
if it breaks user space.  This patch series reverts the offending patches that
caused this, and puts us back in a state that allows user space to work
properly.  We can now go back and look at methods to get consistent on the wire
byte order while maintaining host byte order in the user space API.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

[PATCH 1/2] Revert c6537d6742985da1fbf12ae26cde6a096fd35b5c

[nhorman]

From: Neil Horman <nhorman@tuxdriver.com>

Backout the tipc changes to the flags int he subscription message.  These
changees, while reasonable on the surface, interefere with user space ABI
compatibility which is a no-no.  This was part of the changes to fix the
endianess issues in the TIPC protocol, which would be really nice to do but we
need to do so in a way that is backwards compatible with user space.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
---
 include/linux/tipc.h |   30 ++++++++++++++++++------------
 net/tipc/subscr.c    |   15 +++++----------
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/linux/tipc.h b/include/linux/tipc.h
index 181c8d0..d10614b 100644
--- a/include/linux/tipc.h
+++ b/include/linux/tipc.h
@@ -127,17 +127,23 @@ static inline unsigned int tipc_node(__u32 addr)
  * TIPC topology subscription service definitions
  */
 
-#define TIPC_SUB_SERVICE     	0x00  	/* Filter for service availability    */
-#define TIPC_SUB_PORTS     	0x01  	/* Filter for port availability  */
-#define TIPC_SUB_CANCEL         0x04    /* Cancel a subscription         */
+#define TIPC_SUB_PORTS     	0x01  	/* filter for port availability */
+#define TIPC_SUB_SERVICE     	0x02  	/* filter for service availability */
+#define TIPC_SUB_CANCEL         0x04    /* cancel a subscription */
+#if 0
+/* The following filter options are not currently implemented */
+#define TIPC_SUB_NO_BIND_EVTS	0x04	/* filter out "publish" events */
+#define TIPC_SUB_NO_UNBIND_EVTS	0x08	/* filter out "withdraw" events */
+#define TIPC_SUB_SINGLE_EVT	0x10	/* expire after first event */
+#endif
 
 #define TIPC_WAIT_FOREVER	~0	/* timeout for permanent subscription */
 
 struct tipc_subscr {
-	struct tipc_name_seq seq;	/* NBO. Name sequence of interest */
-	__u32 timeout;			/* NBO. Subscription duration (in ms) */
-        __u32 filter;   		/* NBO. Bitmask of filter options */
-	char usr_handle[8];		/* Opaque. Available for subscriber use */
+	struct tipc_name_seq seq;	/* name sequence of interest */
+	__u32 timeout;			/* subscription duration (in ms) */
+        __u32 filter;   		/* bitmask of filter options */
+	char usr_handle[8];		/* available for subscriber use */
 };
 
 #define TIPC_PUBLISHED		1	/* publication event */
@@ -145,11 +151,11 @@ struct tipc_subscr {
 #define TIPC_SUBSCR_TIMEOUT	3	/* subscription timeout event */
 
 struct tipc_event {
-	__u32 event;			/* NBO. Event type, as defined above */
-	__u32 found_lower;		/* NBO. Matching name seq instances  */
-	__u32 found_upper;		/*  "      "       "   "    "        */
-	struct tipc_portid port;	/* NBO. Associated port              */
-	struct tipc_subscr s;		/* Original, associated subscription */
+	__u32 event;			/* event type */
+	__u32 found_lower;		/* matching name seq instances */
+	__u32 found_upper;		/*    "      "    "     "      */
+	struct tipc_portid port;	/* associated port */
+	struct tipc_subscr s;		/* associated subscription */
 };
 
 /*
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 18813ac..ecccfdb 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -274,7 +274,7 @@ static void subscr_cancel(struct tipc_subscr *s,
 {
 	struct subscription *sub;
 	struct subscription *sub_temp;
-	__u32 type, lower, upper, timeout, filter;
+	__u32 type, lower, upper;
 	int found = 0;
 
 	/* Find first matching subscription, exit if not found */
@@ -282,18 +282,12 @@ static void subscr_cancel(struct tipc_subscr *s,
 	type = ntohl(s->seq.type);
 	lower = ntohl(s->seq.lower);
 	upper = ntohl(s->seq.upper);
-	timeout = ntohl(s->timeout);
-	filter = ntohl(s->filter) & ~TIPC_SUB_CANCEL;
 
 	list_for_each_entry_safe(sub, sub_temp, &subscriber->subscription_list,
 				 subscription_list) {
 			if ((type == sub->seq.type) &&
 			    (lower == sub->seq.lower) &&
-			    (upper == sub->seq.upper) &&
-			    (timeout == sub->timeout) &&
-                            (filter == sub->filter) &&
-                             !memcmp(s->usr_handle,sub->evt.s.usr_handle,
-				     sizeof(s->usr_handle)) ){
+			    (upper == sub->seq.upper)) {
 				found = 1;
 				break;
 			}
@@ -310,7 +304,7 @@ static void subscr_cancel(struct tipc_subscr *s,
 		k_term_timer(&sub->timer);
 		spin_lock_bh(subscriber->lock);
 	}
-	dbg("Cancel: removing sub %u,%u,%u from subscriber %p list\n",
+	dbg("Cancel: removing sub %u,%u,%u from subscriber %x list\n",
 	    sub->seq.type, sub->seq.lower, sub->seq.upper, subscriber);
 	subscr_del(sub);
 }
@@ -358,7 +352,8 @@ static struct subscription *subscr_subscribe(struct tipc_subscr *s,
 	sub->seq.upper = ntohl(s->seq.upper);
 	sub->timeout = ntohl(s->timeout);
 	sub->filter = ntohl(s->filter);
-	if ((sub->filter && (sub->filter != TIPC_SUB_PORTS)) ||
+	if ((!(sub->filter & TIPC_SUB_PORTS) ==
+	     !(sub->filter & TIPC_SUB_SERVICE)) ||
 	    (sub->seq.lower > sub->seq.upper)) {
 		warn("Subscription rejected, illegal request\n");
 		kfree(sub);
-- 
1.7.2.3

Re: [PATCH 1/2] Revert c6537d6742985da1fbf12ae26cde6a096fd35b5c

[David Miller]

From: nhorman@tuxdriver.com
Date: Thu, 21 Oct 2010 07:06:15 -0400

> From: Neil Horman <nhorman@tuxdriver.com>
> 
> Backout the tipc changes to the flags int he subscription message.  These
> changees, while reasonable on the surface, interefere with user space ABI
> compatibility which is a no-no.  This was part of the changes to fix the
> endianess issues in the TIPC protocol, which would be really nice to do but we
> need to do so in a way that is backwards compatible with user space.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Applied and queued up for -stable.

[PATCH 2/2] Revert d88dca79d3852a3623f606f781e013d61486828a

[nhorman]

From: Neil Horman <nhorman@tuxdriver.com>

TIPC needs to have its endianess issues fixed.  Unfortunately, the format of a
subscriber message is passed in directly from user space, so requiring this
message to be in network byte order breaks user space ABI.  Revert this change
until such time as we can determine how to do this in a backwards compatible
manner.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
---
 net/tipc/subscr.c |   57 ++++++++++++++++++++++++++++++++--------------------
 net/tipc/subscr.h |    2 +
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index ecccfdb..3331396 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -76,6 +76,19 @@ struct top_srv {
 static struct top_srv topsrv = { 0 };
 
 /**
+ * htohl - convert value to endianness used by destination
+ * @in: value to convert
+ * @swap: non-zero if endianness must be reversed
+ *
+ * Returns converted value
+ */
+
+static u32 htohl(u32 in, int swap)
+{
+	return swap ? swab32(in) : in;
+}
+
+/**
  * subscr_send_event - send a message containing a tipc_event to the subscriber
  *
  * Note: Must not hold subscriber's server port lock, since tipc_send() will
@@ -94,11 +107,11 @@ static void subscr_send_event(struct subscription *sub,
 	msg_sect.iov_base = (void *)&sub->evt;
 	msg_sect.iov_len = sizeof(struct tipc_event);
 
-	sub->evt.event = htonl(event);
-	sub->evt.found_lower = htonl(found_lower);
-	sub->evt.found_upper = htonl(found_upper);
-	sub->evt.port.ref = htonl(port_ref);
-	sub->evt.port.node = htonl(node);
+	sub->evt.event = htohl(event, sub->swap);
+	sub->evt.found_lower = htohl(found_lower, sub->swap);
+	sub->evt.found_upper = htohl(found_upper, sub->swap);
+	sub->evt.port.ref = htohl(port_ref, sub->swap);
+	sub->evt.port.node = htohl(node, sub->swap);
 	tipc_send(sub->server_ref, 1, &msg_sect);
 }
 
@@ -274,23 +287,16 @@ static void subscr_cancel(struct tipc_subscr *s,
 {
 	struct subscription *sub;
 	struct subscription *sub_temp;
-	__u32 type, lower, upper;
 	int found = 0;
 
 	/* Find first matching subscription, exit if not found */
 
-	type = ntohl(s->seq.type);
-	lower = ntohl(s->seq.lower);
-	upper = ntohl(s->seq.upper);
-
 	list_for_each_entry_safe(sub, sub_temp, &subscriber->subscription_list,
 				 subscription_list) {
-			if ((type == sub->seq.type) &&
-			    (lower == sub->seq.lower) &&
-			    (upper == sub->seq.upper)) {
-				found = 1;
-				break;
-			}
+		if (!memcmp(s, &sub->evt.s, sizeof(struct tipc_subscr))) {
+			found = 1;
+			break;
+		}
 	}
 	if (!found)
 		return;
@@ -319,10 +325,16 @@ static struct subscription *subscr_subscribe(struct tipc_subscr *s,
 					     struct subscriber *subscriber)
 {
 	struct subscription *sub;
+	int swap;
+
+	/* Determine subscriber's endianness */
+
+	swap = !(s->filter & (TIPC_SUB_PORTS | TIPC_SUB_SERVICE));
 
 	/* Detect & process a subscription cancellation request */
 
-	if (ntohl(s->filter) & TIPC_SUB_CANCEL) {
+	if (s->filter & htohl(TIPC_SUB_CANCEL, swap)) {
+		s->filter &= ~htohl(TIPC_SUB_CANCEL, swap);
 		subscr_cancel(s, subscriber);
 		return NULL;
 	}
@@ -347,11 +359,11 @@ static struct subscription *subscr_subscribe(struct tipc_subscr *s,
 
 	/* Initialize subscription object */
 
-	sub->seq.type = ntohl(s->seq.type);
-	sub->seq.lower = ntohl(s->seq.lower);
-	sub->seq.upper = ntohl(s->seq.upper);
-	sub->timeout = ntohl(s->timeout);
-	sub->filter = ntohl(s->filter);
+	sub->seq.type = htohl(s->seq.type, swap);
+	sub->seq.lower = htohl(s->seq.lower, swap);
+	sub->seq.upper = htohl(s->seq.upper, swap);
+	sub->timeout = htohl(s->timeout, swap);
+	sub->filter = htohl(s->filter, swap);
 	if ((!(sub->filter & TIPC_SUB_PORTS) ==
 	     !(sub->filter & TIPC_SUB_SERVICE)) ||
 	    (sub->seq.lower > sub->seq.upper)) {
@@ -364,6 +376,7 @@ static struct subscription *subscr_subscribe(struct tipc_subscr *s,
 	INIT_LIST_HEAD(&sub->nameseq_list);
 	list_add(&sub->subscription_list, &subscriber->subscription_list);
 	sub->server_ref = subscriber->port_ref;
+	sub->swap = swap;
 	memcpy(&sub->evt.s, s, sizeof(struct tipc_subscr));
 	atomic_inc(&topsrv.subscription_count);
 	if (sub->timeout != TIPC_WAIT_FOREVER) {
diff --git a/net/tipc/subscr.h b/net/tipc/subscr.h
index c20f496..45d89bf 100644
--- a/net/tipc/subscr.h
+++ b/net/tipc/subscr.h
@@ -53,6 +53,7 @@ typedef void (*tipc_subscr_event) (struct subscription *sub,
  * @nameseq_list: adjacent subscriptions in name sequence's subscription list
  * @subscription_list: adjacent subscriptions in subscriber's subscription list
  * @server_ref: object reference of server port associated with subscription
+ * @swap: indicates if subscriber uses opposite endianness in its messages
  * @evt: template for events generated by subscription
  */
 
@@ -65,6 +66,7 @@ struct subscription {
 	struct list_head nameseq_list;
 	struct list_head subscription_list;
 	u32 server_ref;
+	int swap;
 	struct tipc_event evt;
 };
 
-- 
1.7.2.3

Re: [PATCH 2/2] Revert d88dca79d3852a3623f606f781e013d61486828a

[David Miller]

From: nhorman@tuxdriver.com
Date: Thu, 21 Oct 2010 07:06:16 -0400

> From: Neil Horman <nhorman@tuxdriver.com>
> 
> TIPC needs to have its endianess issues fixed.  Unfortunately, the format of a
> subscriber message is passed in directly from user space, so requiring this
> message to be in network byte order breaks user space ABI.  Revert this change
> until such time as we can determine how to do this in a backwards compatible
> manner.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Applied and queued up for -stable.

Re: [PATCH] tipc: revert some previously ABI breaking changes to the user space tipc interface

[Paul Gortmaker]

On 10-10-21 07:06 AM, nhorman@tuxdriver.com wrote:
> Based on discussions with Leandro Lucarella and Jon Maloy, it appears we've
> managed to break the TIPC user space ABI.  While it would be really nice to have
> some level of consistency in regards to on the wire byte order, we can't do that
> if it breaks user space.  This patch series reverts the offending patches that
> caused this, and puts us back in a state that allows user space to work
> properly.  We can now go back and look at methods to get consistent on the wire
> byte order while maintaining host byte order in the user space API.

Thanks Neil for working with Jon and Leandro on this and getting
a fix out quickly (faster than I could have, for sure.)

Paul.

> 
> Signed-off-by: Neil Horman<nhorman@tuxdriver.com>


------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in  U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store 
http://p.sf.net/sfu/nokia-dev2dev

Re: [PATCH] tipc: revert some previously ABI breaking changes to the user space tipc interface

[Neil Horman]

On Thu, Oct 21, 2010 at 09:06:22AM -0400, Paul Gortmaker wrote:
> On 10-10-21 07:06 AM, nhorman@tuxdriver.com wrote:
> > Based on discussions with Leandro Lucarella and Jon Maloy, it appears we've
> > managed to break the TIPC user space ABI.  While it would be really nice to have
> > some level of consistency in regards to on the wire byte order, we can't do that
> > if it breaks user space.  This patch series reverts the offending patches that
> > caused this, and puts us back in a state that allows user space to work
> > properly.  We can now go back and look at methods to get consistent on the wire
> > byte order while maintaining host byte order in the user space API.
> 
> Thanks Neil for working with Jon and Leandro on this and getting
> a fix out quickly (faster than I could have, for sure.)
> 
> Paul.
> 
Not to worry, and apologies for the noise.  I missed the possibility previously
that received messages in that path could come in from the local client as well
as over the wire.  As soon as I get a few other things out of the way, I'll take
a second look at this.  It would be really nice if we could create a consisten
byte order on the wire without busting userspace.

Neil

> > 
> > Signed-off-by: Neil Horman<nhorman@tuxdriver.com>
> 
>