pespin submitted this change.

View Change

gtp_endpoint: Avoid deadlocks logging while thread is cancelled

We cannot guarantee that LOGP will not end up calling a syscall which can
be a cancellation point. Since the syscall will be probably called while
having the logging mutex locked, an eventual cancellation of the thread
would leave the logging mutex locked forever, hence making all other
threads deadlock as soon as they try to write anything to the log.

Similar to what's already done in tun_device_thread().

Change-Id: I1e141175440402a7db34f3d246104c6ea38031fb
---
M daemon/gtp_endpoint.c
1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/daemon/gtp_endpoint.c b/daemon/gtp_endpoint.c
index 1ad8c27..7892d4e 100644
--- a/daemon/gtp_endpoint.c
+++ b/daemon/gtp_endpoint.c
@@ -27,6 +27,16 @@
 #define LOGEP(ep, lvl, fmt, args ...) \
 	LOGP(DEP, lvl, "%s: " fmt, (ep)->name, ## args)
 
+/* LOGEP "No Cancel": Use within the pthread which can be pthread_cancel()ed, in
+ * order to avoid exiting with the logging mutex held and causing a deadlock afterwards. */
+#define LOGEP_NC(ep, lvl, fmt, args ...) \
+	do { \
+		int _old_cancelst_unused; \
+		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &_old_cancelst_unused); \
+		LOGEP(ep, lvl, fmt, ## args); \
+		pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &_old_cancelst_unused); \
+	} while (0)
+
 /***********************************************************************
  * GTP Endpoint (UDP socket)
  ***********************************************************************/
@@ -42,24 +52,24 @@
 	uint16_t gtp_len;
 
 	if (nread < sizeof(*gtph)) {
-		LOGEP(ep, LOGL_NOTICE, "Short read: %u < %lu\n", nread, sizeof(*gtph));
+		LOGEP_NC(ep, LOGL_NOTICE, "Short read: %u < %lu\n", nread, sizeof(*gtph));
 		return;
 	}
 	gtph = (struct gtp1_header *)buffer;
 
 	/* check GTP header contents */
 	if ((gtph->flags & 0xf0) != 0x30) {
-		LOGEP(ep, LOGL_NOTICE, "Unexpected GTP Flags: 0x%02x\n", gtph->flags);
+		LOGEP_NC(ep, LOGL_NOTICE, "Unexpected GTP Flags: 0x%02x\n", gtph->flags);
 		return;
 	}
 	if (gtph->type != GTP_TPDU) {
-		LOGEP(ep, LOGL_NOTICE, "Unexpected GTP Message Type: 0x%02x\n", gtph->type);
+		LOGEP_NC(ep, LOGL_NOTICE, "Unexpected GTP Message Type: 0x%02x\n", gtph->type);
 		return;
 	}
 
 	gtp_len = ntohs(gtph->length);
 	if (sizeof(*gtph)+gtp_len > nread) {
-		LOGEP(ep, LOGL_NOTICE, "Short GTP Message: %lu < len=%u\n",
+		LOGEP_NC(ep, LOGL_NOTICE, "Short GTP Message: %lu < len=%u\n",
 			sizeof(*gtph)+gtp_len, nread);
 		return;
 	}
@@ -69,7 +79,7 @@
 	if (gtph->flags & GTP1_F_MASK) {
 		const struct gtp1_exthdr *exthdr = (const struct gtp1_exthdr *)payload;
 		if (gtp_len < 4) {
-			LOGEP(ep, LOGL_NOTICE, "Short GTP Message according to flags 0x%02x: %lu < len=%u\n",
+			LOGEP_NC(ep, LOGL_NOTICE, "Short GTP Message according to flags 0x%02x: %lu < len=%u\n",
 				gtph->flags, sizeof(*gtph) + gtp_len, nread);
 			return;
 		}
@@ -79,13 +89,13 @@
 		while (*it != 0) {
 			unsigned int ext_len;
 			if (gtp_len < 1) {
-				LOGEP(ep, LOGL_NOTICE, "Short GTP Message according to flags 0x%02x: %lu < len=%u\n",
+				LOGEP_NC(ep, LOGL_NOTICE, "Short GTP Message according to flags 0x%02x: %lu < len=%u\n",
 				gtph->flags, sizeof(*gtph) + gtp_len, nread);
 				return;
 			}
 			ext_len = 1 + 1 + it[1] + 1;
 			if (gtp_len < ext_len) {
-				LOGEP(ep, LOGL_NOTICE, "Short GTP Message according to flags 0x%02x: %lu < len=%u\n",
+				LOGEP_NC(ep, LOGL_NOTICE, "Short GTP Message according to flags 0x%02x: %lu < len=%u\n",
 					gtph->flags, sizeof(*gtph) + gtp_len, nread);
 				return;
 			}
@@ -100,7 +110,7 @@
 	t = _gtp_tunnel_find_r(d, teid, ep);
 	if (!t) {
 		pthread_rwlock_unlock(&d->rwlock);
-		LOGEP(ep, LOGL_NOTICE, "Unable to find tunnel for TEID=0x%08x\n", teid);
+		LOGEP_NC(ep, LOGL_NOTICE, "Unable to find tunnel for TEID=0x%08x\n", teid);
 		return;
 	}
 	outfd = t->tun_dev->fd;
@@ -109,12 +119,16 @@
 	/* 3) write to TUN device */
 	rc = write(outfd, payload, gtp_len);
 	if (rc < gtp_len) {
-		LOGEP(ep, LOGL_FATAL, "Error writing to tun device %s\n", strerror(errno));
+		LOGEP_NC(ep, LOGL_FATAL, "Error writing to tun device %s\n", strerror(errno));
 		exit(1);
 	}
 }
 
-/* one thread for reading from each GTP/UDP socket (GTP decapsulation -> tun) */
+/* one thread for reading from each GTP/UDP socket (GTP decapsulation -> tun)
+ * IMPORTANT!: All logging functions in this function block must be called with
+ * PTHREAD_CANCEL_DISABLE set, otherwise the thread could be cancelled while
+ * holding the logging mutex, hence causing deadlock with main (or other)
+ * thread. */
 static void *gtp_endpoint_thread(void *arg)
 {
 	struct gtp_endpoint *ep = (struct gtp_endpoint *)arg;
@@ -130,7 +144,7 @@
 		/* 1) read GTP packet from UDP socket */
 		rc = recvfrom(ep->fd, buffer, sizeof(buffer), 0, (struct sockaddr *)NULL, 0);
 		if (rc < 0) {
-			LOGEP(ep, LOGL_FATAL, "Error reading from UDP socket: %s\n", strerror(errno));
+			LOGEP_NC(ep, LOGL_FATAL, "Error reading from UDP socket: %s\n", strerror(errno));
 			exit(1);
 		}
 		handle_gtp1u(ep, buffer, rc);

To view, visit change 40788. To unsubscribe, or for help writing mail filters, visit settings.