e1_input: add new refcounting scheme to avoid leaking E1 lines
This patch 's/e1inp_line_get/e1inp_line_find/g' since we need this
function name for the new refcounting scheme.
Basically, I have added a new function to clone lines that is used
by the ipaccess driver:
struct e1inp_line *e1inp_line_clone(void *ctx, struct e1inp_line *line);
And two functions to bump and decrement the refcount:
void e1inp_line_get(struct e1inp_line *line);
void e1inp_line_put(struct e1inp_line *line);
This is useful to avoid leaking virtual E1 lines in the ipaccess
case, since we have two sockets for OML and RSL respectively, we
have to release the line *once* both sockets have been closed.
Without this patch, there are cases in which we don't know if it's
time to release the virtual E1 line.
This patch also includes a fix to avoid crashing if we open a
connection with OML/RSL port without sending any ID_RESP message
(in that case, we have no chance to set the signal link). I tested
these situations with netcat.
diff --git a/include/osmocom/abis/e1_input.h b/include/osmocom/abis/e1_input.h
index 51c9273..b317ed9 100644
--- a/include/osmocom/abis/e1_input.h
+++ b/include/osmocom/abis/e1_input.h
@@ -166,11 +166,20 @@
int e1inp_line_register(struct e1inp_line *line);
/* get a line by its ID */
-struct e1inp_line *e1inp_line_get(uint8_t e1_nr);
+struct e1inp_line *e1inp_line_find(uint8_t e1_nr);
/* create a line in the E1 input core */
struct e1inp_line *e1inp_line_create(uint8_t e1_nr, const char *driver_name);
+/* clone one existing E1 input line */
+struct e1inp_line *e1inp_line_clone(void *ctx, struct e1inp_line *line);
+
+/* increment refcount use of E1 input line */
+void e1inp_line_get(struct e1inp_line *line);
+
+/* decrement refcount use of E1 input line, release if unused */
+void e1inp_line_put(struct e1inp_line *line);
+
/* bind operations to one E1 input line */
void e1inp_line_bind_ops(struct e1inp_line *line, const struct e1inp_line_ops *ops);
diff --git a/src/e1_input.c b/src/e1_input.c
index 419ec8c..6873f21 100644
--- a/src/e1_input.c
+++ b/src/e1_input.c
@@ -269,7 +269,7 @@
return 0;
}
-struct e1inp_line *e1inp_line_get(uint8_t e1_nr)
+struct e1inp_line *e1inp_line_find(uint8_t e1_nr)
{
struct e1inp_line *e1i_line;
@@ -288,7 +288,7 @@
struct e1inp_line *line;
int i;
- line = e1inp_line_get(e1_nr);
+ line = e1inp_line_find(e1_nr);
if (line) {
LOGP(DINP, LOGL_ERROR, "E1 Line %u already exists\n",
e1_nr);
@@ -313,11 +313,39 @@
line->ts[i].num = i+1;
line->ts[i].line = line;
}
+ line->refcnt++;
llist_add_tail(&line->list, &e1inp_line_list);
return line;
}
+struct e1inp_line *
+e1inp_line_clone(void *ctx, struct e1inp_line *line)
+{
+ struct e1inp_line *clone;
+
+ /* clone virtual E1 line for this new OML link. */
+ clone = talloc_zero(ctx, struct e1inp_line);
+ if (clone == NULL)
+ return NULL;
+
+ memcpy(clone, line, sizeof(struct e1inp_line));
+ clone->refcnt = 1;
+ return clone;
+}
+
+void e1inp_line_get(struct e1inp_line *line)
+{
+ line->refcnt++;
+}
+
+void e1inp_line_put(struct e1inp_line *line)
+{
+ line->refcnt--;
+ if (line->refcnt == 0)
+ talloc_free(line);
+}
+
void
e1inp_line_bind_ops(struct e1inp_line *line, const struct e1inp_line_ops *ops)
{
@@ -325,12 +353,12 @@
}
#if 0
-struct e1inp_line *e1inp_line_get_create(uint8_t e1_nr)
+struct e1inp_line *e1inp_line_find_create(uint8_t e1_nr)
{
struct e1inp_line *line;
int i;
- line = e1inp_line_get(e1_nr);
+ line = e1inp_line_find(e1_nr);
if (line)
return line;
@@ -353,7 +381,7 @@
{
struct e1inp_line *e1i_line;
- e1i_line = e1inp_line_get(e1_nr);
+ e1i_line = e1inp_line_find(e1_nr);
if (!e1i_line)
return NULL;
@@ -549,8 +577,10 @@
struct input_signal_data isd;
int rc;
+ e1inp_line_get(line);
+
/* This line has been already initialized, skip this. */
- if (++line->refcnt > 1)
+ if (line->refcnt > 2)
return 0;
if (line->driver && line->ops && line->driver->line_update) {
diff --git a/src/e1_input_vty.c b/src/e1_input_vty.c
index 55a9951..212a4ac 100644
--- a/src/e1_input_vty.c
+++ b/src/e1_input_vty.c
@@ -47,7 +47,7 @@
struct e1inp_line *line;
int e1_nr = atoi(argv[0]);
- line = e1inp_line_get(e1_nr);
+ line = e1inp_line_find(e1_nr);
if (line) {
vty_out(vty, "%% Line %d already exists%s", e1_nr, VTY_NEWLINE);
return CMD_WARNING;
diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c
index fda4ead..d0a1b4e 100644
--- a/src/input/ipaccess.c
+++ b/src/input/ipaccess.c
@@ -249,9 +249,9 @@
bfd->fd = -1;
ret = -ENOENT;
}
- /* release the virtual E1 line that we cloned for this socket,
- * OML and RSL links should have been closed after sign_link_down. */
- talloc_free(line);
+ /* put the virtual E1 line that we cloned for this socket, if
+ * it becomes unused, it gets released. */
+ e1inp_line_put(line);
return ret;
}
@@ -319,8 +319,9 @@
goto err;
}
} else if (bfd->priv_nr == E1INP_SIGN_RSL) {
- struct e1inp_ts *e1i_ts;
+ struct e1inp_ts *ts;
struct osmo_fd *newbfd;
+ struct e1inp_line *new_line;
sign_link =
line->ops->sign_link_up(&unit_data, line,
@@ -334,17 +335,19 @@
}
/* Finally, we know which OML link is associated with
* this RSL link, attach it to this socket. */
- bfd->data = line = sign_link->ts->line;
- e1i_ts = &line->ts[E1INP_SIGN_RSL+unit_data.trx_id-1];
- newbfd = &e1i_ts->driver.ipaccess.fd;
+ bfd->data = new_line = sign_link->ts->line;
+ e1inp_line_get(new_line);
+ ts = &new_line->ts[E1INP_SIGN_RSL+unit_data.trx_id-1];
+ newbfd = &ts->driver.ipaccess.fd;
/* get rid of our old temporary bfd */
memcpy(newbfd, bfd, sizeof(*newbfd));
newbfd->priv_nr = E1INP_SIGN_RSL + unit_data.trx_id;
osmo_fd_unregister(bfd);
bfd->fd = -1;
- talloc_free(bfd);
osmo_fd_register(newbfd);
+ /* now we can release the dummy RSL line. */
+ e1inp_line_put(line);
}
break;
default:
@@ -357,6 +360,7 @@
osmo_fd_unregister(bfd);
close(bfd->fd);
bfd->fd = -1;
+ e1inp_line_put(line);
return ret;
}
@@ -364,7 +368,7 @@
{
struct e1inp_line *line = bfd->data;
unsigned int ts_nr = bfd->priv_nr;
- struct e1inp_ts *e1i_ts = NULL;
+ struct e1inp_ts *e1i_ts = &line->ts[ts_nr-1];
struct e1inp_sign_link *link;
struct ipaccess_head *hh;
struct msgb *msg;
@@ -382,9 +386,6 @@
}
DEBUGP(DMI, "RX %u: %s\n", ts_nr, osmo_hexdump(msgb_l2(msg), msgb_l2len(msg)));
- /* Now that we're sure that we've got a line for socket. */
- e1i_ts = &line->ts[ts_nr-1];
-
hh = (struct ipaccess_head *) msg->data;
if (hh->proto == IPAC_PROTO_IPACCESS) {
ipaccess_rcvmsg(line, msg, bfd);
@@ -421,6 +422,7 @@
osmo_fd_unregister(bfd);
close(bfd->fd);
bfd->fd = -1;
+ e1inp_line_put(line);
return ret;
}
@@ -566,12 +568,11 @@
struct osmo_fd *bfd;
/* clone virtual E1 line for this new OML link. */
- line = talloc_zero(tall_ipa_ctx, struct e1inp_line);
+ line = e1inp_line_clone(tall_ipa_ctx, link->line);
if (line == NULL) {
LOGP(DINP, LOGL_ERROR, "could not clone E1 line\n");
return -1;
}
- memcpy(line, link->line, sizeof(struct e1inp_line));
/* create virrtual E1 timeslots for signalling */
e1inp_ts_config_sign(&line->ts[E1INP_SIGN_OML-1], line);
@@ -592,6 +593,7 @@
if (ret < 0) {
LOGP(DINP, LOGL_ERROR, "could not register FD\n");
close(bfd->fd);
+ e1inp_line_put(line);
return ret;
}
@@ -603,18 +605,31 @@
static int ipaccess_bsc_rsl_cb(struct ipa_server_link *link, int fd)
{
+ struct e1inp_line *line;
+ struct e1inp_ts *e1i_ts;
struct osmo_fd *bfd;
- int ret;
+ int i, ret;
/* We don't know yet which OML link to associate it with. Thus, we
- * allocate a temporary bfd until we have received ID. */
- bfd = talloc_zero(tall_ipa_ctx, struct osmo_fd);
- if (!bfd)
- return -ENOMEM;
+ * allocate a temporary E1 line until we have received ID. */
+ line = e1inp_line_clone(tall_ipa_ctx, link->line);
+ if (line == NULL) {
+ LOGP(DINP, LOGL_ERROR, "could not clone E1 line\n");
+ return -1;
+ }
+ /* initialize the fds */
+ for (i = 0; i < ARRAY_SIZE(line->ts); ++i)
+ line->ts[i].driver.ipaccess.fd.fd = -1;
+ /* we need this to initialize this in case to avoid crashes in case
+ * that the socket is closed before we've seen an ID_RESP. */
+ e1inp_ts_config_sign(&line->ts[E1INP_SIGN_OML-1], line);
+
+ e1i_ts = &line->ts[E1INP_SIGN_RSL-1];
+
+ bfd = &e1i_ts->driver.ipaccess.fd;
bfd->fd = fd;
- /* This dummy line is replaced once we can attach it to the OML link */
- bfd->data = link->line;
+ bfd->data = line;
bfd->priv_nr = E1INP_SIGN_RSL;
bfd->cb = ipaccess_fd_cb;
bfd->when = BSC_FD_READ;
@@ -622,7 +637,7 @@
if (ret < 0) {
LOGP(DINP, LOGL_ERROR, "could not register FD\n");
close(bfd->fd);
- talloc_free(bfd);
+ e1inp_line_put(line);
return ret;
}
/* Request ID. FIXME: request LOCATION, HW/SW VErsion, Unit Name, Serno */