[PATCH net-next v2 0/6] tipc: refcount and memory leak fixes

[PATCH net-next v2 0/6] tipc: refcount and memory leak fixes

[erik.hugne]

From: Erik Hugne <erik.hugne@ericsson.com>

v2: Drop specific -ENOMEM logging in patch #1 (tipc: allow connection
    shutdown callback to be invoked in advance) And add a general error
    message if an internal server tries to send a message on a
    closed/nonexisting connection.

In addition to the fix for refcount leak and memory leak during
module removal, we also fix a problem where the topology server
listening socket where unexpectedly closed. We also eliminate an
unnecessary context switch during accept()/recvmsg() for nonblocking
sockets.

The following should be applied on stable aswell:
tipc: allow connection shutdown callback to be invoked in advance
tipc: fix connection refcount leak
tipc: drop subscriber connection id invalidation

We'd also like to have the fixes for memleak during module removal
in stable aswell, but unfortunately they won't apply cleanly on the
net tree:
tipc: fix memory leak during module removal
tipc: don't log disabled tasklet handler errors

Erik Hugne (3):
  tipc: drop subscriber connection id invalidation
  tipc: fix memory leak during module removal
  tipc: don't log disabled tasklet handler errors

Ying Xue (3):
  tipc: allow connection shutdown callback to be invoked in advance
  tipc: fix connection refcount leak
  tipc: avoid to unnecessary process switch under non-block mode

 net/tipc/config.c     |  9 ++-------
 net/tipc/handler.c    |  1 -
 net/tipc/name_table.c | 37 ++++++++++++++++++++++++++++++++++---
 net/tipc/server.c     | 19 +++++++++++--------
 net/tipc/socket.c     |  4 ++--
 net/tipc/subscr.c     | 19 ++-----------------
 6 files changed, 51 insertions(+), 38 deletions(-)

-- 
1.8.3.2

[PATCH net-next v2 1/6] tipc: allow connection shutdown callback to be invoked in advance

[erik.hugne]

From: Ying Xue <ying.xue@windriver.com>

Currently connection shutdown callback function is called when
connection instance is released in tipc_conn_kref_release(), and
receiving packets and sending packets are running in different
threads. Even if connection is closed by the thread of receiving
packets, its shutdown callback may not be called immediately as
the connection reference count is non-zero at that moment. So,
although the connection is shut down by the thread of receiving
packets, the thread of sending packets doesn't know it. Before
its shutdown callback is invoked to tell the sending thread its
connection has been closed, the sending thread may deliver
messages by tipc_conn_sendmsg(), this is why the following error
information appears:

"Sending subscription event failed, no memory"

To eliminate it, allow connection shutdown callback function to
be called before connection id is removed in tipc_close_conn(),
which makes the sending thread know the truth in time that its
socket is closed so that it doesn't send message to it. We also
replace the specific error reporting from topology/config service
with a general one if either service tries to send a message on
a nonexisting/closed connection.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
Reviewed-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/config.c |  9 ++-------
 net/tipc/server.c | 13 +++++++------
 net/tipc/subscr.c |  8 ++------
 3 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/net/tipc/config.c b/net/tipc/config.c
index c301a9a..5afe633 100644
--- a/net/tipc/config.c
+++ b/net/tipc/config.c
@@ -376,7 +376,6 @@ static void cfg_conn_msg_event(int conid, struct sockaddr_tipc *addr,
 	struct tipc_cfg_msg_hdr *req_hdr;
 	struct tipc_cfg_msg_hdr *rep_hdr;
 	struct sk_buff *rep_buf;
-	int ret;
 
 	/* Validate configuration message header (ignore invalid message) */
 	req_hdr = (struct tipc_cfg_msg_hdr *)buf;
@@ -398,12 +397,8 @@ static void cfg_conn_msg_event(int conid, struct sockaddr_tipc *addr,
 		memcpy(rep_hdr, req_hdr, sizeof(*rep_hdr));
 		rep_hdr->tcm_len = htonl(rep_buf->len);
 		rep_hdr->tcm_flags &= htons(~TCM_F_REQUEST);
-
-		ret = tipc_conn_sendmsg(&cfgsrv, conid, addr, rep_buf->data,
-					rep_buf->len);
-		if (ret < 0)
-			pr_err("Sending cfg reply message failed, no memory\n");
-
+		tipc_conn_sendmsg(&cfgsrv, conid, addr, rep_buf->data,
+				  rep_buf->len);
 		kfree_skb(rep_buf);
 	}
 }
diff --git a/net/tipc/server.c b/net/tipc/server.c
index b635ca3..91423ca 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -87,7 +87,6 @@ static void tipc_clean_outqueues(struct tipc_conn *con);
 static void tipc_conn_kref_release(struct kref *kref)
 {
 	struct tipc_conn *con = container_of(kref, struct tipc_conn, kref);
-	struct tipc_server *s = con->server;
 
 	if (con->sock) {
 		tipc_sock_release_local(con->sock);
@@ -95,10 +94,6 @@ static void tipc_conn_kref_release(struct kref *kref)
 	}
 
 	tipc_clean_outqueues(con);
-
-	if (con->conid)
-		s->tipc_conn_shutdown(con->conid, con->usr_data);
-
 	kfree(con);
 }
 
@@ -181,6 +176,9 @@ static void tipc_close_conn(struct tipc_conn *con)
 	struct tipc_server *s = con->server;
 
 	if (test_and_clear_bit(CF_CONNECTED, &con->flags)) {
+		if (con->conid)
+			s->tipc_conn_shutdown(con->conid, con->usr_data);
+
 		spin_lock_bh(&s->idr_lock);
 		idr_remove(&s->conn_idr, con->conid);
 		s->idr_in_use--;
@@ -413,8 +411,11 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
 	struct tipc_conn *con;
 
 	con = tipc_conn_lookup(s, conid);
-	if (!con)
+	if (!con) {
+		pr_err("Connection %d not found on server %s\n", conid,
+		       s->name);
 		return -EINVAL;
+	}
 
 	e = tipc_alloc_entry(data, len);
 	if (!e) {
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 7cb0bd5..a6ce3bb 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -96,20 +96,16 @@ static void subscr_send_event(struct tipc_subscription *sub, u32 found_lower,
 {
 	struct tipc_subscriber *subscriber = sub->subscriber;
 	struct kvec msg_sect;
-	int ret;
 
 	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);
-	ret = tipc_conn_sendmsg(&topsrv, subscriber->conid, NULL,
-				msg_sect.iov_base, msg_sect.iov_len);
-	if (ret < 0)
-		pr_err("Sending subscription event failed, no memory\n");
+	tipc_conn_sendmsg(&topsrv, subscriber->conid, NULL, msg_sect.iov_base,
+			  msg_sect.iov_len);
 }
 
 /**
-- 
1.8.3.2

Re: [PATCH net-next v2 1/6] tipc: allow connection shutdown callback to be invoked in advance

[David Miller]

From: <erik.hugne@ericsson.com>
Date: Wed, 5 Mar 2014 08:56:13 +0100

> @@ -413,8 +411,11 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
>  	struct tipc_conn *con;
>  
>  	con = tipc_conn_lookup(s, conid);
> -	if (!con)
> +	if (!con) {
> +		pr_err("Connection %d not found on server %s\n", conid,
> +		       s->name);
>  		return -EINVAL;
> +	}
>  
>  	e = tipc_alloc_entry(data, len);
>  	if (!e) {

You really don't want to add kernel log messages which might be easily
triggerable by the user.  Please remove this.

This is the second such case I've pointed out in this patch set, please
audit the rest of your changes for the same issue before resubmitting
so we don't go into this loop yet another time.

Thank you.

Re: [PATCH net-next v2 1/6] tipc: allow connection shutdown callback to be invoked in advance

[Erik Hugne]

On Wed, Mar 05, 2014 at 11:56:18PM -0500, David Miller wrote:
> From: <erik.hugne@ericsson.com>
> Date: Wed, 5 Mar 2014 08:56:13 +0100
> 
> > @@ -413,8 +411,11 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
> >  	struct tipc_conn *con;
> >  
> >  	con = tipc_conn_lookup(s, conid);
> > -	if (!con)
> > +	if (!con) {
> > +		pr_err("Connection %d not found on server %s\n", conid,
> > +		       s->name);
> >  		return -EINVAL;
> > +	}
> >  
> >  	e = tipc_alloc_entry(data, len);
> >  	if (!e) {
> 
> You really don't want to add kernel log messages which might be easily
> triggerable by the user.  Please remove this.

Ok i'll remove it, but i'd anyway like to mention that this cannot be triggered
by users. This function is only invoked from the TIPC internal services
(subscriber and config) as a response from network events.

//E

Re: [PATCH net-next v2 1/6] tipc: allow connection shutdown callback to be invoked in advance

[Ben Hutchings]

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

On Thu, 2014-03-06 at 10:06 +0100, Erik Hugne wrote:
> On Wed, Mar 05, 2014 at 11:56:18PM -0500, David Miller wrote:
> > From: <erik.hugne@ericsson.com>
> > Date: Wed, 5 Mar 2014 08:56:13 +0100
> > 
> > > @@ -413,8 +411,11 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
> > >  	struct tipc_conn *con;
> > >  
> > >  	con = tipc_conn_lookup(s, conid);
> > > -	if (!con)
> > > +	if (!con) {
> > > +		pr_err("Connection %d not found on server %s\n", conid,
> > > +		       s->name);
> > >  		return -EINVAL;
> > > +	}
> > >  
> > >  	e = tipc_alloc_entry(data, len);
> > >  	if (!e) {
> > 
> > You really don't want to add kernel log messages which might be easily
> > triggerable by the user.  Please remove this.
> 
> Ok i'll remove it, but i'd anyway like to mention that this cannot be triggered
> by users. This function is only invoked from the TIPC internal services
> (subscriber and config) as a response from network events.

If 'network events' are triggered by packets received from remote hosts
or packets sent on behalf of arbitrary local users, then this is just as
bad.  You need to rate-limit such logging, usually by making it
conditional on net_ratelimit().

Ben.

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

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

[PATCH net-next v2 2/6] tipc: fix connection refcount leak

[erik.hugne]

From: Ying Xue <ying.xue@windriver.com>

When tipc_conn_sendmsg() calls tipc_conn_lookup() to query a
connection instance, its reference count value is increased if
it's found. But subsequently if it's found that the connection is
closed, the work of sending message is not queued into its server
send workqueue, and the connection reference count is not decreased.
This will cause a reference count leak. To reproduce this problem,
an application would need to open and closes topology server
connections with high intensity.

We fix this by immediately decrementing the connection reference
count if a send fails due to the connection being closed.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Erik Hugne <erik.hugne@ericsson.com>
Reviewed-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/server.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/tipc/server.c b/net/tipc/server.c
index 91423ca..0eb6301 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -430,10 +430,12 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
 	list_add_tail(&e->list, &con->outqueue);
 	spin_unlock_bh(&con->outqueue_lock);
 
-	if (test_bit(CF_CONNECTED, &con->flags))
+	if (test_bit(CF_CONNECTED, &con->flags)) {
 		if (!queue_work(s->send_wq, &con->swork))
 			conn_put(con);
-
+	} else {
+		conn_put(con);
+	}
 	return 0;
 }
 
-- 
1.8.3.2


------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk

[PATCH net-next v2 3/6] tipc: avoid to unnecessary process switch under non-block mode

[erik.hugne]

From: Ying Xue <ying.xue@windriver.com>

When messages are received via tipc socket under non-block mode,
schedule_timeout() is called in tipc_wait_for_rcvmsg(), that is,
the process of receiving messages will be scheduled once although
timeout value passed to schedule_timeout() is 0. The same issue
exists in accept()/wait_for_accept(). To avoid this unnecessary
process switch, we only call schedule_timeout() if the timeout
value is non-zero.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Reviewed-by: Erik Hugne <erik.hugne@ericsson.com>
Reviewed-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index fb88597..96978fb 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1001,7 +1001,7 @@ static int tipc_wait_for_rcvmsg(struct socket *sock, long timeo)
 
 	for (;;) {
 		prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
-		if (skb_queue_empty(&sk->sk_receive_queue)) {
+		if (timeo && skb_queue_empty(&sk->sk_receive_queue)) {
 			if (sock->state == SS_DISCONNECTING) {
 				err = -ENOTCONN;
 				break;
@@ -1627,7 +1627,7 @@ static int tipc_wait_for_accept(struct socket *sock, long timeo)
 	for (;;) {
 		prepare_to_wait_exclusive(sk_sleep(sk), &wait,
 					  TASK_INTERRUPTIBLE);
-		if (skb_queue_empty(&sk->sk_receive_queue)) {
+		if (timeo && skb_queue_empty(&sk->sk_receive_queue)) {
 			release_sock(sk);
 			timeo = schedule_timeout(timeo);
 			lock_sock(sk);
-- 
1.8.3.2

[PATCH net-next v2 4/6] tipc: drop subscriber connection id invalidation

[erik.hugne]

From: Erik Hugne <erik.hugne@ericsson.com>

When a topology server subscriber is disconnected, the associated
connection id is set to zero. A check vs zero is then done in the
subscription timeout function to see if the subscriber have been
shut down. This is unnecessary, because all subscription timers
will be cancelled when a subscriber terminates. Setting the
connection id to zero is actually harmful because id zero is the
identity of the topology server listening socket, and can cause a
race that leads to this socket being closed instead.

Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Reviewed-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/subscr.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index a6ce3bb..11c9ae0 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -149,14 +149,6 @@ static void subscr_timeout(struct tipc_subscription *sub)
 	/* The spin lock per subscriber is used to protect its members */
 	spin_lock_bh(&subscriber->lock);
 
-	/* Validate if the connection related to the subscriber is
-	 * closed (in case subscriber is terminating)
-	 */
-	if (subscriber->conid == 0) {
-		spin_unlock_bh(&subscriber->lock);
-		return;
-	}
-
 	/* Validate timeout (in case subscription is being cancelled) */
 	if (sub->timeout == TIPC_WAIT_FOREVER) {
 		spin_unlock_bh(&subscriber->lock);
@@ -211,9 +203,6 @@ static void subscr_release(struct tipc_subscriber *subscriber)
 
 	spin_lock_bh(&subscriber->lock);
 
-	/* Invalidate subscriber reference */
-	subscriber->conid = 0;
-
 	/* Destroy any existing subscriptions for subscriber */
 	list_for_each_entry_safe(sub, sub_temp, &subscriber->subscription_list,
 				 subscription_list) {
-- 
1.8.3.2


------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk

[PATCH net-next v2 5/6] tipc: fix memory leak during module removal

[erik.hugne]

From: Erik Hugne <erik.hugne@ericsson.com>

When the TIPC module is removed, the tasklet handler is disabled
before all other subsystems. This will cause lingering publications
in the name table because the node_down tasklets responsible to
clean up publications from an unreachable node will never run.
When the name table is shut down, these publications are detected
and an error message is logged:
tipc: nametbl_stop(): orphaned hash chain detected
This is actually a memory leak, introduced with commit
993b858e37b3120ee76d9957a901cca22312ffaa ("tipc: correct the order
of stopping services at rmmod")

Instead of just logging an error and leaking memory, we free
the orphaned entries during nametable shutdown.

Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
Reviewed-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/name_table.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 92a1533..25c32dd 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -941,20 +941,51 @@ int tipc_nametbl_init(void)
 	return 0;
 }
 
+/**
+ * tipc_purge_publications - remove all publications for a given type
+ *
+ * tipc_nametbl_lock must be held when calling this function
+ */
+static void tipc_purge_publications(struct name_seq *seq)
+{
+	struct publication *publ, *safe;
+	struct sub_seq *sseq;
+	struct name_info *info;
+
+	if (!seq->sseqs) {
+		nameseq_delete_empty(seq);
+		return;
+	}
+	sseq = seq->sseqs;
+	info = sseq->info;
+	list_for_each_entry_safe(publ, safe, &info->zone_list, zone_list) {
+		tipc_nametbl_remove_publ(publ->type, publ->lower, publ->node,
+					 publ->ref, publ->key);
+	}
+}
+
 void tipc_nametbl_stop(void)
 {
 	u32 i;
+	struct name_seq *seq;
+	struct hlist_head *seq_head;
+	struct hlist_node *safe;
 
 	if (!table.types)
 		return;
 
-	/* Verify name table is empty, then release it */
+	/* Verify name table is empty and purge any lingering
+	 * publications, then release the name table
+	 */
 	write_lock_bh(&tipc_nametbl_lock);
 	for (i = 0; i < TIPC_NAMETBL_SIZE; i++) {
 		if (hlist_empty(&table.types[i]))
 			continue;
-		pr_err("nametbl_stop(): orphaned hash chain detected\n");
-		break;
+		seq_head = &table.types[i];
+		hlist_for_each_entry_safe(seq, safe, seq_head, ns_list) {
+			tipc_purge_publications(seq);
+		}
+		continue;
 	}
 	kfree(table.types);
 	table.types = NULL;
-- 
1.8.3.2

[PATCH net-next v2 6/6] tipc: don't log disabled tasklet handler errors

[erik.hugne]

From: Erik Hugne <erik.hugne@ericsson.com>

Failure to schedule a TIPC tasklet with tipc_k_signal because the
tasklet handler is disabled is not an error. It means TIPC is
currently in the process of shutting down. We remove the error
logging in this case.

Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
Reviewed-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/handler.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/tipc/handler.c b/net/tipc/handler.c
index e4bc8a2..1fabf16 100644
--- a/net/tipc/handler.c
+++ b/net/tipc/handler.c
@@ -58,7 +58,6 @@ unsigned int tipc_k_signal(Handler routine, unsigned long argument)
 
 	spin_lock_bh(&qitem_lock);
 	if (!handler_enabled) {
-		pr_err("Signal request ignored by handler\n");
 		spin_unlock_bh(&qitem_lock);
 		return -ENOPROTOOPT;
 	}
-- 
1.8.3.2