| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1330886358-4857-1-git-send-email-abi.dmitryt@gmail.com>
Date: Sun, 4 Mar 2012 19:39:18 +0100
From: Dmitry Tarnyagin <abi.dmitryt@...il.com>
To: "David S. Miller" <davem@...emloft.net>
Cc: netdev@...r.kernel.org,
Sjur Brændeland <sjurbren@...il.com>
Subject: [PATCH] caif: Fix for a race in socket transmit with flow control.
From: Dmitry Tarnyagin <dmitry.tarnyagin@...ricsson.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@...ricsson.com>
Signed-off-by: Dmitry Tarnyagin <dmitry.tarnyagin@...ricsson.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists