On 20/12/20(Sun) 20:55, Mark Kettenis wrote:
> > Date: Sat, 19 Dec 2020 18:07:41 -0300
> > From: Martin Pieuchot <[email protected]>
> >
> > On 18/12/20(Fri) 08:04, Todd C. Miller wrote:
> > > On Fri, 18 Dec 2020 13:34:39 +0100, Mark Kettenis wrote:
> > >
> > > > Anyway, your analysis is right. When a kernel thread wants to use
> > > > pmap_extract(9) on a userland pmap, it needs to lock pm_apte_mtx to
> > > > prevent another thread from simultaniously activating a userland pmap
> > > > too. So indeed, pm_apte_mtx needs to be properly initialized for the
> > > > kernel pmap.
> > > >
> > > > However, pm_mtx should never be used for the kernel pmap. If we don't
> > > > initialize the lock, witness will help us catching this condition, so
> > > > maybe we shouldn't...
> > >
> > > I think a comment is warranted if we don't want to initialize the
> > > lock to prevent someone from fixing this in the future ;-)
> >
> > A solution based on a comment and a non-enabled by option seems very
> > fragile to me. I came up with the idea of poisoning the ipl of the
> > mutex. What do you think?
>
> Not sure if it makes sense to do this only for i386. And is
> splraise() the right place for the check? Instead of mtx_enter()?
Well calling splraise() with a negative value would also be a bug, no?
Sure, nothing prevent us to add this check for more architectures. Diff
below does it for amd64 and sparc64 as well. Could you do it for the ones
you can test?
Index: arch/amd64/amd64/intr.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/intr.c,v
retrieving revision 1.54
diff -u -p -r1.54 intr.c
--- arch/amd64/amd64/intr.c 17 Jun 2020 06:14:52 -0000 1.54
+++ arch/amd64/amd64/intr.c 21 Dec 2020 11:59:31 -0000
@@ -696,6 +696,8 @@ splraise(int nlevel)
int olevel;
struct cpu_info *ci = curcpu();
+ KASSERT(nlevel >= IPL_NONE);
+
olevel = ci->ci_ilevel;
ci->ci_ilevel = MAX(ci->ci_ilevel, nlevel);
return (olevel);
Index: arch/i386/i386/machdep.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.641
diff -u -p -r1.641 machdep.c
--- arch/i386/i386/machdep.c 8 Nov 2020 20:37:23 -0000 1.641
+++ arch/i386/i386/machdep.c 21 Dec 2020 11:58:12 -0000
@@ -3996,6 +3996,8 @@ splraise(int ncpl)
{
int ocpl;
+ KASSERT(ncpl >= IPL_NONE);
+
_SPLRAISE(ocpl, ncpl);
return (ocpl);
}
Index: arch/i386/i386/pmap.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/pmap.c,v
retrieving revision 1.209
diff -u -p -r1.209 pmap.c
--- arch/i386/i386/pmap.c 24 Sep 2020 11:36:50 -0000 1.209
+++ arch/i386/i386/pmap.c 19 Dec 2020 20:58:48 -0000
@@ -961,6 +961,8 @@ pmap_bootstrap(vaddr_t kva_start)
*/
kpm = pmap_kernel();
+ mtx_init(&kpm->pm_mtx, -1); /* must not be used */
+ mtx_init(&kpm->pm_apte_mtx, IPL_VM);
uvm_objinit(&kpm->pm_obj, NULL, 1);
bzero(&kpm->pm_list, sizeof(kpm->pm_list)); /* pm_list not used */
kpm->pm_pdir = (vaddr_t)(proc0.p_addr->u_pcb.pcb_cr3 + KERNBASE);
Index: arch/sparc64/sparc64/intr.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/sparc64/intr.c,v
retrieving revision 1.61
diff -u -p -r1.61 intr.c
--- arch/sparc64/sparc64/intr.c 24 Jun 2020 22:03:40 -0000 1.61
+++ arch/sparc64/sparc64/intr.c 21 Dec 2020 12:00:37 -0000
@@ -322,6 +322,7 @@ intr_establish(int level, struct intrhan
int
splraise(int ipl)
{
+ KASSERT(ipl >= IPL_NONE);
return (_splraise(ipl));
}
