| Message ID | 20100308200315.GE23634@hmsreliant.think-freely.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Neil Horman <nhorman@tuxdriver.com> Date: Mon, 8 Mar 2010 15:03:15 -0500 > Remove htohl implementation from tipc Applied, thanks Neil. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi there: There are a couple of problems with this patch that need to be resolved before it can be applied to the upstream kernel. 1) Neil's replacement of the htohl() call with the equivalent htonl()/ntohl() calls, while well intentioned, was done without understanding why the htohl() calls were put there in the first place. In addition, the TIPC specification that he used to justify some of his decisions is out-dated, and doesn't reflect the latest version of the TIPC protocol. (I'll discuss this further in a follow-up email.) 2) Neil's alteration of the memcpy() in the subscription cancelation routine is simply wrong. The pieces of the data structure that he claims are local are not local, and must be checked to ensure that the cancellation is done properly. I'm also surprised to see that this patch was immediately applied to net-2.6. First, there was no opportunity given for people to comment on the patch. Secondly, I would have expected this patch to be applied to net-next-2.6, since the functionality being changed here (at least the first part of it) is more like a feature enhancement than a bug fix. Am I misunderstanding the process being followed here? If so, please explain ... Regards, Al > -----Original Message----- > From: Neil Horman [mailto:nhorman@tuxdriver.com] > Sent: Monday, March 08, 2010 3:03 PM > To: netdev@vger.kernel.org > Cc: Stephens, Allan; davem@davemloft.net; nhorman@tuxdriver.com > Subject: [PATCH] tipc: fix endianness on tipc subscriber messages > > Remove htohl implementation from tipc > > I was working on forward porting the downstream commits for > TIPC and ran accross this one: > http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/all > an/tipc.git;a=commitdiff;h=894279b9437b63cbb02405ad5b8e033b51e4e31e > > I was going to just take it, when I looked closer and noted > what it was doing. > This is basically a routine to byte swap fields of data in > sent/received packets for tipc, dependent upon the receivers > guessed endianness of the peer when a connection is > established. Asside from just seeming silly to me, it > appears to violate the latest RFC draft for tipc: > http://tipc.sourceforge.net/doc/draft-spec-tipc-02.txt > Which, according to section 4.2 and 4.3.3, requires that all > fields of all commands be sent in network byte order. So > instead of just taking this patch, instead I'm removing the > htohl function and replacing the calls with calls to ntohl in > the rx path and htonl in the send path. > > As part of this fix, I'm also changing the subscr_cancel > function, which searches the list of subscribers, using a > memcmp of the entire subscriber list, for the entry to tear > down. unfortunately it memcmps the entire tipc_subscr > structure which has several bits that are private to the > local side, so nothing will ever match. section 5.2 of the > draft spec indicates the <type,upper,lower> tuple should > uniquely identify a subscriber, so convert subscr_cancel to > just match on those fields (properly endian swapped). > > I've tested this using the tipc test suite, and its passed > without issue. > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > CC: Allan Stephens <allan.stephens@windriver.com> > > > subscr.c | 57 > ++++++++++++++++++++++----------------------------------- > subscr.h | 2 -- > 2 files changed, 22 insertions(+), 37 deletions(-) > > diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c index > ac91f0d..ff123e5 100644 > --- a/net/tipc/subscr.c > +++ b/net/tipc/subscr.c > @@ -76,19 +76,6 @@ 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 @@ -107,11 +94,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 = 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); > + 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); > tipc_send(sub->server_ref, 1, &msg_sect); } > > @@ -287,16 +274,23 @@ 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 (!memcmp(s, &sub->evt.s, sizeof(struct > tipc_subscr))) { > - found = 1; > - break; > - } > + if ((type == sub->seq.type) && > + (lower == sub->seq.lower) && > + (upper == sub->seq.upper)) { > + found = 1; > + break; > + } > } > if (!found) > return; > @@ -325,16 +319,10 @@ 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 (s->filter & htohl(TIPC_SUB_CANCEL, swap)) { > - s->filter &= ~htohl(TIPC_SUB_CANCEL, swap); > + if (ntohl(s->filter) & TIPC_SUB_CANCEL) { > subscr_cancel(s, subscriber); > return NULL; > } > @@ -359,11 +347,11 @@ static struct subscription > *subscr_subscribe(struct tipc_subscr *s, > > /* Initialize subscription object */ > > - 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); > + 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); > if ((!(sub->filter & TIPC_SUB_PORTS) == > !(sub->filter & TIPC_SUB_SERVICE)) || > (sub->seq.lower > sub->seq.upper)) { @@ -376,7 > +364,6 @@ 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 45d89bf..c20f496 100644 > --- a/net/tipc/subscr.h > +++ b/net/tipc/subscr.h > @@ -53,7 +53,6 @@ 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 > */ > > @@ -66,7 +65,6 @@ struct subscription { > struct list_head nameseq_list; > struct list_head subscription_list; > u32 server_ref; > - int swap; > struct tipc_event evt; > }; > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Stephens, Allan" <allan.stephens@windriver.com> Date: Mon, 8 Mar 2010 12:48:38 -0800 > Secondly, I would have expected this patch to be applied to > net-next-2.6, since the functionality being changed here (at least > the first part of it) is more like a feature enhancement than a bug > fix. It fixes a bug so I'm going to apply it. You can't disappear from maintaining this shinking ship for years then complain about people who are actually working to get upstream back into shape. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 08, 2010 at 12:48:38PM -0800, Stephens, Allan wrote: > Hi there: > > There are a couple of problems with this patch that need to be resolved > before it can be applied to the upstream kernel. > > 1) Neil's replacement of the htohl() call with the equivalent > htonl()/ntohl() calls, while well intentioned, was done without > understanding why the htohl() calls were put there in the first place. I'd love to hear what those reasons are. You're formating on-the-wire data to an endianness defined by the receiving entity. I'm hard pressed to imagine a reason why thats sane. > In addition, the TIPC specification that he used to justify some of his > decisions is out-dated, and doesn't reflect the latest version of the > TIPC protocol. (I'll discuss this further in a follow-up email.) > Please, point out a more recent spec. The one I referenced is the most recent publically available spec that I can find. although again, I'm hard pressed to believe that the spec has changed to include a requirement to send data in receiver byte order. > 2) Neil's alteration of the memcpy() in the subscription cancelation > routine is simply wrong. The pieces of the data structure that he > claims are local are not local, and must be checked to ensure that the > cancellation is done properly. > The origional memcmp checks sizeof(tipc_subscr) bytes of each entry in the array. tipc_subscr is defined as: struct tipc_subscr { 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 */ }; In subscr_subscribe (from which we also call subscr_cancel), we allocate new entries for that list, only fillingout the seq.type, seq.lower, seq.upper, timeout and filter. So if anyone locally changes usr_handle (which I see several places that do), the memcmp won't match up properly. Additionally, I'm hard pressed to believe (and this would be supported by the section of the spec that I referenced), timeout, and filter also don't bear on the uniqueness of the subscriber Id. Of course a subsequent version of the spec might have changed that, But if it is, please point it out to me. > I'm also surprised to see that this patch was immediately applied to > net-2.6. First, there was no opportunity given for people to comment on > the patch. Secondly, I would have expected this patch to be applied to > net-next-2.6, since the functionality being changed here (at least the > first part of it) is more like a feature enhancement than a bug fix. Am > I misunderstanding the process being followed here? If so, please > explain ... > I'll let David Comment of this, since the destination tree is his final decision. Neil > Regards, > Al > > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > Sent: Monday, March 08, 2010 3:03 PM > > To: netdev@vger.kernel.org > > Cc: Stephens, Allan; davem@davemloft.net; nhorman@tuxdriver.com > > Subject: [PATCH] tipc: fix endianness on tipc subscriber messages > > > > Remove htohl implementation from tipc > > > > I was working on forward porting the downstream commits for > > TIPC and ran accross this one: > > http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/all > > an/tipc.git;a=commitdiff;h=894279b9437b63cbb02405ad5b8e033b51e4e31e > > > > I was going to just take it, when I looked closer and noted > > what it was doing. > > This is basically a routine to byte swap fields of data in > > sent/received packets for tipc, dependent upon the receivers > > guessed endianness of the peer when a connection is > > established. Asside from just seeming silly to me, it > > appears to violate the latest RFC draft for tipc: > > http://tipc.sourceforge.net/doc/draft-spec-tipc-02.txt > > Which, according to section 4.2 and 4.3.3, requires that all > > fields of all commands be sent in network byte order. So > > instead of just taking this patch, instead I'm removing the > > htohl function and replacing the calls with calls to ntohl in > > the rx path and htonl in the send path. > > > > As part of this fix, I'm also changing the subscr_cancel > > function, which searches the list of subscribers, using a > > memcmp of the entire subscriber list, for the entry to tear > > down. unfortunately it memcmps the entire tipc_subscr > > structure which has several bits that are private to the > > local side, so nothing will ever match. section 5.2 of the > > draft spec indicates the <type,upper,lower> tuple should > > uniquely identify a subscriber, so convert subscr_cancel to > > just match on those fields (properly endian swapped). > > > > I've tested this using the tipc test suite, and its passed > > without issue. > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > CC: Allan Stephens <allan.stephens@windriver.com> > > > > > > subscr.c | 57 > > ++++++++++++++++++++++----------------------------------- > > subscr.h | 2 -- > > 2 files changed, 22 insertions(+), 37 deletions(-) > > > > diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c index > > ac91f0d..ff123e5 100644 > > --- a/net/tipc/subscr.c > > +++ b/net/tipc/subscr.c > > @@ -76,19 +76,6 @@ 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 @@ -107,11 +94,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 = 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); > > + 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); > > tipc_send(sub->server_ref, 1, &msg_sect); } > > > > @@ -287,16 +274,23 @@ 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 (!memcmp(s, &sub->evt.s, sizeof(struct > > tipc_subscr))) { > > - found = 1; > > - break; > > - } > > + if ((type == sub->seq.type) && > > + (lower == sub->seq.lower) && > > + (upper == sub->seq.upper)) { > > + found = 1; > > + break; > > + } > > } > > if (!found) > > return; > > @@ -325,16 +319,10 @@ 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 (s->filter & htohl(TIPC_SUB_CANCEL, swap)) { > > - s->filter &= ~htohl(TIPC_SUB_CANCEL, swap); > > + if (ntohl(s->filter) & TIPC_SUB_CANCEL) { > > subscr_cancel(s, subscriber); > > return NULL; > > } > > @@ -359,11 +347,11 @@ static struct subscription > > *subscr_subscribe(struct tipc_subscr *s, > > > > /* Initialize subscription object */ > > > > - 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); > > + 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); > > if ((!(sub->filter & TIPC_SUB_PORTS) == > > !(sub->filter & TIPC_SUB_SERVICE)) || > > (sub->seq.lower > sub->seq.upper)) { @@ -376,7 > > +364,6 @@ 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 45d89bf..c20f496 100644 > > --- a/net/tipc/subscr.h > > +++ b/net/tipc/subscr.h > > @@ -53,7 +53,6 @@ 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 > > */ > > > > @@ -66,7 +65,6 @@ struct subscription { > > struct list_head nameseq_list; > > struct list_head subscription_list; > > u32 server_ref; > > - int swap; > > struct tipc_event evt; > > }; > > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Neil wrote: > > There are a couple of problems with this patch that need to be > > resolved before it can be applied to the upstream kernel. > > > > 1) Neil's replacement of the htohl() call with the equivalent > > htonl()/ntohl() calls, while well intentioned, was done without > > understanding why the htohl() calls were put there in the > first place. > I'd love to hear what those reasons are. You're formating > on-the-wire data to an endianness defined by the receiving > entity. I'm hard pressed to imagine a reason why thats sane. When TIPC evolved from using ioctl() as the API to the topology service to a message-based interface it was felt that it would be desirable to allow the fields in those messages to be formatted in host-byte order, since that was what existing applications were already doing. I presume that the idea was to make the migration process as painless as possible, and to avoid problems with future applications that didn't take endianness into account. However, since it was theoretically possible for an application to send a message to a topology service on another node (whose endianness was unknown), the node that received the message needed to be able to recognize if a message contents had the "wrong" endianness and do the necessary conversion. That is what was implemented back in TIPC 1.5, and this API still exists today which is why the htohl() conversions are done the way they are. (I can't say I'm a fan of the htohl() name ... it would be nice to have something more meaningful.) The difficulty with the patch you've made is that it will break existing TIPC applications that adhere to the existing API (i.e. they don't do endianness conversion on the messages they exchange with the topology server) if the application is running on an architecture whose host byte order isn't the same as network byte order. > > In addition, the TIPC specification that he used to justify some of > > his decisions is out-dated, and doesn't reflect the latest > version of > > the TIPC protocol. (I'll discuss this further in a > follow-up email.) > > > Please, point out a more recent spec. The one I referenced > is the most recent publically available spec that I can find. > although again, I'm hard pressed to believe that the spec has > changed to include a requirement to send data in receiver byte order. The TIPC spec you're referencing was created a number of years ago when Ericsson was endeavouring to get TIPC accepted by the IETF. That effort was unsuccessful, and when that happened Ericsson decided not to pursue things further and let the spec become stale. The document was kept on the SourceForge website out of historical interest, and its description was updated to state the spec wasn't a reliable indication of the current state of the code. The TIPC working group has continued development of TIPC by letting the code "do the talking". > > 2) Neil's alteration of the memcpy() in the subscription > cancelation > > routine is simply wrong. The pieces of the data structure that he > > claims are local are not local, and must be checked to > ensure that the > > cancellation is done properly. > > > > The origional memcmp checks sizeof(tipc_subscr) bytes of each > entry in the array. tipc_subscr is defined as: > struct tipc_subscr { > 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 */ > }; > > In subscr_subscribe (from which we also call subscr_cancel), > we allocate new entries for that list, only fillingout the > seq.type, seq.lower, seq.upper, timeout and filter. So if > anyone locally changes usr_handle (which I see several places > that do), the memcmp won't match up properly. Additionally, > I'm hard pressed to believe (and this would be supported by > the section of the spec that I referenced), timeout, and > filter also don't bear on the uniqueness of the subscriber > Id. Of course a subsequent version of the spec might have > changed that, But if it is, please point it out to me. When a subscription request message is received, a new subscription object is created (struct subscription). This routine copies over the sequence, timeout, and filter fields from the original message, and converts them to host byte order so they can be easily referenced later; it doesn't copy over the usr_handle info because that is only meaningful to the application that sent the request. However, in addition, the subscription object also copies the subscription request message (struct tipc_subscr), which contains all of these fields in the sender's byte order, including the usr_handle; this is done by the memcpy near the end of subscr_subscribe(). Now when subscr_cancel() is invoked to try cancelling an existing subscription, the caller passes in the cancellation message structure (struct tipc_subscr) with the cancellation request bit forced off. The routine can then do a memcmp() against the copy of the original subscription request (also struct tipc_subscr), ensuring that a match is declared only if all of the fields specified in the cancellation message match the fields in the original request. Regards, Al -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Allan, I don't think you're being reasonable at all. You tell Neil he's breaking things, and that his code isn't following the current spec. Neil asks where the spec is. And you tell him your out-of-tree code is the spec. That's absolutely rediculious. It is totally unreasonable to require Neil to unravel the mess that is the out-of-tree TIPC implementation in order to figure out what the current protocol "spec" is. You're doing a lot of "oh crap, don't do this, you'll break this or that." But frankly, you really don't care what's in the upstream kernel. If you did, your stuff wouldn't be out of tree. Therefore I take every piece of feedback you give to Neil's patches with a grain of salt, at best. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 08, 2010 at 02:32:38PM -0800, Stephens, Allan wrote: > Neil wrote: > > > > There are a couple of problems with this patch that need to be > > > resolved before it can be applied to the upstream kernel. > > > > > > 1) Neil's replacement of the htohl() call with the equivalent > > > htonl()/ntohl() calls, while well intentioned, was done without > > > understanding why the htohl() calls were put there in the > > first place. > > I'd love to hear what those reasons are. You're formating > > on-the-wire data to an endianness defined by the receiving > > entity. I'm hard pressed to imagine a reason why thats sane. > > When TIPC evolved from using ioctl() as the API to the topology service > to a message-based interface it was felt that it would be desirable to > allow the fields in those messages to be formatted in host-byte order, > since that was what existing applications were already doing. I presume > that the idea was to make the migration process as painless as possible, > and to avoid problems with future applications that didn't take > endianness into account. > So you made a decision to make broken applications continue to work by breaking the protocol stack, and in so doing violated the specification. Thats wrong. The right thing to do is fix the stack to conform to the spec, identify the broken applications, then fix them to conform to the specification. > However, since it was theoretically possible for an application to send > a message to a topology service on another node (whose endianness was > unknown), the node that received the message needed to be able to > recognize if a message contents had the "wrong" endianness and do the > necessary conversion. That is what was implemented back in TIPC 1.5, Thats a big mistake. The right thing to do is to make it clear to the applications that they need to send everything in network byte order, and assume everything receive is in network byte order. Then the application doesn't have to care what the receivers byte order is. Anything less than that is completely broken. Supporting backwards compatibility with broken applications by further breaking things is just more broken. > and this API still exists today which is why the htohl() conversions are > done the way they are. (I can't say I'm a fan of the htohl() name ... > it would be nice to have something more meaningful.) > htohl is perfectly meaningful, its just wrong. > The difficulty with the patch you've made is that it will break existing > TIPC applications that adhere to the existing API (i.e. they don't do > endianness conversion on the messages they exchange with the topology > server) if the application is running on an architecture whose host byte > order isn't the same as network byte order. > Well, thats broken, but the fix isn't to allow them to continue to support munging byte order for them, its to make them fix their own brokenness. You may not be able to break the API for your customers, but upstream can. I won't speak for Dave or anyone else on the list, but for me, convincing me this isn't needed will require a technical argument, not a business one. Show me that this code doesn't violate the latest draft RFC as published. > > > > In addition, the TIPC specification that he used to justify some of > > > his decisions is out-dated, and doesn't reflect the latest > > version of > > > the TIPC protocol. (I'll discuss this further in a > > follow-up email.) > > > > > Please, point out a more recent spec. The one I referenced > > is the most recent publically available spec that I can find. > > although again, I'm hard pressed to believe that the spec has > > changed to include a requirement to send data in receiver byte order. > > The TIPC spec you're referencing was created a number of years ago when > Ericsson was endeavouring to get TIPC accepted by the IETF. That effort > was unsuccessful, and when that happened Ericsson decided not to pursue > things further and let the spec become stale. The document was kept on > the SourceForge website out of historical interest, and its description > was updated to state the spec wasn't a reliable indication of the > current state of the code. The TIPC working group has continued > development of TIPC by letting the code "do the talking". > Setting asside how unacceptable I find that, I find a flaw in your logic here. you assert the code defines the specification, yet from your statements above, we can't change the behavior of the code (read: we can't change the specification, because using applications have already assumed the code behaves the way it currently does). As such, it would seem that we can do little more with tipc than maintain exact bug-for-bug compatibility, in perpituity, and thats unacceptable. Either we: 1) Fix the bugs we find, using the latest published specification 2) Drop tipc from the kernel, since we don't really know how it should work Seriously, if you're tree is the definitive source on how tipc works, drop it from upstream, maintain it yourself, and save others the grief of having to fix anything with it. > > > > 2) Neil's alteration of the memcpy() in the subscription > > cancelation > > > routine is simply wrong. The pieces of the data structure that he > > > claims are local are not local, and must be checked to > > ensure that the > > > cancellation is done properly. > > > > > > > The origional memcmp checks sizeof(tipc_subscr) bytes of each > > entry in the array. tipc_subscr is defined as: > > struct tipc_subscr { > > 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 */ > > }; > > > > In subscr_subscribe (from which we also call subscr_cancel), > > we allocate new entries for that list, only fillingout the > > seq.type, seq.lower, seq.upper, timeout and filter. So if > > anyone locally changes usr_handle (which I see several places > > that do), the memcmp won't match up properly. Additionally, > > I'm hard pressed to believe (and this would be supported by > > the section of the spec that I referenced), timeout, and > > filter also don't bear on the uniqueness of the subscriber > > Id. Of course a subsequent version of the spec might have > > changed that, But if it is, please point it out to me. > > When a subscription request message is received, a new subscription > object is created (struct subscription). This routine copies over the > sequence, timeout, and filter fields from the original message, and > converts them to host byte order so they can be easily referenced later; > it doesn't copy over the usr_handle info because that is only meaningful > to the application that sent the request. However, in addition, the > subscription object also copies the subscription request message (struct > tipc_subscr), which contains all of these fields in the sender's byte > order, including the usr_handle; this is done by the memcpy near the end > of subscr_subscribe(). > > Now when subscr_cancel() is invoked to try cancelling an existing > subscription, the caller passes in the cancellation message structure > (struct tipc_subscr) with the cancellation request bit forced off. The > routine can then do a memcmp() against the copy of the original > subscription request (also struct tipc_subscr), ensuring that a match is > declared only if all of the fields specified in the cancellation message > match the fields in the original request. > Yes, I see that, but where in the spec (section 5.2 IIRC defines the subscriber data), include the timeout, filter and user handle as components of the -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c index ac91f0d..ff123e5 100644 --- a/net/tipc/subscr.c +++ b/net/tipc/subscr.c @@ -76,19 +76,6 @@ 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 @@ -107,11 +94,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 = 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); + 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); tipc_send(sub->server_ref, 1, &msg_sect); } @@ -287,16 +274,23 @@ 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 (!memcmp(s, &sub->evt.s, sizeof(struct tipc_subscr))) { - found = 1; - break; - } + if ((type == sub->seq.type) && + (lower == sub->seq.lower) && + (upper == sub->seq.upper)) { + found = 1; + break; + } } if (!found) return; @@ -325,16 +319,10 @@ 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 (s->filter & htohl(TIPC_SUB_CANCEL, swap)) { - s->filter &= ~htohl(TIPC_SUB_CANCEL, swap); + if (ntohl(s->filter) & TIPC_SUB_CANCEL) { subscr_cancel(s, subscriber); return NULL; } @@ -359,11 +347,11 @@ static struct subscription *subscr_subscribe(struct tipc_subscr *s, /* Initialize subscription object */ - 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); + 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); if ((!(sub->filter & TIPC_SUB_PORTS) == !(sub->filter & TIPC_SUB_SERVICE)) || (sub->seq.lower > sub->seq.upper)) { @@ -376,7 +364,6 @@ 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 45d89bf..c20f496 100644 --- a/net/tipc/subscr.h +++ b/net/tipc/subscr.h @@ -53,7 +53,6 @@ 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 */ @@ -66,7 +65,6 @@ struct subscription { struct list_head nameseq_list; struct list_head subscription_list; u32 server_ref; - int swap; struct tipc_event evt; };
Remove htohl implementation from tipc I was working on forward porting the downstream commits for TIPC and ran accross this one: http://tipc.cslab.ericsson.net/cgi-bin/gitweb.cgi?p=people/allan/tipc.git;a=commitdiff;h=894279b9437b63cbb02405ad5b8e033b51e4e31e I was going to just take it, when I looked closer and noted what it was doing. This is basically a routine to byte swap fields of data in sent/received packets for tipc, dependent upon the receivers guessed endianness of the peer when a connection is established. Asside from just seeming silly to me, it appears to violate the latest RFC draft for tipc: http://tipc.sourceforge.net/doc/draft-spec-tipc-02.txt Which, according to section 4.2 and 4.3.3, requires that all fields of all commands be sent in network byte order. So instead of just taking this patch, instead I'm removing the htohl function and replacing the calls with calls to ntohl in the rx path and htonl in the send path. As part of this fix, I'm also changing the subscr_cancel function, which searches the list of subscribers, using a memcmp of the entire subscriber list, for the entry to tear down. unfortunately it memcmps the entire tipc_subscr structure which has several bits that are private to the local side, so nothing will ever match. section 5.2 of the draft spec indicates the <type,upper,lower> tuple should uniquely identify a subscriber, so convert subscr_cancel to just match on those fields (properly endian swapped). I've tested this using the tipc test suite, and its passed without issue. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: Allan Stephens <allan.stephens@windriver.com> subscr.c | 57 ++++++++++++++++++++++----------------------------------- subscr.h | 2 -- 2 files changed, 22 insertions(+), 37 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html