go/rust vs uvm_map_inentry()

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

go/rust vs uvm_map_inentry()

Martin Pieuchot
I'm no longer able to reproduce the corruption while building lang/go
with the diff below.  Something relevant to threading change in go since
march?

Can someone try this diff and tell me if go and/or rust still fail?

Index: uvm/uvm_map.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.266
diff -u -p -r1.266 uvm_map.c
--- uvm/uvm_map.c 12 Sep 2020 17:08:50 -0000 1.266
+++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -0000
@@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
  boolean_t ok = TRUE;
 
  if (uvm_map_inentry_recheck(serial, addr, ie)) {
- KERNEL_LOCK();
  ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
  if (!ok) {
+ KERNEL_LOCK();
  printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
     addr, ie->ie_start, ie->ie_end);
  p->p_p->ps_acflag |= AMAP;
  sv.sival_ptr = (void *)PROC_PC(p);
  trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
+ KERNEL_UNLOCK();
  }
- KERNEL_UNLOCK();
  }
  return (ok);
 }

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Sebastien Marie-3
On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> I'm no longer able to reproduce the corruption while building lang/go
> with the diff below.  Something relevant to threading change in go since
> march?
>
> Can someone try this diff and tell me if go and/or rust still fail?

quickly tested with rustc build (nightly here), and it is failing at random places (not always at the same) with memory errors (signal 11, compiler ICE signal 6...)


> Index: uvm/uvm_map.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.266
> diff -u -p -r1.266 uvm_map.c
> --- uvm/uvm_map.c 12 Sep 2020 17:08:50 -0000 1.266
> +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -0000
> @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
>   boolean_t ok = TRUE;
>  
>   if (uvm_map_inentry_recheck(serial, addr, ie)) {
> - KERNEL_LOCK();
>   ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
>   if (!ok) {
> + KERNEL_LOCK();
>   printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
>      addr, ie->ie_start, ie->ie_end);
>   p->p_p->ps_acflag |= AMAP;
>   sv.sival_ptr = (void *)PROC_PC(p);
>   trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> + KERNEL_UNLOCK();
>   }
> - KERNEL_UNLOCK();
>   }
>   return (ok);
>  }
>

--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Mark Kettenis
> Date: Sun, 13 Sep 2020 16:49:48 +0200
> From: Sebastien Marie <[hidden email]>
>
> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below.  Something relevant to threading change in go since
> > march?
> >
> > Can someone try this diff and tell me if go and/or rust still fail?
>
> quickly tested with rustc build (nightly here), and it is failing at
> random places (not always at the same) with memory errors (signal
> 11, compiler ICE signal 6...)

Is it failing when you don't have tracing enabled and not failing when
the tracing is disabled perhaps?

> > Index: uvm/uvm_map.c
> > ===================================================================
> > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > retrieving revision 1.266
> > diff -u -p -r1.266 uvm_map.c
> > --- uvm/uvm_map.c 12 Sep 2020 17:08:50 -0000 1.266
> > +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -0000
> > @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> >   boolean_t ok = TRUE;
> >  
> >   if (uvm_map_inentry_recheck(serial, addr, ie)) {
> > - KERNEL_LOCK();
> >   ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> >   if (!ok) {
> > + KERNEL_LOCK();
> >   printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> >      addr, ie->ie_start, ie->ie_end);
> >   p->p_p->ps_acflag |= AMAP;
> >   sv.sival_ptr = (void *)PROC_PC(p);
> >   trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> > + KERNEL_UNLOCK();
> >   }
> > - KERNEL_UNLOCK();
> >   }
> >   return (ok);
> >  }
> >
>
> --
> Sebastien Marie
>
>

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Theo de Raadt-2
In reply to this post by Sebastien Marie-3
Sebastien Marie <[hidden email]> wrote:

> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below.  Something relevant to threading change in go since
> > march?
> >
> > Can someone try this diff and tell me if go and/or rust still fail?
>
> quickly tested with rustc build (nightly here), and it is failing at random places (not always at the same) with memory errors (signal 11, compiler ICE signal 6...)

Ah, so that is a firm no.  Totally busted.

Clearly uvm_map_inentry_fix() obviously needs the KERNEL_LOCK in the
presence of threads, I guess one thread can get into here while another
is changing the map.

The first check in uvm_map_inentry_fix does two checks against the map,
but the map is not locked:

        if (addr < map->min_offset || addr >= map->max_offset)


>
>
> > Index: uvm/uvm_map.c
> > ===================================================================
> > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > retrieving revision 1.266
> > diff -u -p -r1.266 uvm_map.c
> > --- uvm/uvm_map.c 12 Sep 2020 17:08:50 -0000 1.266
> > +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -0000
> > @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> >   boolean_t ok = TRUE;
> >  
> >   if (uvm_map_inentry_recheck(serial, addr, ie)) {
> > - KERNEL_LOCK();
> >   ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> >   if (!ok) {
> > + KERNEL_LOCK();
> >   printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> >      addr, ie->ie_start, ie->ie_end);
> >   p->p_p->ps_acflag |= AMAP;
> >   sv.sival_ptr = (void *)PROC_PC(p);
> >   trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> > + KERNEL_UNLOCK();
> >   }
> > - KERNEL_UNLOCK();
> >   }
> >   return (ok);
> >  }
> >
>
> --
> Sebastien Marie
>

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Theo de Raadt-2
We need to know which one is hitting a broken counter party -- is it the
stack checker, or the syscall checker.

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Mark Kettenis
In reply to this post by Theo de Raadt-2
> From: "Theo de Raadt" <[hidden email]>
> Date: Sun, 13 Sep 2020 08:56:04 -0600
>
> Sebastien Marie <[hidden email]> wrote:
>
> > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > > I'm no longer able to reproduce the corruption while building lang/go
> > > with the diff below.  Something relevant to threading change in go since
> > > march?
> > >
> > > Can someone try this diff and tell me if go and/or rust still fail?
> >
> > quickly tested with rustc build (nightly here), and it is failing at random places (not always at the same) with memory errors (signal 11, compiler ICE signal 6...)
>
> Ah, so that is a firm no.  Totally busted.
>
> Clearly uvm_map_inentry_fix() obviously needs the KERNEL_LOCK in the
> presence of threads, I guess one thread can get into here while another
> is changing the map.
>
> The first check in uvm_map_inentry_fix does two checks against the map,
> but the map is not locked:
>
>         if (addr < map->min_offset || addr >= map->max_offset)

No that should work; min_offset and max_offset are immutable after exec.

> > > Index: uvm/uvm_map.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > > retrieving revision 1.266
> > > diff -u -p -r1.266 uvm_map.c
> > > --- uvm/uvm_map.c 12 Sep 2020 17:08:50 -0000 1.266
> > > +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -0000
> > > @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> > >   boolean_t ok = TRUE;
> > >  
> > >   if (uvm_map_inentry_recheck(serial, addr, ie)) {
> > > - KERNEL_LOCK();
> > >   ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> > >   if (!ok) {
> > > + KERNEL_LOCK();
> > >   printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> > >      addr, ie->ie_start, ie->ie_end);
> > >   p->p_p->ps_acflag |= AMAP;
> > >   sv.sival_ptr = (void *)PROC_PC(p);
> > >   trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> > > + KERNEL_UNLOCK();
> > >   }
> > > - KERNEL_UNLOCK();
> > >   }
> > >   return (ok);
> > >  }
> > >
> >
> > --
> > Sebastien Marie
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Martin Pieuchot
In reply to this post by Mark Kettenis
On 13/09/20(Sun) 16:54, Mark Kettenis wrote:

> > Date: Sun, 13 Sep 2020 16:49:48 +0200
> > From: Sebastien Marie <[hidden email]>
> >
> > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > > I'm no longer able to reproduce the corruption while building lang/go
> > > with the diff below.  Something relevant to threading change in go since
> > > march?
> > >
> > > Can someone try this diff and tell me if go and/or rust still fail?
> >
> > quickly tested with rustc build (nightly here), and it is failing at
> > random places (not always at the same) with memory errors (signal
> > 11, compiler ICE signal 6...)
>
> Is it failing when you don't have tracing enabled and not failing when
> the tracing is disabled perhaps?

It is failing even without tracing.

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Sebastien Marie-3
In reply to this post by Theo de Raadt-2
On Sun, Sep 13, 2020 at 09:15:15AM -0600, Theo de Raadt wrote:
> crashes -- but without any kernel printfs?

crashes and no kernel printfs

--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Mark Kettenis
In reply to this post by Martin Pieuchot
> Date: Sun, 13 Sep 2020 17:54:18 +0200
> From: Martin Pieuchot <[hidden email]>
>
> On 13/09/20(Sun) 16:54, Mark Kettenis wrote:
> > > Date: Sun, 13 Sep 2020 16:49:48 +0200
> > > From: Sebastien Marie <[hidden email]>
> > >
> > > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > > > I'm no longer able to reproduce the corruption while building lang/go
> > > > with the diff below.  Something relevant to threading change in go since
> > > > march?
> > > >
> > > > Can someone try this diff and tell me if go and/or rust still fail?
> > >
> > > quickly tested with rustc build (nightly here), and it is failing at
> > > random places (not always at the same) with memory errors (signal
> > > 11, compiler ICE signal 6...)
> >
> > Is it failing when you don't have tracing enabled and not failing when
> > the tracing is disabled perhaps?
>
> It is failing even without tracing.

Sorry, I meant is it failing even with tracing?

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Sebastien Marie-3
In reply to this post by Sebastien Marie-3
On Sun, Sep 13, 2020 at 04:49:48PM +0200, Sebastien Marie wrote:
> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below.  Something relevant to threading change in go since
> > march?
> >
> > Can someone try this diff and tell me if go and/or rust still fail?
>
> quickly tested with rustc build (nightly here), and it is failing at random places (not always at the same) with memory errors (signal 11, compiler ICE signal 6...)
>

A first hint.

With the help of deraadt@, it was found that disabling
uvm_map_inentry() call in usertrap() is enough to avoid the crashes.

To be clear, I am using the following diff:

diff 3e16148d8fe176d83ff415f6c03a79618da4401e /data/semarie/repos/openbsd/src
blob - 7f195a5309280943e0138953c61fffcb6a80c6bf
file + sys/arch/amd64/conf/GENERIC.MP
--- sys/arch/amd64/conf/GENERIC.MP
+++ sys/arch/amd64/conf/GENERIC.MP
@@ -4,6 +4,8 @@ include "arch/amd64/conf/GENERIC"
 
 option MULTIPROCESSOR
 #option MP_LOCKDEBUG
-#option WITNESS
+option WITNESS
+
+pseudo-device dt
 
 cpu* at mainbus?
blob - fc23bc67e305a1a1edc7d6f08ecb982dccdc4a45
file + sys/uvm/uvm_map.c
--- sys/uvm/uvm_map.c
+++ sys/uvm/uvm_map.c
@@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p_inentry *ie,
  boolean_t ok = TRUE;
 
  if (uvm_map_inentry_recheck(serial, addr, ie)) {
- KERNEL_LOCK();
  ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
  if (!ok) {
+ KERNEL_LOCK();
  printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
     addr, ie->ie_start, ie->ie_end);
  p->p_p->ps_acflag |= AMAP;
  sv.sival_ptr = (void *)PROC_PC(p);
  trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
+ KERNEL_UNLOCK();
  }
- KERNEL_UNLOCK();
  }
  return (ok);
 }
blob - 4a4c6275aa766fe2e4f5c9d913d1257f41a9d578
file + sys/arch/amd64/amd64/trap.c
--- sys/arch/amd64/amd64/trap.c
+++ sys/arch/amd64/amd64/trap.c
@@ -343,10 +343,12 @@ usertrap(struct trapframe *frame)
  p->p_md.md_regs = frame;
  refreshcreds(p);
 
+#if 0
  if (!uvm_map_inentry(p, &p->p_spinentry, PROC_STACK(p),
     "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
     uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
  goto out;
+#endif
 
  switch (type) {
  case T_PROTFLT: /* protection fault */


Thanks.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Mark Kettenis
> Date: Sun, 13 Sep 2020 19:48:19 +0200
> From: Sebastien Marie <[hidden email]>
>
> On Sun, Sep 13, 2020 at 04:49:48PM +0200, Sebastien Marie wrote:
> > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > > I'm no longer able to reproduce the corruption while building lang/go
> > > with the diff below.  Something relevant to threading change in go since
> > > march?
> > >
> > > Can someone try this diff and tell me if go and/or rust still fail?
> >
> > quickly tested with rustc build (nightly here), and it is failing at random places (not always at the same) with memory errors (signal 11, compiler ICE signal 6...)
> >
>
> A first hint.
>
> With the help of deraadt@, it was found that disabling
> uvm_map_inentry() call in usertrap() is enough to avoid the crashes.
>
> To be clear, I am using the following diff:

The diff below fixes at (for amd64).

What's happening is that uvm_map_inentry() may sleep to grab the lock
of the map.  The fault address is read from cr2 in pageflttrap() which
gets called after this check and if the check sleeps, cr2 is likely to
be clobbered by a page fault in another process.

Diff below fixes this by reading cr2 early and passing it to pageflttrap().

ok?


Index: arch/amd64/amd64/trap.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
retrieving revision 1.80
diff -u -p -r1.80 trap.c
--- arch/amd64/amd64/trap.c 19 Aug 2020 10:10:57 -0000 1.80
+++ arch/amd64/amd64/trap.c 14 Sep 2020 11:17:35 -0000
@@ -92,7 +92,7 @@
 
 #include "isa.h"
 
-int pageflttrap(struct trapframe *, int _usermode);
+int pageflttrap(struct trapframe *, uint64_t, int _usermode);
 void kerntrap(struct trapframe *);
 void usertrap(struct trapframe *);
 void ast(struct trapframe *);
@@ -157,12 +157,11 @@ fault(const char *format, ...)
  * if something was so broken that we should panic.
  */
 int
-pageflttrap(struct trapframe *frame, int usermode)
+pageflttrap(struct trapframe *frame, uint64_t cr2, int usermode)
 {
  struct proc *p = curproc;
  struct pcb *pcb;
  int error;
- uint64_t cr2;
  vaddr_t va;
  struct vm_map *map;
  vm_prot_t ftype;
@@ -172,7 +171,6 @@ pageflttrap(struct trapframe *frame, int
 
  map = &p->p_vmspace->vm_map;
  pcb = &p->p_addr->u_pcb;
- cr2 = rcr2();
  va = trunc_page((vaddr_t)cr2);
 
  KERNEL_LOCK();
@@ -280,6 +278,7 @@ void
 kerntrap(struct trapframe *frame)
 {
  int type = (int)frame->tf_trapno;
+ uint64_t cr2 = rcr2();
 
  verify_smap(__func__);
  uvmexp.traps++;
@@ -299,7 +298,7 @@ kerntrap(struct trapframe *frame)
  /*NOTREACHED*/
 
  case T_PAGEFLT: /* allow page faults in kernel mode */
- if (pageflttrap(frame, 0))
+ if (pageflttrap(frame, cr2, 0))
  return;
  goto we_re_toast;
 
@@ -333,6 +332,7 @@ usertrap(struct trapframe *frame)
 {
  struct proc *p = curproc;
  int type = (int)frame->tf_trapno;
+ uint64_t cr2 = rcr2();
  union sigval sv;
  int sig, code;
 
@@ -381,7 +381,7 @@ usertrap(struct trapframe *frame)
  break;
 
  case T_PAGEFLT: /* page fault */
- if (pageflttrap(frame, 1))
+ if (pageflttrap(frame, cr2, 1))
  goto out;
  /* FALLTHROUGH */
 

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Sebastien Marie-3
On Mon, Sep 14, 2020 at 01:25:03PM +0200, Mark Kettenis wrote:

> > Date: Sun, 13 Sep 2020 19:48:19 +0200
> > From: Sebastien Marie <[hidden email]>
> >
> > On Sun, Sep 13, 2020 at 04:49:48PM +0200, Sebastien Marie wrote:
> > > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > > > I'm no longer able to reproduce the corruption while building lang/go
> > > > with the diff below.  Something relevant to threading change in go since
> > > > march?
> > > >
> > > > Can someone try this diff and tell me if go and/or rust still fail?
> > >
> > > quickly tested with rustc build (nightly here), and it is failing at random places (not always at the same) with memory errors (signal 11, compiler ICE signal 6...)
> > >
> >
> > A first hint.
> >
> > With the help of deraadt@, it was found that disabling
> > uvm_map_inentry() call in usertrap() is enough to avoid the crashes.
> >
> > To be clear, I am using the following diff:
>
> The diff below fixes at (for amd64).
>
> What's happening is that uvm_map_inentry() may sleep to grab the lock
> of the map.  The fault address is read from cr2 in pageflttrap() which
> gets called after this check and if the check sleeps, cr2 is likely to
> be clobbered by a page fault in another process.
>
> Diff below fixes this by reading cr2 early and passing it to pageflttrap().
>
> ok?

it makes sens. and I can't trigger the crashes with rustc build anymore.

ok semarie@
 

>
> Index: arch/amd64/amd64/trap.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 trap.c
> --- arch/amd64/amd64/trap.c 19 Aug 2020 10:10:57 -0000 1.80
> +++ arch/amd64/amd64/trap.c 14 Sep 2020 11:17:35 -0000
> @@ -92,7 +92,7 @@
>  
>  #include "isa.h"
>  
> -int pageflttrap(struct trapframe *, int _usermode);
> +int pageflttrap(struct trapframe *, uint64_t, int _usermode);
>  void kerntrap(struct trapframe *);
>  void usertrap(struct trapframe *);
>  void ast(struct trapframe *);
> @@ -157,12 +157,11 @@ fault(const char *format, ...)
>   * if something was so broken that we should panic.
>   */
>  int
> -pageflttrap(struct trapframe *frame, int usermode)
> +pageflttrap(struct trapframe *frame, uint64_t cr2, int usermode)
>  {
>   struct proc *p = curproc;
>   struct pcb *pcb;
>   int error;
> - uint64_t cr2;
>   vaddr_t va;
>   struct vm_map *map;
>   vm_prot_t ftype;
> @@ -172,7 +171,6 @@ pageflttrap(struct trapframe *frame, int
>  
>   map = &p->p_vmspace->vm_map;
>   pcb = &p->p_addr->u_pcb;
> - cr2 = rcr2();
>   va = trunc_page((vaddr_t)cr2);
>  
>   KERNEL_LOCK();
> @@ -280,6 +278,7 @@ void
>  kerntrap(struct trapframe *frame)
>  {
>   int type = (int)frame->tf_trapno;
> + uint64_t cr2 = rcr2();
>  
>   verify_smap(__func__);
>   uvmexp.traps++;
> @@ -299,7 +298,7 @@ kerntrap(struct trapframe *frame)
>   /*NOTREACHED*/
>  
>   case T_PAGEFLT: /* allow page faults in kernel mode */
> - if (pageflttrap(frame, 0))
> + if (pageflttrap(frame, cr2, 0))
>   return;
>   goto we_re_toast;
>  
> @@ -333,6 +332,7 @@ usertrap(struct trapframe *frame)
>  {
>   struct proc *p = curproc;
>   int type = (int)frame->tf_trapno;
> + uint64_t cr2 = rcr2();
>   union sigval sv;
>   int sig, code;
>  
> @@ -381,7 +381,7 @@ usertrap(struct trapframe *frame)
>   break;
>  
>   case T_PAGEFLT: /* page fault */
> - if (pageflttrap(frame, 1))
> + if (pageflttrap(frame, cr2, 1))
>   goto out;
>   /* FALLTHROUGH */
>  

--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Theo de Raadt-2
In reply to this post by Mark Kettenis
i386 has the same problem.

Index: arch/i386/i386/trap.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/trap.c,v
retrieving revision 1.143
diff -u -p -u -r1.143 trap.c
--- arch/i386/i386/trap.c 19 Aug 2020 10:10:58 -0000 1.143
+++ arch/i386/i386/trap.c 14 Sep 2020 11:23:01 -0000
@@ -119,7 +119,7 @@ trap(struct trapframe *frame)
  vm_prot_t ftype;
  union sigval sv;
  caddr_t onfault;
- uint32_t cr2;
+ uint32_t cr2 = rcr2();
 
  uvmexp.traps++;
 
@@ -135,7 +135,7 @@ trap(struct trapframe *frame)
  if (trapdebug) {
  printf("trap %d code %x eip %x cs %x eflags %x cr2 %x cpl %x\n",
     frame->tf_trapno, frame->tf_err, frame->tf_eip,
-    frame->tf_cs, frame->tf_eflags, rcr2(), lapic_tpr);
+    frame->tf_cs, frame->tf_eflags, rcr2, lapic_tpr);
  printf("curproc %p\n", curproc);
  }
 #endif
@@ -182,7 +182,7 @@ trap(struct trapframe *frame)
  printf(" in %s mode\n", (type & T_USER) ? "user" : "supervisor");
  printf("trap type %d code %x eip %x cs %x eflags %x cr2 %x cpl %x\n",
     type, frame->tf_err, frame->tf_eip, frame->tf_cs,
-    frame->tf_eflags, rcr2(), lapic_tpr);
+    frame->tf_eflags, rcr2, lapic_tpr);
 
  panic("trap type %d, code=%x, pc=%x",
     type, frame->tf_err, frame->tf_eip);
@@ -333,7 +333,6 @@ trap(struct trapframe *frame)
  goto we_re_toast;
 
  pcb = &p->p_addr->u_pcb;
- cr2 = rcr2();
  KERNEL_LOCK();
  /* This will only trigger if SMEP is enabled */
  if (cr2 <= VM_MAXUSER_ADDRESS && frame->tf_err & PGEX_I)
@@ -353,7 +352,6 @@ trap(struct trapframe *frame)
  int error;
  int signal, sicode;
 
- cr2 = rcr2();
  KERNEL_LOCK();
  faultcommon:
  vm = p->p_vmspace;
@@ -434,11 +432,11 @@ trap(struct trapframe *frame)
 #endif
 
  case T_BPTFLT|T_USER: /* bpt instruction fault */
- sv.sival_int = rcr2();
+ sv.sival_int = rcr2;
  trapsignal(p, SIGTRAP, type &~ T_USER, TRAP_BRKPT, sv);
  break;
  case T_TRCTRAP|T_USER: /* trace trap */
- sv.sival_int = rcr2();
+ sv.sival_int = rcr2;
  trapsignal(p, SIGTRAP, type &~ T_USER, TRAP_TRACE, sv);
  break;
 

Reply | Threaded
Open this post in threaded view
|

Re: go/rust vs uvm_map_inentry()

Theo de Raadt-2
In reply to this post by Mark Kettenis
A similar fix for the "sh" cpu, which is in the landisk.

Index: sh/sh/trap.c
===================================================================
RCS file: /cvs/src/sys/arch/sh/sh/trap.c,v
retrieving revision 1.40
diff -u -p -u -r1.40 trap.c
--- sh/sh/trap.c 6 Sep 2019 12:22:01 -0000 1.40
+++ sh/sh/trap.c 14 Sep 2020 11:44:56 -0000
@@ -155,7 +155,7 @@ void
 general_exception(struct proc *p, struct trapframe *tf, uint32_t va)
 {
  int expevt = tf->tf_expevt;
- int tra;
+ int tra = _reg_read_4(SH_(TRA));
  int usermode = !KERNELMODE(tf->tf_ssr);
  union sigval sv;
 
@@ -191,7 +193,6 @@ general_exception(struct proc *p, struct
  case EXPEVT_TRAPA:
 #ifdef DDB
  /* Check for ddb request */
- tra = _reg_read_4(SH_(TRA));
  if (tra == (_SH_TRA_BREAK << 2) &&
     db_ktrap(expevt, tra, tf))
  return;
@@ -201,7 +202,6 @@ general_exception(struct proc *p, struct
  break;
  case EXPEVT_TRAPA | EXP_USER:
  /* Check for debugger break */
- tra = _reg_read_4(SH_(TRA));
  switch (tra) {
  case _SH_TRA_BREAK << 2:
  tf->tf_spc -= 2; /* back to the breakpoint address */