> Date: Thu, 17 Mar 2022 15:14:51 +0100
> From: Alexander Bluhm <alexander.bl...@gmx.net>
>
> On Thu, Mar 17, 2022 at 02:09:39PM +0100, Mark Kettenis wrote:
> > I fear the fundamental problem is that we should not expose data
> > structures internal to the kernel to userland. What I don't
> > understand though is how that happens. The sysctl code doesn't seem
> > to export "struct inpcb" instances directly, but instead it exports
> > selected members through "struct kinfo_file". So why is "struct
> > inpcb" exposed to userland at all?
>
> A few tools use it. One thing is post mortem analysis of kernel
> core dumps. Sometimes I get dumps sent from customers. They don't
> have WITNESS.
>
> Some traditional network debugging tools use these strutures.
>
> lib/libkvm
> sbin/sysctl
> usr.bin/netstat
> usr.bin/tcpbench
> usr.sbin/trpt
>
> As you need sysctl kern.allowkmem=1 for them this is only useful
> on debugging machines. Ob course they can be rewritten using sysctl.
> A drawback is that you have to write a lot of copy code and post
> mortem analysis code gets out of sync. I see tools with -M -N
> breaking over time.
Well, maybe the libkvm case is fine for debugging tools. You're
supposed to know what you're doing in that case. And if you put the
mutex at the very end of the structure maybe you won't be utterly
confused if you try to run those tools on a WITNESS kernel.
However, for sysctl(8) we really should use explicitly defined export
data structures. You listed sysctl(8) above together with the other
tools, but sysctl doesn't use libkvm. Is the sysctl binary needlessly
pulling in <netinet/in_pcb.h>?
> The question is how to proceed. For MP in the network stack I need
> mutex in structs. And I don't want to rewrite all tools before
> making progress.
Well, the big question here is whether <netinet/in_pcb.h> is a
userland header or not. If it is, we need to be careful about what it
exports and not add random kernel data structures into it without
proper #ifdef _KERNEL. If it isn't, then your solution of including
<sys/mutex.h> is fine, but we should make sure that only the libkvm
users include that header file.
> > > Index: sys/netinet/in_pcb.h
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.h,v
> > > retrieving revision 1.125
> > > diff -u -p -r1.125 in_pcb.h
> > > --- sys/netinet/in_pcb.h 14 Mar 2022 22:38:43 -0000 1.125
> > > +++ sys/netinet/in_pcb.h 17 Mar 2022 00:44:54 -0000
> > > @@ -65,6 +65,7 @@
> > > #define _NETINET_IN_PCB_H_
> > >
> > > #include <sys/queue.h>
> > > +#include <sys/mutex.h>
> > > #include <sys/refcnt.h>
> > > #include <netinet/ip6.h>
> > > #include <netinet6/ip6_var.h>
> > > Index: sys/sys/mutex.h
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/sys/mutex.h,v
> > > retrieving revision 1.18
> > > diff -u -p -r1.18 mutex.h
> > > --- sys/sys/mutex.h 23 Apr 2019 13:35:12 -0000 1.18
> > > +++ sys/sys/mutex.h 17 Mar 2022 00:44:23 -0000
> > > @@ -48,6 +48,8 @@ struct mutex {
> > > #endif
> > > };
> > >
> > > +#ifdef _KERNEL
> > > +
> > > /*
> > > * To prevent lock ordering problems with the kernel lock, we need to
> > > * make sure we block all interrupts that can grab the kernel lock.
> > > @@ -148,7 +150,7 @@ void _mtx_init_flags(struct mutex *, int
> > >
> > > #endif /* WITNESS */
> > >
> > > -#if defined(_KERNEL) && defined(DDB)
> > > +#ifdef DDB
> > >
> > > struct db_mutex {
> > > struct cpu_info *mtx_owner;
> > > @@ -160,6 +162,8 @@ struct db_mutex {
> > > void db_mtx_enter(struct db_mutex *);
> > > void db_mtx_leave(struct db_mutex *);
> > >
> > > -#endif /* _KERNEL && DDB */
> > > +#endif /* DDB */
> > > +
> > > +#endif /* _KERNEL */
> > >
> > > #endif
> > >
> > >
>
