[PATCH] caif: Fix for a race in socket transmit with flow control.

[PATCH] caif: Fix for a race in socket transmit with flow control.

[Dmitry Tarnyagin]

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

There is a race between socket transmit and caif flow stop. The race itself
is not something invalid, but handling of the race in net/caif/caif_socket.c
was not correct. Non-fatal -EAGAIN code coming from cfsrvl_ready() led to
killing of calling process by SIGPIPE.

Patch implements retry of transmit if CAIF channel is not ready. It works both for
blocking and non-blocking cases, and both for seqpacket and stream sockets.

Reviewed-by: Sjur BRENDELAND <sjur.brandeland@stericsson.com>
Signed-off-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
---
 net/caif/caif_dev.c    |    8 +++-
 net/caif/caif_socket.c |   83 +++++++++++++++++++++++++++++-------------------
 net/caif/cfdbgl.c      |    4 ++-
 net/caif/cfdgml.c      |    9 ++++-
 net/caif/cfrfml.c      |   25 ++++++--------
 net/caif/cfsrvl.c      |    6 +---
 net/caif/cfutill.c     |    5 ++-
 net/caif/cfvidl.c      |    6 +++-
 8 files changed, 87 insertions(+), 59 deletions(-)

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index aa6f716..8b5c485 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -224,8 +224,12 @@ noxoff:
 	rcu_read_unlock_bh();
 
 	err = dev_queue_xmit(skb);
-	if (err > 0)
-		err = -EIO;
+	if (unlikely(err > 0)) {
+		if (err == NET_XMIT_DROP)
+			err = -EAGAIN;
+		else
+			err = -EIO;
+	}
 
 	return err;
 }
diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index 5016fa5..3e464ac 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -541,47 +541,55 @@ static int caif_seqpkt_sendmsg(struct kiocb *kiocb, struct socket *sock,
 	ret = -EINVAL;
 	if (unlikely(msg->msg_iov->iov_base == NULL))
 		goto err;
-	noblock = msg->msg_flags & MSG_DONTWAIT;
-
-	timeo = sock_sndtimeo(sk, noblock);
-	timeo = caif_wait_for_flow_on(container_of(sk, struct caifsock, sk),
-				1, timeo, &ret);
-
-	if (ret)
-		goto err;
-	ret = -EPIPE;
-	if (cf_sk->sk.sk_state != CAIF_CONNECTED ||
-		sock_flag(sk, SOCK_DEAD) ||
-		(sk->sk_shutdown & RCV_SHUTDOWN))
-		goto err;
 
 	/* Error if trying to write more than maximum frame size. */
 	ret = -EMSGSIZE;
 	if (len > cf_sk->maxframe && cf_sk->sk.sk_protocol != CAIFPROTO_RFM)
 		goto err;
 
-	buffer_size = len + cf_sk->headroom + cf_sk->tailroom;
+	noblock = msg->msg_flags & MSG_DONTWAIT;
+	timeo = sock_sndtimeo(sk, noblock);
 
-	ret = -ENOMEM;
-	skb = sock_alloc_send_skb(sk, buffer_size, noblock, &ret);
+	do {
+		timeo = caif_wait_for_flow_on(
+				container_of(sk, struct caifsock, sk),
+				1, timeo, &ret);
+		if (ret)
+			goto err;
 
-	if (!skb || skb_tailroom(skb) < buffer_size)
-		goto err;
+		ret = -EPIPE;
+		if (cf_sk->sk.sk_state != CAIF_CONNECTED ||
+			sock_flag(sk, SOCK_DEAD) ||
+			(sk->sk_shutdown & RCV_SHUTDOWN))
+			goto err;
 
-	skb_reserve(skb, cf_sk->headroom);
+		buffer_size = len + cf_sk->headroom + cf_sk->tailroom;
 
-	ret = memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len);
+		ret = -ENOMEM;
+		skb = sock_alloc_send_skb(sk, buffer_size, noblock, &ret);
 
-	if (ret)
-		goto err;
-	ret = transmit_skb(skb, cf_sk, noblock, timeo);
-	if (ret < 0)
+		if (!skb || skb_tailroom(skb) < buffer_size)
+			goto err_skb;
+
+		skb_reserve(skb, cf_sk->headroom);
+
+		ret = memcpy_fromiovecend(skb_put(skb, len),
+				msg->msg_iov, 0, len);
+		if (ret)
+			goto err_skb;
+
+		ret = transmit_skb(skb, cf_sk, noblock, timeo);
+	} while (ret == -EAGAIN);
+
+	if (unlikely(ret < 0))
 		/* skb is already freed */
-		return ret;
+		goto err;
 
 	return len;
-err:
+
+err_skb:
 	kfree_skb(skb);
+err:
 	return ret;
 }
 
@@ -608,10 +616,6 @@ static int caif_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
 		goto out_err;
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
-	timeo = caif_wait_for_flow_on(cf_sk, 1, timeo, &err);
-
-	if (unlikely(sk->sk_shutdown & SEND_SHUTDOWN))
-		goto pipe_err;
 
 	while (sent < len) {
 
@@ -627,6 +631,14 @@ static int caif_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
 		if (size > SKB_MAX_ALLOC)
 			size = SKB_MAX_ALLOC;
 
+		timeo = caif_wait_for_flow_on(cf_sk, 1, timeo, &err);
+		if (err)
+			goto out_err;
+
+		if (unlikely(sk->sk_shutdown & SEND_SHUTDOWN))
+			goto pipe_err;
+
+
 		skb = sock_alloc_send_skb(sk,
 					size + cf_sk->headroom +
 					cf_sk->tailroom,
@@ -645,16 +657,21 @@ static int caif_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
 		 */
 		size = min_t(int, size, skb_tailroom(skb));
 
-		err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
+		err = memcpy_fromiovecend(skb_put(skb, size),
+				msg->msg_iov, sent, size);
 		if (err) {
 			kfree_skb(skb);
 			goto out_err;
 		}
 		err = transmit_skb(skb, cf_sk,
 				msg->msg_flags&MSG_DONTWAIT, timeo);
-		if (err < 0)
+		if (unlikely(err < 0)) {
 			/* skb is already freed */
-			goto pipe_err;
+			if (err == -EAGAIN)
+				continue;
+			else
+				goto pipe_err;
+		}
 
 		sent += size;
 	}
diff --git a/net/caif/cfdbgl.c b/net/caif/cfdbgl.c
index 65d6ef3..2914659 100644
--- a/net/caif/cfdbgl.c
+++ b/net/caif/cfdbgl.c
@@ -41,8 +41,10 @@ static int cfdbgl_transmit(struct cflayer *layr, struct cfpkt *pkt)
 	struct caif_payload_info *info;
 	int ret;
 
-	if (!cfsrvl_ready(service, &ret))
+	if (!cfsrvl_ready(service, &ret)) {
+		cfpkt_destroy(pkt);
 		return ret;
+	}
 
 	/* Add info for MUX-layer to route the packet out */
 	info = cfpkt_info(pkt);
diff --git a/net/caif/cfdgml.c b/net/caif/cfdgml.c
index 0f5ff27..a63f4a5 100644
--- a/net/caif/cfdgml.c
+++ b/net/caif/cfdgml.c
@@ -86,12 +86,17 @@ static int cfdgml_transmit(struct cflayer *layr, struct cfpkt *pkt)
 	struct caif_payload_info *info;
 	struct cfsrvl *service = container_obj(layr);
 	int ret;
-	if (!cfsrvl_ready(service, &ret))
+
+	if (!cfsrvl_ready(service, &ret)) {
+		cfpkt_destroy(pkt);
 		return ret;
+	}
 
 	/* STE Modem cannot handle more than 1500 bytes datagrams */
-	if (cfpkt_getlen(pkt) > DGM_MTU)
+	if (cfpkt_getlen(pkt) > DGM_MTU) {
+		cfpkt_destroy(pkt);
 		return -EMSGSIZE;
+	}
 
 	cfpkt_add_head(pkt, &zero, 3);
 	packet_type = 0x08; /* B9 set - UNCLASSIFIED */
diff --git a/net/caif/cfrfml.c b/net/caif/cfrfml.c
index 6dc75d4..2b563ad 100644
--- a/net/caif/cfrfml.c
+++ b/net/caif/cfrfml.c
@@ -184,6 +184,11 @@ out:
 					rfml->serv.dev_info.id);
 	}
 	spin_unlock(&rfml->sync);
+
+	if (unlikely(err == -EAGAIN))
+		/* It is not possible to recover after drop of a fragment */
+		err = -EIO;
+
 	return err;
 }
 
@@ -218,7 +223,7 @@ static int cfrfml_transmit(struct cflayer *layr, struct cfpkt *pkt)
 	caif_assert(layr->dn->transmit != NULL);
 
 	if (!cfsrvl_ready(&rfml->serv, &err))
-		return err;
+		goto out;
 
 	err = -EPROTO;
 	if (cfpkt_getlen(pkt) <= RFM_HEAD_SIZE-1)
@@ -251,8 +256,11 @@ static int cfrfml_transmit(struct cflayer *layr, struct cfpkt *pkt)
 
 		err = cfrfml_transmit_segment(rfml, frontpkt);
 
-		if (err != 0)
+		if (err != 0) {
+			frontpkt = NULL;
 			goto out;
+		}
+
 		frontpkt = rearpkt;
 		rearpkt = NULL;
 
@@ -286,19 +294,8 @@ out:
 		if (rearpkt)
 			cfpkt_destroy(rearpkt);
 
-		if (frontpkt && frontpkt != pkt) {
-
+		if (frontpkt)
 			cfpkt_destroy(frontpkt);
-			/*
-			 * Socket layer will free the original packet,
-			 * but this packet may already be sent and
-			 * freed. So we have to return 0 in this case
-			 * to avoid socket layer to re-free this packet.
-			 * The return of shutdown indication will
-			 * cause connection to be invalidated anyhow.
-			 */
-			err = 0;
-		}
 	}
 
 	return err;
diff --git a/net/caif/cfsrvl.c b/net/caif/cfsrvl.c
index b99f5b2..4aa33d4 100644
--- a/net/caif/cfsrvl.c
+++ b/net/caif/cfsrvl.c
@@ -174,15 +174,11 @@ void cfsrvl_init(struct cfsrvl *service,
 
 bool cfsrvl_ready(struct cfsrvl *service, int *err)
 {
-	if (service->open && service->modem_flow_on && service->phy_flow_on)
-		return true;
 	if (!service->open) {
 		*err = -ENOTCONN;
 		return false;
 	}
-	caif_assert(!(service->modem_flow_on && service->phy_flow_on));
-	*err = -EAGAIN;
-	return false;
+	return true;
 }
 
 u8 cfsrvl_getphyid(struct cflayer *layer)
diff --git a/net/caif/cfutill.c b/net/caif/cfutill.c
index 53e49f3..86d2dad 100644
--- a/net/caif/cfutill.c
+++ b/net/caif/cfutill.c
@@ -84,8 +84,11 @@ static int cfutill_transmit(struct cflayer *layr, struct cfpkt *pkt)
 	caif_assert(layr != NULL);
 	caif_assert(layr->dn != NULL);
 	caif_assert(layr->dn->transmit != NULL);
-	if (!cfsrvl_ready(service, &ret))
+
+	if (!cfsrvl_ready(service, &ret)) {
+		cfpkt_destroy(pkt);
 		return ret;
+	}
 
 	cfpkt_add_head(pkt, &zero, 1);
 	/* Add info for MUX-layer to route the packet out. */
diff --git a/net/caif/cfvidl.c b/net/caif/cfvidl.c
index e3f37db..a8e2a2d 100644
--- a/net/caif/cfvidl.c
+++ b/net/caif/cfvidl.c
@@ -50,8 +50,12 @@ static int cfvidl_transmit(struct cflayer *layr, struct cfpkt *pkt)
 	struct caif_payload_info *info;
 	u32 videoheader = 0;
 	int ret;
-	if (!cfsrvl_ready(service, &ret))
+
+	if (!cfsrvl_ready(service, &ret)) {
+		cfpkt_destroy(pkt);
 		return ret;
+	}
+
 	cfpkt_add_head(pkt, &videoheader, 4);
 	/* Add info for MUX-layer to route the packet out */
 	info = cfpkt_info(pkt);
-- 
1.7.9

Re: [PATCH] caif: Fix for a race in socket transmit with flow control.

[David Miller]

From: Dmitry Tarnyagin <abi.dmitryt@gmail.com>
Date: Sun,  4 Mar 2012 19:39:18 +0100

> @@ -224,8 +224,12 @@ noxoff:
>  	rcu_read_unlock_bh();
>  
>  	err = dev_queue_xmit(skb);
> -	if (err > 0)
> -		err = -EIO;
> +	if (unlikely(err > 0)) {
> +		if (err == NET_XMIT_DROP)
> +			err = -EAGAIN;
> +		else
> +			err = -EIO;
> +	}
>  
>  	return err;
>  }

If there is a packet scheduler classifier rule saying to drop all
packets that match a certain criteria, this will always return -EAGAIN
for certain SKBs.

> +		ret = transmit_skb(skb, cf_sk, noblock, timeo);
> +	} while (ret == -EAGAIN);

And therefore this will loop forever.

I think this is just a bad way to handle it, drops are drops and they
can happen anywhere not just the place that signals things like
NET_XMIT_DROP.

Just simply ignore positive return values from dev_queue_xmit() and
traslate them into zero.  Then you need to make no other changes at
all.