Index | Thread | Search

From:
Christian Ludwig <christian_ludwig@genua.de>
Subject:
btrace: Only allow one probe of each type
To:
<tech@openbsd.org>
Cc:
Martin Pieuchot <mpi@grenadille.net>
Date:
Tue, 3 Dec 2024 09:10:50 +0100

Download raw body.

Thread
  • Christian Ludwig:

    btrace: Only allow one probe of each type

Hi,

btrace(8) recognizes dt(4) events by their kernel probe number. That
means if a bt(5) script has the same probe multiple times, btrace cannot
tell which rules to execute. It simply executes all of them. That is not
a problem for regular probes. But it is for 'interval' and 'profile'
probes, because the interval is lost.

The following script illustrates that:

 # btrace -e '
 interval:hz:1  { print("1"); }
 interval:hz:10 { print("10"); }
 '
 1
 10
 1
 10
 1
 10
 ^C

This diff disallows to register multiple probes of the same type. That
means the script above fails. This really only affects 'interval' and
'profile' probes. For all other probe types, their rules can be merged
into one block.


 - Christian

---
 regress/usr.sbin/btrace/Makefile      |  2 +-
 regress/usr.sbin/btrace/multiend.bt   |  5 +++
 regress/usr.sbin/btrace/multiend.ok   |  1 +
 regress/usr.sbin/btrace/multiprobe.bt | 11 +----
 regress/usr.sbin/btrace/multiprobe.ok |  3 +-
 sys/dev/dt/dt_dev.c                   |  7 +++
 usr.sbin/btrace/btrace.c              | 82 +++++++++++++++++++++--------------
 7 files changed, 66 insertions(+), 45 deletions(-)

diff --git a/regress/usr.sbin/btrace/Makefile b/regress/usr.sbin/btrace/Makefile
index 10fc07fd0913..5af18e53b181 100644
--- a/regress/usr.sbin/btrace/Makefile
+++ b/regress/usr.sbin/btrace/Makefile
@@ -7,7 +7,7 @@ ALLOWDT!=		sysctl -n kern.allowdt 2>/dev/null
 BT_LANG_SCRIPTS=	arithm beginend beginend-argn boolean comments \
 			delete exit histempty if \
 			map mapclear mapempty mapsyntax mapzero map-unnamed \
-			maxoperand min+max+sum multismts nsecs+var \
+			maxoperand min+max+sum multiend multismts nsecs+var \
 			precedence print printf read-map-after-clear \
 			staticv-empty syntaxerror tuple tupleeval vareval
 
diff --git a/regress/usr.sbin/btrace/multiend.bt b/regress/usr.sbin/btrace/multiend.bt
new file mode 100644
index 000000000000..a18395e23c14
--- /dev/null
+++ b/regress/usr.sbin/btrace/multiend.bt
@@ -0,0 +1,5 @@
+END
+{ }
+
+END
+{ }
diff --git a/regress/usr.sbin/btrace/multiend.ok b/regress/usr.sbin/btrace/multiend.ok
new file mode 100644
index 000000000000..246e6a822256
--- /dev/null
+++ b/regress/usr.sbin/btrace/multiend.ok
@@ -0,0 +1 @@
+btrace: Cannot register multiple probes of the same type: 'END'
diff --git a/regress/usr.sbin/btrace/multiprobe.bt b/regress/usr.sbin/btrace/multiprobe.bt
index fbee65916c10..efe0137f48c7 100644
--- a/regress/usr.sbin/btrace/multiprobe.bt
+++ b/regress/usr.sbin/btrace/multiprobe.bt
@@ -1,10 +1,3 @@
-BEGIN,
-interval:hz:50
-{
-	printf("multi\n");
-}
-
+interval:hz:50,
 interval:hz:100
-{
-	exit();
-}
+{ }
diff --git a/regress/usr.sbin/btrace/multiprobe.ok b/regress/usr.sbin/btrace/multiprobe.ok
index 9b34aa063164..5ccc865dc52f 100644
--- a/regress/usr.sbin/btrace/multiprobe.ok
+++ b/regress/usr.sbin/btrace/multiprobe.ok
@@ -1,2 +1 @@
-multi
-multi
+btrace: Cannot register multiple probes of the same type: 'interval:hz:100'
diff --git a/sys/dev/dt/dt_dev.c b/sys/dev/dt/dt_dev.c
index 0d9e72822642..c077b6fb0f36 100644
--- a/sys/dev/dt/dt_dev.c
+++ b/sys/dev/dt/dt_dev.c
@@ -589,6 +589,7 @@ dt_ioctl_probe_enable(struct dt_softc *sc, struct dtioc_req *dtrq)
 {
 	struct dt_pcb_list plist;
 	struct dt_probe *dtp;
+	struct dt_pcb *dp;
 	int error;
 
 	SIMPLEQ_FOREACH(dtp, &dt_probe_list, dtp_next) {
@@ -598,6 +599,12 @@ dt_ioctl_probe_enable(struct dt_softc *sc, struct dtioc_req *dtrq)
 	if (dtp == NULL)
 		return ENOENT;
 
+	/* Only allow one probe of each type. */
+	TAILQ_FOREACH(dp, &sc->ds_pcbs, dp_snext) {
+		if (dp->dp_dtp->dtp_pbn == dtrq->dtrq_pbn)
+			return EEXIST;
+	}
+
 	TAILQ_INIT(&plist);
 	error = dtp->dtp_prov->dtpv_alloc(dtp, sc, &plist, dtrq);
 	if (error)
diff --git a/usr.sbin/btrace/btrace.c b/usr.sbin/btrace/btrace.c
index e755431be8c8..085b8a3f8db4 100644
--- a/usr.sbin/btrace/btrace.c
+++ b/usr.sbin/btrace/btrace.c
@@ -69,6 +69,8 @@ struct dtioc_probe_info	*dtpi_get_by_value(const char *, const char *,
 /*
  * Main loop and rule evaluation.
  */
+void			 probe_bail(struct bt_probe *);
+const char		*probe_name(struct bt_probe *);
 void			 rules_do(int);
 int			 rules_setup(int);
 int			 rules_apply(int, struct dt_evt *);
@@ -106,7 +108,6 @@ int			 ba2dtflags(struct bt_arg *);
 __dead void		 xabort(const char *, ...);
 void			 debug(const char *, ...);
 void			 debugx(const char *, ...);
-const char		*debug_probe_name(struct bt_probe *);
 void			 debug_dump_term(struct bt_arg *);
 void			 debug_dump_expr(struct bt_arg *);
 void			 debug_dump_filter(struct bt_rule *);
@@ -399,6 +400,37 @@ dtpi_get_by_value(const char *prov, const char *func, const char *name)
 	return NULL;
 }
 
+void
+probe_bail(struct bt_probe *bp)
+{
+	errx(1, "Cannot register multiple probes of the same type: '%s'",
+	    probe_name(bp));
+}
+
+const char *
+probe_name(struct bt_probe *bp)
+{
+	static char buf[64];
+
+	if (bp->bp_type == B_PT_BEGIN)
+		return "BEGIN";
+
+	if (bp->bp_type == B_PT_END)
+		return "END";
+
+	assert(bp->bp_type == B_PT_PROBE);
+
+	if (bp->bp_rate) {
+		snprintf(buf, sizeof(buf), "%s:%s:%u", bp->bp_prov,
+		    bp->bp_unit, bp->bp_rate);
+	} else {
+		snprintf(buf, sizeof(buf), "%s:%s:%s", bp->bp_prov,
+		    bp->bp_unit, bp->bp_name);
+	}
+
+	return buf;
+}
+
 void
 rules_do(int fd)
 {
@@ -497,7 +529,7 @@ rules_setup(int fd)
 {
 	struct dtioc_probe_info *dtpi;
 	struct dtioc_req *dtrq;
-	struct bt_rule *r, *rbegin = NULL;
+	struct bt_rule *r, *rbegin = NULL, *rend = NULL;
 	struct bt_probe *bp;
 	struct bt_stmt *bs;
 	struct bt_arg *ba;
@@ -519,12 +551,20 @@ rules_setup(int fd)
 		evtflags |= rules_action_scan(SLIST_FIRST(&r->br_action));
 
 		SLIST_FOREACH(bp, &r->br_probes, bp_next) {
-			debug("parsed probe '%s'", debug_probe_name(bp));
+			debug("parsed probe '%s'", probe_name(bp));
 			debug_dump_filter(r);
 
 			if (bp->bp_type != B_PT_PROBE) {
-				if (bp->bp_type == B_PT_BEGIN)
+				if (bp->bp_type == B_PT_BEGIN) {
+					if (rbegin != NULL)
+						probe_bail(bp);
 					rbegin = r;
+				}
+				if (bp->bp_type == B_PT_END) {
+					if (rend != NULL)
+						probe_bail(bp);
+					rend = r;
+				}
 				continue;
 			}
 
@@ -570,8 +610,11 @@ rules_setup(int fd)
 				continue;
 
 			dtrq = bp->bp_cookie;
-			if (ioctl(fd, DTIOCPRBENABLE, dtrq))
+			if (ioctl(fd, DTIOCPRBENABLE, dtrq)) {
+				if (errno == EEXIST)
+					probe_bail(bp);
 				err(1, "DTIOCPRBENABLE");
+			}
 		}
 	}
 
@@ -662,7 +705,7 @@ rule_eval(struct bt_rule *r, struct dt_evt *dtev)
 	struct bt_probe *bp;
 
 	SLIST_FOREACH(bp, &r->br_probes, bp_next) {
-		debug("eval rule '%s'", debug_probe_name(bp));
+		debug("eval rule '%s'", probe_name(bp));
 		debug_dump_filter(r);
 	}
 
@@ -2041,33 +2084,6 @@ debug_dump_filter(struct bt_rule *r)
 	debugx("/\n");
 }
 
-const char *
-debug_probe_name(struct bt_probe *bp)
-{
-	static char buf[64];
-
-	if (verbose < 2)
-		return "";
-
-	if (bp->bp_type == B_PT_BEGIN)
-		return "BEGIN";
-
-	if (bp->bp_type == B_PT_END)
-		return "END";
-
-	assert(bp->bp_type == B_PT_PROBE);
-
-	if (bp->bp_rate) {
-		snprintf(buf, sizeof(buf), "%s:%s:%u", bp->bp_prov,
-		    bp->bp_unit, bp->bp_rate);
-	} else {
-		snprintf(buf, sizeof(buf), "%s:%s:%s", bp->bp_prov,
-		    bp->bp_unit, bp->bp_name);
-	}
-
-	return buf;
-}
-
 unsigned long
 dt_get_offset(pid_t pid)
 {