> Date: Thu, 17 Mar 2022 13:24:24 +0100
> From: Alexander Bluhm <alexander.bl...@gmx.net>
>
> On Thu, Mar 17, 2022 at 08:24:10AM +0100, Claudio Jeker wrote:
> > On Thu, Mar 17, 2022 at 12:47:15AM +0100, Alexander Bluhm wrote:
> > > Hi,
> > >
> > > My previous atempt to add a mutex to in_pcb.h was reverted as it
> > > broke userland build.
> > >
> > > Is the correct fix to include sys/mutex.h in every .c file that
> > > includes netinet/in_pcb.h ? I made a release with it.
> > > Or should I include sys/mutex.h in netinet/in_pcb.h ?
> >
> > I would add sys/mutex.h in netinet/in_pcb.h. We do the same in other
> > headers like sys/proc.h etc.
>
> This survived make release. It is similar to what we do in sys/proc.h
> as suggested by claudio@ and has more #ifdef _KERNEL to please
> kettenis@.
>
> ok?
Sorry, but I don't think it is. The problem is that "struct mutex"
changes depending on whether WITNESS is defined. This means that if
you include mutex in a data structure exported to userland and you're
running a kernel with WITNESS enabled, the data structures don't match
up.
The reverse is also possible (although much less likely). Since
WITNESS is in the global namespace, code might define it and therefore
accidentally change the data structure based and cause a mismatch when
you're running on a normal kernel.
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?
> 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
>
>
