[PATCH net 1/2] caif: Bugfix list_del_rcu race in cfmuxl_ctrlcmd.

[PATCH net 1/2] caif: Bugfix list_del_rcu race in cfmuxl_ctrlcmd.

[Sjur Brændeland]

Always use cfmuxl_remove_uplayer when removing a up-layer.
cfmuxl_ctrlcmd() can be called independently and in parallel with
cfmuxl_remove_uplayer(). The race between them could cause list_del_rcu
to be called on a node which has been already taken out from the list.
That lead to a (rare) crash on accessing poisoned node->prev inside
list_del_rcu.

This fix ensures that deletion are done holding the same lock.

Reported-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 net/caif/cfmuxl.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/net/caif/cfmuxl.c b/net/caif/cfmuxl.c
index b36f24a..94b0861 100644
--- a/net/caif/cfmuxl.c
+++ b/net/caif/cfmuxl.c
@@ -248,7 +248,6 @@ static void cfmuxl_ctrlcmd(struct cflayer *layr, enum caif_ctrlcmd ctrl,
 {
 	struct cfmuxl *muxl = container_obj(layr);
 	struct cflayer *layer;
-	int idx;
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(layer, &muxl->srvl_list, node) {
@@ -257,14 +256,9 @@ static void cfmuxl_ctrlcmd(struct cflayer *layr, enum caif_ctrlcmd ctrl,
 
 			if ((ctrl == _CAIF_CTRLCMD_PHYIF_DOWN_IND ||
 				ctrl == CAIF_CTRLCMD_REMOTE_SHUTDOWN_IND) &&
-					layer->id != 0) {
-
-				idx = layer->id % UP_CACHE_SIZE;
-				spin_lock_bh(&muxl->receive_lock);
-				RCU_INIT_POINTER(muxl->up_cache[idx], NULL);
-				list_del_rcu(&layer->node);
-				spin_unlock_bh(&muxl->receive_lock);
-			}
+					layer->id != 0)
+				cfmuxl_remove_uplayer(layr, layer->id);
+
 			/* NOTE: ctrlcmd is not allowed to block */
 			layer->ctrlcmd(layer, ctrl, phyid);
 		}
-- 
1.7.5.4

[PATCH net 2/2] caif: Bugfix double kfree_skb upon xmit failure

[Sjur Brændeland]

From: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>

SKB is freed twice upon send error. The Network stack consumes SKB even
when it returns error code.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 net/caif/caif_socket.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index a986280..a97d97a 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -539,8 +539,10 @@ static int transmit_skb(struct sk_buff *skb, struct caifsock *cf_sk,
 	pkt = cfpkt_fromnative(CAIF_DIR_OUT, skb);
 	memset(skb->cb, 0, sizeof(struct caif_payload_info));
 
-	if (cf_sk->layer.dn == NULL)
+	if (cf_sk->layer.dn == NULL) {
+		kfree_skb(skb);
 		return -EINVAL;
+	}
 
 	return cf_sk->layer.dn->transmit(cf_sk->layer.dn, pkt);
 }
@@ -683,10 +685,10 @@ static int caif_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
 		}
 		err = transmit_skb(skb, cf_sk,
 				msg->msg_flags&MSG_DONTWAIT, timeo);
-		if (err < 0) {
-			kfree_skb(skb);
+		if (err < 0)
+			/* skb is already freed */
 			goto pipe_err;
-		}
+
 		sent += size;
 	}
 
-- 
1.7.5.4

Re: [PATCH net 1/2] caif: Bugfix list_del_rcu race in cfmuxl_ctrlcmd.

[David Miller]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Date: Thu,  2 Feb 2012 12:21:02 +0100

> Always use cfmuxl_remove_uplayer when removing a up-layer.
> cfmuxl_ctrlcmd() can be called independently and in parallel with
> cfmuxl_remove_uplayer(). The race between them could cause list_del_rcu
> to be called on a node which has been already taken out from the list.
> That lead to a (rare) crash on accessing poisoned node->prev inside
> list_del_rcu.
> 
> This fix ensures that deletion are done holding the same lock.
> 
> Reported-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Applied.

Re: [PATCH net 2/2] caif: Bugfix double kfree_skb upon xmit failure

[David Miller]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Date: Thu,  2 Feb 2012 12:21:03 +0100

> From: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
> 
> SKB is freed twice upon send error. The Network stack consumes SKB even
> when it returns error code.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Applied.