Menu
▾
▴
net-snmp-coders
|
From: Mark J. <ma...@fr...> - 2024-04-12 13:44:42
|
I'm quite new to net-snmp, so apologies in advance if this is the wrong
place to discuss patches.
Currently, when one runs snmpd on FreeBSD, it keeps /dev/mem and
/dev/kmem open. This is a result of using kvm_openfiles() to get a kvm
descriptor, which is used to implement various MIBs (via kvm_getprocs(),
kvm_getswapinfo(), etc.). However, the libkvm interfaces used by
net-snmp don't actually require /dev/mem or /dev/kmem - they are all
implemented using sysctls.
I would like to have snmpd not hold /dev/(k)mem open. I can of course
compile using --without-kmem-usage, but that disables the use of libkvm,
so all of that code (used to implement the hostres MIB, for instance)
would need to be rewritten. However, it turns out that one can ask
kvm_openfiles() to not open /dev/mem and /dev/kmem, by passing
"/dev/null" as the path. In fact, snmpd already contains code to do
this, but it's only done in a fallback path in init_kmem().
I would like to introduce the patch below, which gets compiled when
--without-kmem-usage is specified. In this case, snmpd will still use
libkvm, but won't open /dev/(k)mem. In my testing so far, this works
perfectly. Does anyone have any thoughts on this patch/approach? Would
the net-snmp project be willing to accept the patch? Thank you in
advance for any feedback or guidance.
commit 1c7881da5963508bcbcbd4ac44a60192fec4f7d8
Author: Mark Johnston <ma...@Fr...>
Date: Thu Apr 4 16:34:26 2024 -0400
snmpd: Enable the use of libkvm without kmem access on FreeBSD
diff --git a/agent/kernel.c b/agent/kernel.c
index 285a603a77..164a6a8ca2 100644
--- a/agent/kernel.c
+++ b/agent/kernel.c
@@ -259,7 +259,37 @@ free_kmem(void)
kmem = -1;
}
}
+#elif defined(__FreeBSD__)
+kvm_t *kd;
+
+/**
+ * Initialize the libkvm descriptor. On FreeBSD we can use most of libkvm
+ * without requiring /dev/kmem access. Only kvm_nlist() and kvm_read() need
+ * that, and we don't use them.
+ *
+ * @return TRUE upon success; FALSE upon failure.
+ */
+int
+init_kmem(const char *file)
+{
+ char err[4096];
+ kd = kvm_openfiles(NULL, "/dev/null", NULL, O_RDONLY, err);
+ if (!kd) {
+ snmp_log(LOG_CRIT, "init_kmem: kvm_openfiles failed: %s\n", err);
+ return FALSE;
+ }
+ return TRUE;
+}
+
+void
+free_kmem(void)
+{
+ if (kd != NULL) {
+ (void)kvm_close(kd);
+ kd = NULL;
+ }
+}
#else
int
init_kmem(const char *file)
|
|
From: Bart V. A. <bva...@ac...> - 2024-04-12 14:13:57
|
On 4/12/24 06:44, Mark Johnston wrote: > I would like to introduce the patch below, which gets compiled when > --without-kmem-usage is specified. In this case, snmpd will still use > libkvm, but won't open /dev/(k)mem. In my testing so far, this works > perfectly. Does anyone have any thoughts on this patch/approach? Would > the net-snmp project be willing to accept the patch? Thank you in > advance for any feedback or guidance. What privileges are required to call kvm_open() on FreeBSD? Are the same privileges required as for opening /dev/kmem or not? If not, has it been considered to make init_kmem() call kvm_open() on FreeBSD independent of whether or not --without-kmem-usage has been specified? Thanks, Bart. |
|
From: Mark J. <ma...@fr...> - 2024-04-12 14:29:55
|
On Fri, Apr 12, 2024 at 07:13:41AM -0700, Bart Van Assche wrote: > On 4/12/24 06:44, Mark Johnston wrote: > > I would like to introduce the patch below, which gets compiled when > > --without-kmem-usage is specified. In this case, snmpd will still use > > libkvm, but won't open /dev/(k)mem. In my testing so far, this works > > perfectly. Does anyone have any thoughts on this patch/approach? Would > > the net-snmp project be willing to accept the patch? Thank you in > > advance for any feedback or guidance. > > What privileges are required to call kvm_open() on FreeBSD? Are > the same privileges required as for opening /dev/kmem or not? The only privileges required are those needed to open the file. So, kvm_openfiles(NULL, NULL, NULL, O_RDONLY, err) requires root privileges, because it tries to open /dev/mem. If I change the second parameter to "/dev/null", then any user can call it successfully, and most of the kvm_* functions will still work as expected. > If not, > has it been considered to make init_kmem() call kvm_open() on FreeBSD > independent of whether or not --without-kmem-usage has been specified? Assuming I don't misunderstand, that's effectively what my patch does - it just does so in a separate implementation of init_kmem(). Do you mean that on FreeBSD we should always perform an unprivileged kvm_openfile() call, no matter whether --without-kmem-usage is specified? |
|
From: Bart V. A. <bva...@ac...> - 2024-04-12 22:52:04
|
On 4/12/24 7:29 AM, Mark Johnston wrote: > Do you mean that on FreeBSD we should always perform an unprivileged > kvm_openfile() call, no matter whether --without-kmem-usage is > specified? Yes, that's what I'm proposing. If someone disagrees, please share your opinion now. Thanks, Bart. |
|
From: Mark J. <ma...@fr...> - 2024-04-13 14:45:01
|
On Fri, Apr 12, 2024 at 03:51:51PM -0700, Bart Van Assche wrote: > On 4/12/24 7:29 AM, Mark Johnston wrote: > > Do you mean that on FreeBSD we should always perform an unprivileged > > kvm_openfile() call, no matter whether --without-kmem-usage is > > specified? > > Yes, that's what I'm proposing. If someone disagrees, please share your > opinion now. I can't see any reason not to go ahead with that, for what it's worth. I'm confident that none of the existing code in net-snmp needs /dev/kmem access on FreeBSD, and it shouldn't be required by new code. |
|
From: Mark J. <ma...@fr...> - 2024-04-30 17:54:08
|
On Fri, Apr 12, 2024 at 03:51:51PM -0700, Bart Van Assche wrote:
> On 4/12/24 7:29 AM, Mark Johnston wrote:
> > Do you mean that on FreeBSD we should always perform an unprivileged
> > kvm_openfile() call, no matter whether --without-kmem-usage is
> > specified?
>
> Yes, that's what I'm proposing. If someone disagrees, please share your
> opinion now.
I spent some more time testing this and hit a small problem with the
IPv6-TCP MIB. The implementation doesn't compile on FreeBSD (it
references a non-existent field in struct inpcb) and so is disabled in
the FreeBSD ports tree, so I didn't catch it earlier when testing a
patch to avoid using /dev/kmem.
The implementation of var_tcp6() and var_udp6() used on FreeBSD isn't
quite right; it uses the wrong structures to access the output of the
net.inet.{tcp,udp}.pcblist sysctls, and var_tcp6() tries to use kmem
access to get the TCP state of the matched connection, which at least on
FreeBSD isn't necessary (net.inet.tcp.pcblist exports that field
directly via struct xtcpcb).
I wrote the patch below to fix this and manually verified the MIB values
on a test system.
commit f131c5b7dd599cf6c3a15b372075d299262cbfee
Author: Mark Johnston <ma...@Fr...>
Date: Tue Apr 30 10:23:25 2024 -0400
Fix var_udp6 and var_tcp6 on FreeBSD
diff --git a/agent/mibgroup/mibII/ipv6.c b/agent/mibgroup/mibII/ipv6.c
index f44568c9fa..1ec05efce2 100644
--- a/agent/mibgroup/mibII/ipv6.c
+++ b/agent/mibgroup/mibII/ipv6.c
@@ -5,9 +5,6 @@
#include <net-snmp/net-snmp-config.h>
#include <net-snmp/net-snmp-features.h>
-/* For FreeBSD */
-#define _WANT_INPCB 1
-#define _WANT_TCPCB 1
#include <sys/types.h>
#include <sys/socket.h>
#ifdef HAVE_SYS_IOCTL_H
@@ -1513,8 +1510,10 @@ var_udp6(register struct variable * vp,
int result;
int i, j;
caddr_t p;
-#if defined(openbsd4) || defined(freebsd3)
+#if defined(openbsd4)
static struct inpcb in6pcb, savpcb;
+#elif defined(freebsd3)
+ static struct xinpcb in6pcb, savpcb;
#else
static struct in6pcb in6pcb, savpcb;
#endif
@@ -1614,8 +1613,7 @@ var_udp6(register struct variable * vp,
DEBUGMSGTL(("mibII/ipv6", "looping: p=%p\n", p));
#if defined(freebsd3)
- /* To do: fill in in6pcb properly. */
- memset(&in6pcb, 0, sizeof(in6pcb));
+ in6pcb = *(struct xinpcb *) xig;
#elif defined(darwin)
in6pcb = ((struct xinpcb *) xig)->xi_inp;
#else
@@ -2108,12 +2106,18 @@ var_tcp6(register struct variable * vp,
int result;
int i, j;
caddr_t p;
-#if defined(openbsd4) || defined(freebsd3)
+#if defined(openbsd4)
static struct inpcb in6pcb, savpcb;
+#elif defined(freebsd3)
+ static struct xinpcb in6pcb;
+ static int savstate;
#else
static struct in6pcb in6pcb, savpcb;
#endif
+#if !defined(freebsd3)
struct tcpcb tcpcb;
+#endif
+ int state;
int found, savnameLen;
#if defined(__NetBSD__) && __NetBSD_Version__ >= 106250000 || defined(openbsd4) /*1.6Y*/
struct inpcbtable tcbtable;
@@ -2208,8 +2212,7 @@ var_tcp6(register struct variable * vp,
DEBUGMSGTL(("mibII/ipv6", "looping: p=%p\n", p));
#if defined(freebsd3)
- /* To do: fill in in6pcb properly. */
- memset(&in6pcb, 0, sizeof(in6pcb));
+ in6pcb = ((struct xtcpcb *) xig)->xt_inp;
#elif defined(dragonfly)
in6pcb = xtp->xt_inp;
#elif defined(darwin)
@@ -2294,7 +2297,11 @@ var_tcp6(register struct variable * vp,
#endif
result = snmp_oid_compare(name, *length, newname, j);
if (exact && (result == 0)) {
+#if defined(freebsd3)
+ savstate = ((struct xtcpcb *) xig)->t_state;
+#else
memcpy(&savpcb, &in6pcb, sizeof(savpcb));
+#endif
savnameLen = j;
memcpy(savname, newname, j * sizeof(oid));
found++;
@@ -2305,7 +2312,11 @@ var_tcp6(register struct variable * vp,
*/
if ((savnameLen == 0) ||
(snmp_oid_compare(savname, savnameLen, newname, j) > 0)) {
+#if defined(freebsd3)
+ savstate = ((struct xtcpcb *) xig)->t_state;
+#else
memcpy(&savpcb, &in6pcb, sizeof(savpcb));
+#endif
savnameLen = j;
memcpy(savname, newname, j * sizeof(oid));
found++;
@@ -2344,19 +2355,27 @@ var_tcp6(register struct variable * vp,
return NULL;
*length = savnameLen;
memcpy((char *) name, (char *) savname, *length * sizeof(oid));
+#if defined(freebsd3)
+ state = savstate;
+#elif defined(__NetBSD__) && __NetBSD_Version__ >= 999010400
memcpy(&in6pcb, &savpcb, sizeof(savpcb));
-#if defined(__NetBSD__) && __NetBSD_Version__ >= 999010400
if (!NETSNMP_KLOOKUP(in6pcb.in6p_pcb.inp_ppcb, (char *) &tcpcb, sizeof(tcpcb))) {
DEBUGMSGTL(("mibII/ipv6", "klookup fail for tcb6.tcpcb at %p\n",
in6pcb.in6p_pcb.inp_ppcb));
+ found = 0;
+ return NULL;
+ }
+ state = (int)tcpcb.t_state;
#else
+ memcpy(&in6pcb, &savpcb, sizeof(savpcb));
if (!NETSNMP_KLOOKUP(in6pcb.inp_ppcb, (char *) &tcpcb, sizeof(tcpcb))) {
DEBUGMSGTL(("mibII/ipv6", "klookup fail for tcb6.tcpcb at %p\n",
in6pcb.inp_ppcb));
-#endif
found = 0;
return NULL;
}
+ state = (int)tcpcb.t_state;
+#endif
*write_method = 0;
*var_len = sizeof(long); /* default to 'long' results */
@@ -2368,7 +2387,7 @@ var_tcp6(register struct variable * vp,
DEBUGMSGTL(("mibII/ipv6", "magic=%d\n", vp->magic));
switch (vp->magic) {
case IPV6TCPCONNSTATE:
- long_return = mapTcpState((int)tcpcb.t_state);
+ long_return = mapTcpState(state);
return (u_char *) & long_return;
default:
break;
|
|
From: Bart V. A. <bva...@ac...> - 2024-05-06 17:34:34
|
On 4/30/24 10:53 AM, Mark Johnston wrote: > Fix var_udp6 and var_tcp6 on FreeBSD > [ ... ] This patch has been applied on the V5-9-patches and master branches. Thanks for the patch! Bart. |
|
From: Mark J. <ma...@fr...> - 2024-06-10 20:27:38
|
On Fri, Apr 12, 2024 at 03:51:51PM -0700, Bart Van Assche wrote: > On 4/12/24 7:29 AM, Mark Johnston wrote: > > Do you mean that on FreeBSD we should always perform an unprivileged > > kvm_openfile() call, no matter whether --without-kmem-usage is > > specified? > > Yes, that's what I'm proposing. If someone disagrees, please share your > opinion now. Hi Bart, Were you planning to make a change along these lines? Would it be helpful for me to submit a patch? A few of us have been testing snmpd with my original patch (to tell libkvm not to open /dev/kmem etc.) for a while now with no issues. Thanks, -Mark |
|
From: Bart V. A. <bva...@ac...> - 2024-06-10 22:51:41
|
On 6/10/24 13:27, Mark Johnston wrote: > Would it be helpful for me to submit a patch? A few of us have been > testing snmpd with my original patch (to tell libkvm not to open > /dev/kmem etc.) for a while now with no issues. A patch definitely would be welcome. Thanks, Bart. |
|
From: Mark J. <ma...@fr...> - 2024-06-11 14:04:16
|
On Mon, Jun 10, 2024 at 03:51:33PM -0700, Bart Van Assche wrote:
>
> On 6/10/24 13:27, Mark Johnston wrote:
> > Would it be helpful for me to submit a patch? A few of us have been
> > testing snmpd with my original patch (to tell libkvm not to open
> > /dev/kmem etc.) for a while now with no issues.
> A patch definitely would be welcome.
The patch below implements your suggestion. That is, init_kmem() gets a
libkvm handle without opening /dev/kmem, no matter whether --with-kmem-usage
or --without-kmem-usage was specified at compile time.
Thinking about this more, users might still want --with-kmem-usage to
cause snmpd to open /dev/kmem, in the case where they have custom MIBs
implementations which require the use of klookup(). With this patch,
it's impossible to use klookup() on FreeBSD. I believe this is fine for
the code shipped with net-snmp, but it might break some custom 3rd-party
MIBs. I'm not too concerned about this, as the official FreeBSD
net-snmp package will be compiled with --without-kmem-usage, but perhaps
it still makes sense to leave an escape hatch.
commit 304f8cf7f176920cb689d237f612c9a25cd14e84
Author: Mark Johnston <ma...@Fr...>
Date: Thu Apr 4 16:34:26 2024 -0400
snmpd: Always open libkvm in "safe mode" on FreeBSD
By specifying /dev/null as the path to kvm_openfiles(), we can get a
libkvm descriptor which does not hold /dev/kmem open. None of the code
shipped with net-snmp needs a /dev/kmem handle.
Make this change for both the NETSNMP_NO_KMEM_USAGE and
!NETSNMP_NO_KMEM_USAGE cases, per a suggestion from
Bart Van Assche <bva...@ac...>.
diff --git a/agent/kernel.c b/agent/kernel.c
index 9a6d22592c..671e5244fc 100644
--- a/agent/kernel.c
+++ b/agent/kernel.c
@@ -44,7 +44,7 @@
#include "kernel.h"
#include <net-snmp/agent/ds_agent.h>
-#if defined(HAVE_KVM_H) && !defined(NETSNMP_NO_KMEM_USAGE)
+#if defined(HAVE_KVM_H) && !defined(NETSNMP_NO_KMEM_USAGE) && !defined(__FreeBSD__)
kvm_t *kd;
/**
@@ -130,7 +130,7 @@ free_kmem(void)
}
}
-#elif defined(HAVE_NLIST_H) && !defined(__linux__) && \
+#elif defined(HAVE_NLIST_H) && !defined(__linux__) && !defined(__FreeBSD__) && \
!defined(NETSNMP_NO_KMEM_USAGE)
static off_t klseek(off_t);
@@ -252,7 +252,48 @@ free_kmem(void)
kmem = -1;
}
}
+#elif defined(__FreeBSD__)
+kvm_t *kd;
+
+/**
+ * Initialize the libkvm descriptor. On FreeBSD we can use most of libkvm
+ * without requiring /dev/kmem access. Only kvm_nlist() and kvm_read() need
+ * that, and we don't use them.
+ *
+ * @return TRUE upon success; FALSE upon failure.
+ */
+int
+init_kmem(const char *file)
+{
+ char err[4096];
+
+ kd = kvm_openfiles(NULL, "/dev/null", NULL, O_RDONLY, err);
+ if (!kd) {
+ snmp_log(LOG_CRIT, "init_kmem: kvm_openfiles failed: %s\n", err);
+ return FALSE;
+ }
+ return TRUE;
+}
+/**
+ * A stub to return failure to any attempt to read kernel memory. Our
+ * libkvm handle doesn't enable /dev/kmem access. MIB implementations should
+ * use unprivileged to fetch information about the system.
+ */
+int
+klookup(unsigned long off, void *target, size_t siz)
+{
+ return 0;
+}
+
+void
+free_kmem(void)
+{
+ if (kd != NULL) {
+ (void)kvm_close(kd);
+ kd = NULL;
+ }
+}
#else
int
init_kmem(const char *file)
|
|
From: Bart V. A. <bva...@ac...> - 2024-06-18 20:08:24
|
On 6/11/24 7:04 AM, Mark Johnston wrote: > commit 304f8cf7f176920cb689d237f612c9a25cd14e84 > Author: Mark Johnston <ma...@Fr...> > Date: Thu Apr 4 16:34:26 2024 -0400 > > snmpd: Always open libkvm in "safe mode" on FreeBSD This patch has been applied on the V5-9-patches and master branches. Thanks for the patch! Bart. |
Thanks for helping keep SourceForge clean.
X