Change in libosmo-abis[master]: e1_input: Use osmo_use_count in e1inp_line

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Tue Jul 14 19:52:36 UTC 2020


pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-abis/+/19256 )


Change subject: e1_input: Use osmo_use_count in e1inp_line
......................................................................

e1_input: Use osmo_use_count in e1inp_line

struct e1inp_line fields 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
---
M include/osmocom/abis/e1_input.h
M src/e1_input.c
M src/input/ipaccess.c
3 files changed, 76 insertions(+), 48 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmo-abis refs/changes/56/19256/1

diff --git a/include/osmocom/abis/e1_input.h b/include/osmocom/abis/e1_input.h
index 8230e44..1b98cc7 100644
--- a/include/osmocom/abis/e1_input.h
+++ b/include/osmocom/abis/e1_input.h
@@ -5,6 +5,7 @@
 #include <netinet/in.h>
 
 #include <osmocom/core/linuxlist.h>
+#include <osmocom/core/use_count.h>
 #include <osmocom/core/timer.h>
 #include <osmocom/core/msgb.h>
 #include <osmocom/core/select.h>
@@ -49,7 +50,7 @@
 
 	enum e1inp_sign_type type;
 
-	/* trx for msg->trx of received msgs */	
+	/* trx for msg->trx of received msgs */
 	struct gsm_bts_trx *trx;
 
 	/* msgb queue of to-be-transmitted msgs */
@@ -191,7 +192,7 @@
 
 struct e1inp_line {
 	struct llist_head list;
-	int refcnt;
+	struct osmo_use_count use_count;
 
 	unsigned int num;
 	const char *name;
@@ -245,14 +246,18 @@
 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 */
+/* increment refcount use of E1 input line. Deprecated, use e1inp_line_get2() instead. */
 void e1inp_line_get(struct e1inp_line *line);
 
-/* decrement refcount use of E1 input line, release if unused */
+/* decrement refcount use of E1 input line, release if unused. Deprecated, use e1inp_line_put2() instead. */
 void e1inp_line_put(struct e1inp_line *line);
 
+/* 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..32ffec6 100644
--- a/src/e1_input.c
+++ b/src/e1_input.c
@@ -26,6 +26,7 @@
 
 #include <stdio.h>
 #include <unistd.h>
+#include <inttypes.h>
 #include <stdlib.h>
 #include <errno.h>
 #include <string.h>
@@ -366,6 +367,48 @@
 	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 @@
 		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 @@
 	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 @@
 	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 @@
 	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 @@
 			/* 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 @@
 				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 @@
 		close(bfd->fd);
 		bfd->fd = -1;
 	}
-	e1inp_line_put(line);
+	e1inp_line_put2(line, "ipa_bfd");
 	return -1;
 }
 
@@ -603,7 +603,7 @@
 	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_line:
 	close(bfd->fd);
 	bfd->fd = -1;
-	e1inp_line_put(line);
+	e1inp_line_put2(line, "ipa_bfd");
 	return ret;
 }
 
@@ -655,7 +655,7 @@
 
         /* 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_line:
 	close(bfd->fd);
 	bfd->fd = -1;
-	e1inp_line_put(line);
+	e1inp_line_put2(line, "ipa_bfd");
 	return ret;
 }
 

-- 
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/19256
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: I0658b2e9c452598025cc0f1d0b060076171767cc
Gerrit-Change-Number: 19256
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200714/84980ad7/attachment.htm>


More information about the gerrit-log mailing list