Index | Thread | Search

From:
Martijn van Duren <openbsd+tech@list.imperialat.at>
Subject:
Re: snmpd: move smi_getticks to appl_sysuptime
To:
tech@openbsd.org
Date:
Sun, 9 Nov 2025 09:10:32 +0100

Download raw body.

Thread
ping

On 10/28/25 9:11 AM, Martijn van Duren wrote:
> ping
> 
> On Wed, 2025-10-08 at 12:27 +0200, Martijn van Duren wrote:
>> ping
>>
>> On Mon, 2024-03-18 at 18:22 +0100, Martijn van Duren wrote:
>>> smi_getticks is snmpd's current way of supplying the sysUpTime. SMI
>>> doesn't define the sysUpTime (that would be SNMPv2-MIB), so this
>>> placing is wrong. Another issue with it is that it uses gettimeofday,
>>> where CLOCK_MONOTONIC would be a better fit.
>>>
>>> The definition of sysUpTime is:
>>>                "The time (in hundredths of a second) since the
>>>                network management portion of the system was last
>>>                re-initialized."
>>> Where I wasn't able to find a clear definition of "network management
>>> portion". However, the AgentX RFC2741 states in section 6.2.16:
>>>        res.sysUpTime
>>>
>>>             This field contains the current value of sysUpTime for the
>>>             context indicated within the PDU to which this PDU is a
>>>             response.
>>> Implying that the sysUpTime is at the very least on a per context basis.
>>> For this reason I went with placing the information in appl_context.
>>>
>>> Because of this the start time should be set when the context is
>>> created, implying that the context should be created during appl_init,
>>> instead of relying on a random query creating it (either AgentX setup,
>>> or an SNMP packet).
>>>
>>> As for the 'struct appl_context *' vs 'const char *', I've stuck
>>> with 'const char *' for consistency and keep things a bit more in
>>> line with the SNMP/AgentX protocol point of view.
>>>
>>> While here fix the types of sysORLastChange and sysORUpTime.
>>>
>>> OK?
>>>
>>> martijn@

diff e187005a676727d547bd9d1ddb5f1fc0f99df471 c11d3fa04dc651329001615fdd8ee9886239e54a
commit - e187005a676727d547bd9d1ddb5f1fc0f99df471
commit + c11d3fa04dc651329001615fdd8ee9886239e54a
blob - fdc7e42a707112fead524d974bcf3d262029eda1
blob + 3da88e17c2e299b5b374b4ebe6438da6ef1a91ab
--- usr.sbin/snmpd/application.c
+++ usr.sbin/snmpd/application.c
@@ -28,6 +28,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
+#include <time.h>
 
 #include "application.h"
 #include "log.h"
@@ -48,7 +49,7 @@ struct appl_agentcap {
 	uint32_t aa_index;
 	struct ber_oid aa_oid;
 	char aa_descr[256];
-	int aa_uptime;
+	uint32_t aa_uptime;
 
 	TAILQ_ENTRY(appl_agentcap) aa_entry;
 };
@@ -57,10 +58,13 @@ struct appl_context {
 	char ac_name[APPL_CONTEXTNAME_MAX + 1];
 
 	RB_HEAD(appl_regions, appl_region) ac_regions;
+
 	TAILQ_HEAD(, appl_agentcap) ac_agentcaps;
 	int ac_agentcap_lastid;
-	int ac_agentcap_lastchange;
+	uint32_t ac_agentcap_lastchange;
 
+	struct timespec ac_starttime;
+
 	TAILQ_ENTRY(appl_context) ac_entries;
 };
 
@@ -179,6 +183,8 @@ appl(void)
 void
 appl_init(void)
 {
+	if (appl_context(NULL, 1) == NULL)
+		fatal("Failed to create default context");
 	appl_blocklist_init();
 	appl_internal_init();
 	appl_agentx_init();
@@ -220,7 +226,7 @@ appl_context(const char *name, int create)
 	}
 
 	/* Always allow the default namespace */
-	if (!create && name[0] != '\0') {
+	if (!create) {
 		errno = ENOENT;
 		return NULL;
 	}
@@ -235,6 +241,9 @@ appl_context(const char *name, int create)
 	ctx->ac_agentcap_lastchange = 0;
 
 	TAILQ_INSERT_TAIL(&contexts, ctx, ac_entries);
+
+	if (clock_gettime(CLOCK_MONOTONIC, &ctx->ac_starttime) == -1)
+		fatal("clock_gettime");
 	return ctx;
 }
 
@@ -271,7 +280,7 @@ appl_addagentcaps(const char *ctxname, struct ber_oid 
 	cap->aa_context = ctx;
 	cap->aa_index = ++ctx->ac_agentcap_lastid;
 	cap->aa_oid = *oid;
-	cap->aa_uptime = smi_getticks();
+	cap->aa_uptime = appl_sysuptime(ctxname);
 	if (strlcpy(cap->aa_descr, descr,
 	    sizeof(cap->aa_descr)) >= sizeof(cap->aa_descr)) {
 		log_info("%s: Can't add agent capabilities %s: "
@@ -331,10 +340,27 @@ void
 appl_agentcap_free(struct appl_agentcap *cap)
 {
 	TAILQ_REMOVE(&(cap->aa_context->ac_agentcaps), cap, aa_entry);
-	cap->aa_context->ac_agentcap_lastchange = smi_getticks();
+	cap->aa_context->ac_agentcap_lastchange =
+	    appl_sysuptime(cap->aa_context->ac_name);
 	free(cap);
 }
 
+uint32_t
+appl_sysuptime(const char *ctxname)
+{
+	struct appl_context *ctx;
+	struct timespec now, uptime;
+
+	if ((ctx = appl_context(ctxname, 0)) == NULL)
+		return 0;
+
+	if (clock_gettime(CLOCK_MONOTONIC, &now) == -1)
+		fatal("clock_gettime");
+
+	timespecsub(&now, &ctx->ac_starttime, &uptime);
+	return uptime.tv_sec * 100 + uptime.tv_nsec / 10000000;
+}
+
 struct ber_element *
 appl_sysorlastchange(struct ber_oid *oid)
 {
blob - 5c35942137a00b8de9ea34f1f9ae5f6b9651207f
blob + 884dc530020f25e1d03d7df382b7485921a7fdee
--- usr.sbin/snmpd/application.h
+++ usr.sbin/snmpd/application.h
@@ -123,6 +123,7 @@ void appl(void);
 void appl_init(void);
 void appl_shutdown(void);
 struct appl_context *appl_context(const char *, int);
+uint32_t appl_sysuptime(const char *);
 enum appl_error appl_addagentcaps(const char *, struct ber_oid *, const char *,
     struct appl_backend *);
 enum appl_error appl_removeagentcaps(const char *, struct ber_oid *,
blob - cf6c628e9b5e6c53014ea8aa3fd8796ad3ab245e
blob + 0930824f1db50d27be2b5b2faba0768ce8701deb
--- usr.sbin/snmpd/application_agentx.c
+++ usr.sbin/snmpd/application_agentx.c
@@ -423,7 +423,8 @@ appl_agentx_recv(int fd, short event, void *cookie)
 	case AX_PDU_TYPE_PING:
 		ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
 		    pdu->ap_header.aph_transactionid,
-		    pdu->ap_header.aph_packetid, smi_getticks(),
+		    pdu->ap_header.aph_packetid,
+		    appl_sysuptime(pdu->ap_context.aos_string),
 		    APPL_ERROR_NOERROR, 0, NULL, 0);
 		event_add(&(conn->conn_wev), NULL);
 		break;
@@ -433,7 +434,8 @@ appl_agentx_recv(int fd, short event, void *cookie)
 		    ax_pdutype2string(pdu->ap_header.aph_type));
 		ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
 		    pdu->ap_header.aph_transactionid,
-		    pdu->ap_header.aph_packetid, smi_getticks(),
+		    pdu->ap_header.aph_packetid,
+		    appl_sysuptime(pdu->ap_context.aos_string),
 		    APPL_ERROR_PROCESSINGERROR, 1,
 		    pdu->ap_payload.ap_vbl.ap_varbind,
 		    pdu->ap_payload.ap_vbl.ap_nvarbind);
@@ -455,8 +457,8 @@ appl_agentx_recv(int fd, short event, void *cookie)
  fail:
 	ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
 	    pdu->ap_header.aph_transactionid,
-	    pdu->ap_header.aph_packetid, smi_getticks(),
-	    error, 0, NULL, 0);
+	    pdu->ap_header.aph_packetid,
+	    appl_sysuptime(pdu->ap_context.aos_string), error, 0, NULL, 0);
 	event_add(&(conn->conn_wev), NULL);
 	ax_pdu_free(pdu);
 
@@ -565,13 +567,14 @@ appl_agentx_open(struct appl_agentx_connection *conn, 
 
 	ax_response(conn->conn_ax, session->sess_id,
 	    pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
-	    smi_getticks(), APPL_ERROR_NOERROR, 0, NULL, 0);
+	    appl_sysuptime(NULL), APPL_ERROR_NOERROR, 0, NULL, 0);
 	event_add(&(conn->conn_wev), NULL);
 
 	return;
  fail:
 	ax_response(conn->conn_ax, 0, pdu->ap_header.aph_transactionid,
-	    pdu->ap_header.aph_packetid, 0, error, 0, NULL, 0);
+	    pdu->ap_header.aph_packetid, appl_sysuptime(NULL),
+	    error, 0, NULL, 0);
 	event_add(&(conn->conn_wev), NULL);
 	if (session != NULL)
 		free(session->sess_descr.aos_string);
@@ -597,7 +600,7 @@ appl_agentx_close(struct appl_agentx_session *session,
 
 	ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
 	    pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
-	    smi_getticks(), error, 0, NULL, 0);
+	    appl_sysuptime(NULL), error, 0, NULL, 0);
 	event_add(&(conn->conn_wev), NULL);
 	if (error == APPL_ERROR_NOERROR)
 		return;
@@ -676,7 +679,7 @@ appl_agentx_register(struct appl_agentx_session *sessi
  fail:
 	ax_response(session->sess_conn->conn_ax, session->sess_id,
 	    pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
-	    smi_getticks(), error, 0, NULL, 0);
+	    appl_sysuptime(pdu->ap_context.aos_string), error, 0, NULL, 0);
 	event_add(&(session->sess_conn->conn_wev), NULL);
 }
 
@@ -703,7 +706,7 @@ appl_agentx_unregister(struct appl_agentx_session *ses
  fail:
 	ax_response(session->sess_conn->conn_ax, session->sess_id,
 	    pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
-	    smi_getticks(), error, 0, NULL, 0);
+	    appl_sysuptime(pdu->ap_context.aos_string), error, 0, NULL, 0);
 	event_add(&(session->sess_conn->conn_wev), NULL);
 }
 
@@ -835,7 +838,7 @@ appl_agentx_addagentcaps(struct appl_agentx_session *s
  fail:
 	ax_response(session->sess_conn->conn_ax, session->sess_id,
 	    pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
-	    smi_getticks(), error, 0, NULL, 0);
+	    appl_sysuptime(pdu->ap_context.aos_string), error, 0, NULL, 0);
 	event_add(&(session->sess_conn->conn_wev), NULL);
 }
 
@@ -860,7 +863,7 @@ appl_agentx_removeagentcaps(struct appl_agentx_session
  fail:
 	ax_response(session->sess_conn->conn_ax, session->sess_id,
 	    pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
-	    smi_getticks(), error, 0, NULL, 0);
+	    appl_sysuptime(pdu->ap_context.aos_string), error, 0, NULL, 0);
 	event_add(&(session->sess_conn->conn_wev), NULL);
 }
 
blob - 95032aea33b2fcd931b317151a73c8d5780a82d2
blob + 22b890324bfb16a0977624bc5ca1f86cad6653ce
--- usr.sbin/snmpd/application_internal.c
+++ usr.sbin/snmpd/application_internal.c
@@ -577,7 +577,7 @@ appl_internal_system(struct ber_oid *oid)
 	else if (ober_oid_cmp(&OID(MIB_sysOID, 0), oid) == 0)
 		return ober_add_oid(NULL, &s->sys_oid);
 	else if (ober_oid_cmp(&OID(MIB_sysUpTime, 0), oid) == 0) {
-		value = ober_add_integer(NULL, smi_getticks());
+		value = ober_add_integer(NULL, appl_sysuptime(NULL));
 		ober_set_header(value, BER_CLASS_APPLICATION, SNMP_T_TIMETICKS);
 	} else if (ober_oid_cmp(&OID(MIB_sysContact, 0), oid) == 0)
 		return ober_add_string(NULL, s->sys_contact);
blob - 09a34fafd0b4d8262cce2fded935589079e6ec6b
blob + 88ef067c4012494d0cf169a2aff723c03a6ada68
--- usr.sbin/snmpd/smi.c
+++ usr.sbin/snmpd/smi.c
@@ -62,23 +62,6 @@ RB_HEAD(keytree, oid);
 RB_PROTOTYPE(keytree, oid, o_keyword, smi_key_cmp);
 struct keytree smi_keytree;
 
-u_long
-smi_getticks(void)
-{
-	struct timeval	 now, run;
-	u_long		 ticks;
-
-	gettimeofday(&now, NULL);
-	if (timercmp(&now, &snmpd_env->sc_starttime, <=))
-		return (0);
-	timersub(&now, &snmpd_env->sc_starttime, &run);
-	ticks = run.tv_sec * 100;
-	if (run.tv_usec)
-		ticks += run.tv_usec / 10000;
-
-	return (ticks);
-}
-
 char *
 smi_oid2string(struct ber_oid *o, char *buf, size_t len, size_t skip)
 {
blob - c6e31c0cceb6d7d2df62e41b110c8f3517b41967
blob + 47cf842ff3044e353b44da88dfe391a2561ee190
--- usr.sbin/snmpd/smi.h
+++ usr.sbin/snmpd/smi.h
@@ -22,6 +22,5 @@ struct ber_element;
 struct ber_oid;
 
 char	*smi_oid2string(struct ber_oid *, char *, size_t, size_t);
-u_long	 smi_getticks(void);
 char	*smi_print_element(struct ber_element *);
 char	*smi_print_element_legacy(struct ber_element *);
blob - 60af191fb6f00ff295542f65ed9c733b66038830
blob + 85581e7ee9b020d658101571df8bf8a761d7b5f6
--- usr.sbin/snmpd/snmpd.c
+++ usr.sbin/snmpd/snmpd.c
@@ -218,7 +218,6 @@ main(int argc, char *argv[])
 	log_init(debug, LOG_DAEMON);
 	log_setverbose(verbose);
 
-	gettimeofday(&env->sc_starttime, NULL);
 	env->sc_engine_boots = 0;
 
 	proc_init(ps, procs, nitems(procs), debug, argc0, argv0, proc_id);
blob - bc1b9ff7d6b8fab49046e4fe3bec9ba5fca31a35
blob + 44773fae763979f619296a7421a8ea87e7429a46
--- usr.sbin/snmpd/snmpd.h
+++ usr.sbin/snmpd/snmpd.h
@@ -22,7 +22,6 @@
 
 #include <sys/queue.h>
 #include <sys/socket.h>
-#include <sys/time.h>
 #include <sys/tree.h>
 #include <sys/types.h>
 #include <sys/un.h>
@@ -402,7 +401,6 @@ struct snmpd {
 	const char		*sc_confpath;
 	struct addresslist	 sc_addresses;
 	struct axmasterlist	 sc_agentx_masters;
-	struct timeval		 sc_starttime;
 	u_int32_t		 sc_engine_boots;
 
 	char			 sc_rdcommunity[SNMPD_MAXCOMMUNITYLEN];
blob - c68ada5fb0bc53a5c520699e2f5eb7e6114c6dc3
blob + f676ead16b9622eea34346322e562365ec058815
--- usr.sbin/snmpd/trap.c
+++ usr.sbin/snmpd/trap.c
@@ -24,9 +24,9 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "application.h"
 #include "log.h"
 #include "mib.h"
-#include "smi.h"
 #include "snmp.h"
 #include "snmpd.h"
 
@@ -60,7 +60,7 @@ trap_send(struct ber_oid *oid, struct ber_element *elm
 	/* Add mandatory varbind elements */
 	trap = ober_add_sequence(NULL);
 	vblist = ober_printf_elements(trap, "{Odt}{OO}",
-	    &uptime, smi_getticks(),
+	    &uptime, appl_sysuptime(NULL),
 	    BER_CLASS_APPLICATION, SNMP_T_TIMETICKS,
 	    &trapoid, oid);
 	if (elm != NULL)