From 5196cd56415e9fa97d78b92ea1803606205710c4 Mon Sep 17 00:00:00 2001 From: Pau Espin Pedrol Date: Tue, 14 Jul 2020 17:52:09 +0200 Subject: e1_input: Use osmo_use_count in e1inp_line osmo_use_count is available since libosmocore 1.1.0 release, so bump required libosmocore version in autotools and packages. struct e1inp_line field refcnt is kept in order to keep ABI compatibility accessing struct fields. The new use_count is added at the end. Size of struct changing is fine since it is allocated through an API and a pointer should be used by clients. e1inp_line_clone API is changed but it's not used by anyone outside libosmo-abis, so it's fine. Related: OS#4624 Change-Id: I0658b2e9c452598025cc0f1d0b060076171767cc --- TODO-RELEASE | 1 + configure.ac | 6 +-- contrib/libosmo-abis.spec.in | 6 +-- debian/control | 2 +- include/osmocom/abis/e1_input.h | 15 +++++-- src/e1_input.c | 95 +++++++++++++++++++++++++---------------- src/input/ipaccess.c | 14 +++--- 7 files changed, 85 insertions(+), 54 deletions(-) diff --git a/TODO-RELEASE b/TODO-RELEASE index 85b16a7..aa278ec 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -10,3 +10,4 @@ libosmo-abis API change major: add parameter to struct e1inp_line libosmo-trau API change add new function osmo_rtp_socket_set_dscp() libosmo-abis API change major: add parameter to struct lapd_instance +libosmo-abis Field added struct e1inp_line "use_count". REMINDER: Upon LIBVERSION c bump, take the chance to drop struct e1inp_line "refcnt" field. diff --git a/configure.ac b/configure.ac index f095cf5..04ffca6 100644 --- a/configure.ac +++ b/configure.ac @@ -64,9 +64,9 @@ AC_SUBST(SYMBOL_VISIBILITY) dnl Generate the output AM_CONFIG_HEADER(config.h) -PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore >= 1.0.0) -PKG_CHECK_MODULES(LIBOSMOVTY, libosmovty >= 1.0.0) -PKG_CHECK_MODULES(LIBOSMOGSM, libosmogsm >= 1.0.0) +PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore >= 1.1.0) +PKG_CHECK_MODULES(LIBOSMOVTY, libosmovty >= 1.1.0) +PKG_CHECK_MODULES(LIBOSMOGSM, libosmogsm >= 1.1.0) PKG_CHECK_MODULES(ORTP, ortp >= 0.22.0) AC_ARG_ENABLE([dahdi], diff --git a/contrib/libosmo-abis.spec.in b/contrib/libosmo-abis.spec.in index 3ad36b1..da0c2d8 100644 --- a/contrib/libosmo-abis.spec.in +++ b/contrib/libosmo-abis.spec.in @@ -27,9 +27,9 @@ BuildRequires: automake >= 1.6 BuildRequires: libtool >= 2 BuildRequires: pkgconfig >= 0.20 BuildRequires: xz -BuildRequires: pkgconfig(libosmocore) >= 1.0.0 -BuildRequires: pkgconfig(libosmogsm) >= 1.0.0 -BuildRequires: pkgconfig(libosmovty) >= 1.0.0 +BuildRequires: pkgconfig(libosmocore) >= 1.1.0 +BuildRequires: pkgconfig(libosmogsm) >= 1.1.0 +BuildRequires: pkgconfig(libosmovty) >= 1.1.0 BuildRequires: pkgconfig(ortp) >= 0.22 BuildRequires: pkgconfig(talloc) diff --git a/debian/control b/debian/control index b9e2078..7039bfe 100644 --- a/debian/control +++ b/debian/control @@ -11,7 +11,7 @@ Build-Depends: debhelper (>= 9), dh-autoreconf, libdpkg-perl, git, - libosmocore-dev (>= 1.0.0), + libosmocore-dev (>= 1.1.0), pkg-config, libortp-dev Standards-Version: 3.9.7 diff --git a/include/osmocom/abis/e1_input.h b/include/osmocom/abis/e1_input.h index ee33ae6..44708bb 100644 --- a/include/osmocom/abis/e1_input.h +++ b/include/osmocom/abis/e1_input.h @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -191,7 +192,7 @@ struct e1inp_line_ops { struct e1inp_line { struct llist_head list; - int refcnt; + int refcnt; /* unusued, kept for ABI compat, use_count is used instead */ unsigned int num; const char *name; @@ -215,6 +216,8 @@ struct e1inp_line { struct e1inp_driver *driver; void *driver_data; + + struct osmo_use_count use_count; }; #define e1inp_line_ipa_oml_ts(line) (&line->ts[0]) #define e1inp_line_ipa_rsl_ts(line, trx_id) (&line->ts[1 + (trx_id)]) @@ -245,13 +248,17 @@ struct e1inp_line *e1inp_line_find(uint8_t e1_nr); 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); +struct e1inp_line *e1inp_line_clone(void *ctx, struct e1inp_line *line, const char *use); /* increment refcount use of E1 input line */ -void e1inp_line_get(struct e1inp_line *line); +void e1inp_line_get(struct e1inp_line *line) OSMO_DEPRECATED("Use e1inp_line_get2() instead"); /* decrement refcount use of E1 input line, release if unused */ -void e1inp_line_put(struct e1inp_line *line); +void e1inp_line_put(struct e1inp_line *line) OSMO_DEPRECATED("Use e1inp_line_put2() instead"); + +/* Convenience macros for struct foo instances. These are strict about use count errors. */ +#define e1inp_line_get2(line, USE) OSMO_ASSERT( osmo_use_count_get_put(&(line)->use_count, USE, 1) == 0 ); +#define e1inp_line_put2(line, USE) OSMO_ASSERT( osmo_use_count_get_put(&(line)->use_count, USE, -1) == 0 ); /* 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 9ea4f17..fc0370d 100644 --- a/src/e1_input.c +++ b/src/e1_input.c @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -366,6 +367,48 @@ int e1inp_ts_config_hdlc(struct e1inp_ts *ts, struct e1inp_line *line, return 0; } +static int e1inp_line_use_cb(struct osmo_use_count_entry *use_count_entry, int32_t old_use_count, + const char *file, int file_line) +{ + char buf[512]; + struct osmo_use_count *uc = use_count_entry->use_count; + struct e1inp_line *line = uc->talloc_object; + + LOGPSRC(DLINP, LOGL_INFO, file, file_line, + "E1L(%u) Line (%p) reference count %s changed %" PRId32 " -> %" PRId32 " [%s]\n", + (line)->num, line, use_count_entry->use, + old_use_count, use_count_entry->count, + osmo_use_count_name_buf(buf, sizeof(buf), uc)); + + if (!use_count_entry->count) + osmo_use_count_free(use_count_entry); + + if (osmo_use_count_total(uc) > 0) + return 0; + + /* Remove our counter group from libosmocore's global counter + * list if we are freeing the last remaining talloc context. + * Otherwise we get a use-after-free when libosmocore's timer + * ticks again and attempts to update these counters (OS#3011). + * + * Note that talloc internally counts "secondary" references + * _in addition to_ the initial allocation context, so yes, + * we must check for *zero* remaining secondary contexts here. */ + if (talloc_reference_count(line->rate_ctr) == 0) { + rate_ctr_group_free(line->rate_ctr); + } else { + /* We are not freeing the last talloc context. + * Instead of calling talloc_free(), unlink this 'line' pointer + * which serves as one of several talloc contexts for the rate + * counters and driver private state. */ + talloc_unlink(line, line->rate_ctr); + if (line->driver_data) + talloc_unlink(line, line->driver_data); + } + talloc_free(line); + return 0; +} + struct e1inp_line *e1inp_line_find(uint8_t e1_nr) { struct e1inp_line *e1i_line; @@ -417,14 +460,18 @@ e1inp_line_create(uint8_t e1_nr, const char *driver_name) line->ts[i].num = i+1; line->ts[i].line = line; } - line->refcnt++; + + line->use_count.talloc_object = line; + line->use_count.use_cb = e1inp_line_use_cb; + e1inp_line_get2(line, "ctor"); + llist_add_tail(&line->list, &e1inp_line_list); return line; } struct e1inp_line * -e1inp_line_clone(void *ctx, struct e1inp_line *line) +e1inp_line_clone(void *ctx, struct e1inp_line *line, const char *use) { struct e1inp_line *clone; @@ -453,47 +500,23 @@ e1inp_line_clone(void *ctx, struct e1inp_line *line) if (line->driver_data) clone->driver_data = talloc_reference(clone, line->driver_data); - clone->refcnt = 1; + clone->use_count = (struct osmo_use_count) { + .talloc_object = clone, + .use_cb = e1inp_line_use_cb, + .use_counts = {0}, + }; + e1inp_line_get2(clone, use); /* Clone is used internally for bfd */ return clone; } void e1inp_line_get(struct e1inp_line *line) { - int old_refcnt = line->refcnt++; - - LOGPIL(line, DLINP, LOGL_DEBUG, "Line '%s' (%p) reference count get: %d -> %d\n", - line->name, line, old_refcnt, line->refcnt); + e1inp_line_get2(line, "unknown"); } void e1inp_line_put(struct e1inp_line *line) { - int old_refcnt = line->refcnt--; - - LOGPIL(line, DLINP, LOGL_DEBUG, "Line '%s' (%p) reference count put: %d -> %d\n", - line->name, line, old_refcnt, line->refcnt); - - if (line->refcnt == 0) { - /* Remove our counter group from libosmocore's global counter - * list if we are freeing the last remaining talloc context. - * Otherwise we get a use-after-free when libosmocore's timer - * ticks again and attempts to update these counters (OS#3011). - * - * Note that talloc internally counts "secondary" references - * _in addition to_ the initial allocation context, so yes, - * we must check for *zero* remaining secondary contexts here. */ - if (talloc_reference_count(line->rate_ctr) == 0) { - rate_ctr_group_free(line->rate_ctr); - } else { - /* We are not freeing the last talloc context. - * Instead of calling talloc_free(), unlink this 'line' pointer - * which serves as one of several talloc contexts for the rate - * counters and driver private state. */ - talloc_unlink(line, line->rate_ctr); - if (line->driver_data) - talloc_unlink(line, line->driver_data); - } - talloc_free(line); - } + e1inp_line_put2(line, "unknown"); } void @@ -586,7 +609,7 @@ e1inp_sign_link_create(struct e1inp_ts *ts, enum e1inp_sign_type type, link->tei = tei; link->sapi = sapi; - e1inp_line_get(link->ts->line); + e1inp_line_get2(link->ts->line, "e1inp_sign_link"); llist_add_tail(&link->list, &ts->sign.sign_links); @@ -609,7 +632,7 @@ void e1inp_sign_link_destroy(struct e1inp_sign_link *link) if (link->ts->line->driver->close) link->ts->line->driver->close(link); - e1inp_line_put(link->ts->line); + e1inp_line_put2(link->ts->line, "e1inp_sign_link"); talloc_free(link); } diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 917a195..5d17839 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -293,7 +293,7 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, /* Finally, we know which OML link is associated with * this RSL link, attach it to this socket. */ bfd->data = new_line = sign_link->ts->line; - e1inp_line_get(new_line); + e1inp_line_get2(new_line, "ipa_bfd"); ts = e1inp_line_ipa_rsl_ts(new_line, unit_data.trx_id); newbfd = &ts->driver.ipaccess.fd; @@ -310,7 +310,7 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, goto err; } /* now we can release the dummy RSL line. */ - e1inp_line_put(line); + e1inp_line_put2(line, "ipa_bfd"); e1i_ts = ipaccess_line_ts(newbfd, new_line); ipaccess_bsc_keepalive_fsm_alloc(e1i_ts, newbfd, "rsl_bsc_to_bts"); @@ -328,7 +328,7 @@ err: close(bfd->fd); bfd->fd = -1; } - e1inp_line_put(line); + e1inp_line_put2(line, "ipa_bfd"); return -1; } @@ -603,7 +603,7 @@ static int ipaccess_bsc_oml_cb(struct ipa_server_link *link, int fd) struct osmo_fd *bfd; /* clone virtual E1 line for this new OML link. */ - line = e1inp_line_clone(tall_ipa_ctx, link->line); + line = e1inp_line_clone(tall_ipa_ctx, link->line, "ipa_bfd"); if (line == NULL) { LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n"); return -ENOMEM; @@ -642,7 +642,7 @@ err_socket: err_line: close(bfd->fd); bfd->fd = -1; - e1inp_line_put(line); + e1inp_line_put2(line, "ipa_bfd"); return ret; } @@ -655,7 +655,7 @@ static int ipaccess_bsc_rsl_cb(struct ipa_server_link *link, int fd) /* We don't know yet which OML link to associate it with. Thus, we * allocate a temporary E1 line until we have received ID. */ - line = e1inp_line_clone(tall_ipa_ctx, link->line); + line = e1inp_line_clone(tall_ipa_ctx, link->line, "ipa_bfd"); if (line == NULL) { LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n"); return -ENOMEM; @@ -692,7 +692,7 @@ err_socket: err_line: close(bfd->fd); bfd->fd = -1; - e1inp_line_put(line); + e1inp_line_put2(line, "ipa_bfd"); return ret; } -- cgit v1.2.3