arehbein has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bts/+/31533 ) Change subject: common: Have PCU socket connection use osmo_wqueue ...................................................................... common: Have PCU socket connection use osmo_wqueue Fixes memleak in case of connected PCU process being suspended without proper close on socket Related: OS#5774 Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 --- M include/osmo-bts/bts.h M include/osmo-bts/pcuif_proto.h M src/common/pcu_sock.c 3 files changed, 57 insertions(+), 68 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/33/31533/1 diff --git a/include/osmo-bts/bts.h b/include/osmo-bts/bts.h index 0ecec1e..2c36c55 100644 --- a/include/osmo-bts/bts.h +++ b/include/osmo-bts/bts.h @@ -360,6 +360,7 @@ struct { char *sock_path; + int sock_qlength_max; } pcu; /* GSMTAP Um logging (disabled by default) */ diff --git a/include/osmo-bts/pcuif_proto.h b/include/osmo-bts/pcuif_proto.h index 0fece48..7e854ce 100644 --- a/include/osmo-bts/pcuif_proto.h +++ b/include/osmo-bts/pcuif_proto.h @@ -4,7 +4,8 @@ #include <osmocom/gsm/l1sap.h> #include <arpa/inet.h> -#define PCU_SOCK_DEFAULT "/tmp/pcu_bts" +#define PCU_SOCK_DEFAULT "/tmp/pcu_bts" +#define PCU_SOCK_QLENGTH_MAX_DEFAULT 10 #define PCU_IF_VERSION 0x0a #define TXT_MAX_LEN 128 diff --git a/src/common/pcu_sock.c b/src/common/pcu_sock.c index 8f8b3af..1d9fd2e 100644 --- a/src/common/pcu_sock.c +++ b/src/common/pcu_sock.c @@ -30,6 +30,7 @@ #include <osmocom/core/talloc.h> #include <osmocom/core/select.h> #include <osmocom/core/socket.h> +#include <osmocom/core/write_queue.h> #include <osmocom/gsm/gsm23003.h> #include <osmocom/gsm/abis_nm.h> #include <osmo-bts/logging.h> @@ -956,10 +957,11 @@ struct pcu_sock_state { struct gsm_network *net; struct osmo_fd listen_bfd; /* fd for listen socket */ - struct osmo_fd conn_bfd; /* fd for connection to lcr */ - struct llist_head upqueue; /* queue for sending messages */ + struct osmo_wqueue upqueue; /* For sending messages; has fd for conn. to PCU */ }; +static void pcu_sock_close(struct pcu_sock_state *state); + int pcu_sock_send(struct gsm_network *net, struct msgb *msg) { struct pcu_sock_state *state = net->pcu_state; @@ -974,7 +976,7 @@ msgb_free(msg); return -EINVAL; } - conn_bfd = &state->conn_bfd; + conn_bfd = &state->upqueue.bfd; if (conn_bfd->fd <= 0) { if (pcu_prim->msg_type != PCU_IF_MSG_TIME_IND && pcu_prim->msg_type != PCU_IF_MSG_INTERF_IND) @@ -983,15 +985,19 @@ msgb_free(msg); return -EIO; } - msgb_enqueue(&state->upqueue, msg); - osmo_fd_write_enable(conn_bfd); - + if (osmo_wqueue_enqueue(&state->upqueue, msg) == -ENOSPC) { + LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %d messages waiting). Closing connection\n", + state->upqueue.max_length); + pcu_sock_close(state); + } else { + osmo_fd_write_enable(conn_bfd); + } return 0; } static void pcu_sock_close(struct pcu_sock_state *state) { - struct osmo_fd *bfd = &state->conn_bfd; + struct osmo_fd *bfd = &state->upqueue.bfd; struct gsm_bts *bts; struct gsm_bts_trx *trx; unsigned int tn; @@ -1037,11 +1043,7 @@ } } - /* flush the queue */ - while (!llist_empty(&state->upqueue)) { - struct msgb *msg = msgb_dequeue(&state->upqueue); - msgb_free(msg); - } + osmo_wqueue_clear(&state->upqueue); } static int pcu_sock_read(struct osmo_fd *bfd) @@ -1090,45 +1092,30 @@ return -1; } -static int pcu_sock_write(struct osmo_fd *bfd) +static int pcu_sock_write(struct osmo_fd *bfd, struct msgb *m) { struct pcu_sock_state *state = bfd->data; int rc; + struct gsm_pcu_if *pcu_prim; - while (!llist_empty(&state->upqueue)) { - struct msgb *msg, *msg2; - struct gsm_pcu_if *pcu_prim; - - /* peek at the beginning of the queue */ - msg = llist_entry(state->upqueue.next, struct msgb, list); - pcu_prim = (struct gsm_pcu_if *)msg->data; - - osmo_fd_write_disable(bfd); - - /* bug hunter 8-): maybe someone forgot msgb_put(...) ? */ - if (!msgb_length(msg)) { - LOGP(DPCU, LOGL_ERROR, "message type (%d) with ZERO " - "bytes!\n", pcu_prim->msg_type); - goto dontsend; + pcu_prim = (struct gsm_pcu_if *)m->data; + osmo_fd_write_disable(&state->upqueue.bfd); + /* bug hunter 8-): maybe someone forgot msgb_put(...) ? */ + if (!msgb_length(m)) { + LOGP(DPCU, LOGL_ERROR, "message type (%d) with ZERO " + "bytes!\n", pcu_prim->msg_type); + return 0; + } + /* try to send it over the socket */ + rc = write(bfd->fd, msgb_data(m), msgb_length(m)); + if (rc == 0) + goto close; + if (rc < 0) { + if (errno == EAGAIN) { + osmo_fd_write_enable(bfd); + return -EAGAIN; } - - /* try to send it over the socket */ - rc = write(bfd->fd, msgb_data(msg), msgb_length(msg)); - if (rc == 0) - goto close; - if (rc < 0) { - if (errno == EAGAIN) { - osmo_fd_write_enable(bfd); - break; - } - goto close; - } - -dontsend: - /* _after_ we send it, we can deueue */ - msg2 = msgb_dequeue(&state->upqueue); - assert(msg == msg2); - msgb_free(msg); + goto close; } return 0; @@ -1138,26 +1125,11 @@ return -1; } -static int pcu_sock_cb(struct osmo_fd *bfd, unsigned int flags) -{ - int rc = 0; - - if (flags & OSMO_FD_READ) - rc = pcu_sock_read(bfd); - if (rc < 0) - return rc; - - if (flags & OSMO_FD_WRITE) - rc = pcu_sock_write(bfd); - - return rc; -} - /* accept connection coming from PCU */ static int pcu_sock_accept(struct osmo_fd *bfd, unsigned int flags) { struct pcu_sock_state *state = (struct pcu_sock_state *)bfd->data; - struct osmo_fd *conn_bfd = &state->conn_bfd; + struct osmo_fd *conn_bfd = &state->upqueue.bfd; struct sockaddr_un un_addr; socklen_t len; int rc; @@ -1178,7 +1150,7 @@ return 0; } - osmo_fd_setup(conn_bfd, rc, OSMO_FD_READ, pcu_sock_cb, state, 0); + osmo_fd_setup(conn_bfd, rc, OSMO_FD_READ, osmo_wqueue_bfd_cb, state, 0); if (osmo_fd_register(conn_bfd) != 0) { LOGP(DPCU, LOGL_ERROR, "Failed to register new connection " @@ -1206,9 +1178,12 @@ if (!state) return -ENOMEM; - INIT_LLIST_HEAD(&state->upqueue); + osmo_wqueue_init(&state->upqueue, PCU_SOCK_QLENGTH_MAX_DEFAULT); + state->upqueue.read_cb = pcu_sock_read; + state->upqueue.write_cb = pcu_sock_write; + state->upqueue.bfd.fd = -1; + state->net = &bts_gsmnet; - state->conn_bfd.fd = -1; bfd = &state->listen_bfd; @@ -1249,7 +1224,7 @@ return; osmo_signal_unregister_handler(SS_GLOBAL, pcu_if_signal_cb, NULL); - conn_bfd = &state->conn_bfd; + conn_bfd = &state->upqueue.bfd; if (conn_bfd->fd > 0) pcu_sock_close(state); bfd = &state->listen_bfd; @@ -1265,7 +1240,7 @@ if (!state) return false; - if (state->conn_bfd.fd <= 0) + if (state->upqueue.bfd.fd <= 0) return false; return true; } -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31533 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 Gerrit-Change-Number: 31533 Gerrit-PatchSet: 1 Gerrit-Owner: arehbein &lt;arehbein(a)sysmocom.de&gt; Gerrit-MessageType: newchange