[OpenSmalltalk/opensmalltalk-vm] 28630b: The extra +1 in size parameter passed to mprotect(...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[OpenSmalltalk/opensmalltalk-vm] 28630b: The extra +1 in size parameter passed to mprotect(...

Eliot Miranda-3
 
  Branch: refs/heads/Cog
  Home:   https://github.com/OpenSmalltalk/opensmalltalk-vm
  Commit: 28630b8c60c35096175f6d6fddb5cd91d7868882
      https://github.com/OpenSmalltalk/opensmalltalk-vm/commit/28630b8c60c35096175f6d6fddb5cd91d7868882
  Author: Ronie Salgado <[hidden email]>
  Date:   2016-12-20 (Tue, 20 Dec 2016)

  Changed paths:
    M platforms/unix/vm/sqUnixSpurMemory.c
    M platforms/win32/vm/sqWin32SpurAlloc.c

  Log Message:
  -----------
  The extra +1 in size parameter passed to mprotect(Unix) and VirtualProtect(Windows).


  Commit: 4d732c6db375be18a96a3c850f60a14538d2d867
      https://github.com/OpenSmalltalk/opensmalltalk-vm/commit/4d732c6db375be18a96a3c850f60a14538d2d867
  Author: Ronie Salgado <[hidden email]>
  Date:   2016-12-21 (Wed, 21 Dec 2016)

  Changed paths:
    M platforms/unix/vm/sqUnixSpurMemory.c

  Log Message:
  -----------
  Computing the size after rounding down the start address.


  Commit: 11880398af1da706e7b61802c11985f12bef280e
      https://github.com/OpenSmalltalk/opensmalltalk-vm/commit/11880398af1da706e7b61802c11985f12bef280e
  Author: Nicolas Cellier <[hidden email]>
  Date:   2016-12-22 (Thu, 22 Dec 2016)

  Changed paths:
    M platforms/unix/vm/sqUnixSpurMemory.c
    M platforms/win32/vm/sqWin32SpurAlloc.c

  Log Message:
  -----------
  Merge pull request #109 from ronsaldo/MProtectBug

Bug when calling mprotect and VirtualAlloc with the SpurMemoryManager


Compare: https://github.com/OpenSmalltalk/opensmalltalk-vm/compare/28293aa6b85e...11880398af1d
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] 28630b: The extra +1 in size parameter passed to mprotect(...

Ben Coman
 
Just curious about the behaviour and performance of mprotect and VirtualProtect.  Would it be feasible to use on each FFI callout so that aberrant memory access from C code cannot corrupt the Smalltalk Image, but generate an exception that could be handled in-Image?

cheers -ben

On Fri, Dec 23, 2016 at 12:37 AM, GitHub <[hidden email]> wrote:
 
  Branch: refs/heads/Cog
  Home:   https://github.com/OpenSmalltalk/opensmalltalk-vm
  Commit: 28630b8c60c35096175f6d6fddb5cd91d7868882
      https://github.com/OpenSmalltalk/opensmalltalk-vm/commit/28630b8c60c35096175f6d6fddb5cd91d7868882
  Author: Ronie Salgado <[hidden email]>
  Date:   2016-12-20 (Tue, 20 Dec 2016)

  Changed paths:
    M platforms/unix/vm/sqUnixSpurMemory.c
    M platforms/win32/vm/sqWin32SpurAlloc.c

  Log Message:
  -----------
  The extra +1 in size parameter passed to mprotect(Unix) and VirtualProtect(Windows).


  Commit: 4d732c6db375be18a96a3c850f60a14538d2d867
      https://github.com/OpenSmalltalk/opensmalltalk-vm/commit/4d732c6db375be18a96a3c850f60a14538d2d867
  Author: Ronie Salgado <[hidden email]>
  Date:   2016-12-21 (Wed, 21 Dec 2016)

  Changed paths:
    M platforms/unix/vm/sqUnixSpurMemory.c

  Log Message:
  -----------
  Computing the size after rounding down the start address.


  Commit: 11880398af1da706e7b61802c11985f12bef280e
      https://github.com/OpenSmalltalk/opensmalltalk-vm/commit/11880398af1da706e7b61802c11985f12bef280e
  Author: Nicolas Cellier <[hidden email]>
  Date:   2016-12-22 (Thu, 22 Dec 2016)

  Changed paths:
    M platforms/unix/vm/sqUnixSpurMemory.c
    M platforms/win32/vm/sqWin32SpurAlloc.c

  Log Message:
  -----------
  Merge pull request #109 from ronsaldo/MProtectBug

Bug when calling mprotect and VirtualAlloc with the SpurMemoryManager


Compare: https://github.com/OpenSmalltalk/opensmalltalk-vm/compare/28293aa6b85e...11880398af1d

Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] 28630b: The extra +1 in size parameter passed to mprotect(...

Holger Freyther
 

> On 23 Dec 2016, at 01:46, Ben Coman <[hidden email]> wrote:
>
> Just curious about the behaviour and performance of mprotect and VirtualProtect.  Would it be feasible to use on each FFI callout so that aberrant memory access from C code cannot corrupt the Smalltalk Image, but generate an exception that could be handled in-Image?

Hi Ben,

* syscall overhead (e.g. mprotect is not part of the VDSO)
* lock/unlock of Page Table Entries (PTEs)
* walking page table entries
* modifying page table entries
* merging/combining virtual memory areas now (allocation)
* cache/tlb being invalidated


E.g. for FreeBSD the chain is like this with a lot of details on the way

* sys_mprotect[1] the syscall after the dispatch
* vm_map_protect[2] for the VM subsystem
* AMD64's pmap_protect[3] implementation various ways to invalidate the TLB in it

holger



[1] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/vm/vm_mmap.c#L657
[2] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/vm/vm_map.c#L1924
[3] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/amd64/amd64/pmap.c#L3906
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] 28630b: The extra +1 in size parameter passed to mprotect(...

Ben Coman
 
On Sat, Dec 24, 2016 at 2:33 AM, Holger Freyther <[hidden email]> wrote:

>
>
>
> > On 23 Dec 2016, at 01:46, Ben Coman <[hidden email]> wrote:
> >
> > Just curious about the behaviour and performance of mprotect and VirtualProtect.  Would it be feasible to use on each FFI callout so that aberrant memory access from C code cannot corrupt the Smalltalk Image, but generate an exception that could be handled in-Image?
>
> Hi Ben,
>
> * syscall overhead (e.g. mprotect is not part of the VDSO)
> * lock/unlock of Page Table Entries (PTEs)
> * walking page table entries
> * modifying page table entries
> * merging/combining virtual memory areas now (allocation)
> * cache/tlb being invalidated
>
>
> E.g. for FreeBSD the chain is like this with a lot of details on the way
>
> * sys_mprotect[1] the syscall after the dispatch
> * vm_map_protect[2] for the VM subsystem
> * AMD64's pmap_protect[3] implementation various ways to invalidate the TLB in it
>
> holger
>
>
>
> [1] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/vm/vm_mmap.c#L657
> [2] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/vm/vm_map.c#L1924
> [3] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/amd64/amd64/pmap.c#L3906


Thanks Holger.  A bit hard to soak up in a first sitting, but
interesting to get a feel for how it works under the covers.
I summarised the loops, so it seems it needs to modify every page table entry.

sys_mprotect()

   vm_map_protect()
      /* Make a first pass to check for protection violations.*/
      while ((current != &map->header) && (current->start < end))
      {... current = current->next; ...}

      /* Do an accounting pass for private read-only mappings that now
will do cow due to allowed write (e.g. debugger sets breakpoint on
text segment) */
     for (current = entry; (current != &map->header) &&
(current->start < end); current = current->next)
     {...}

     /* Go back and fix up protections. [Note that clipping is not
necessary the second time.] */
     current = entry;
     while ((current != &map->header) && (current->start < end))
     {
         ...
         pmap_protect()
              for (; sva < eva; sva = va_next) {
                 va_next = (sva + NBPDR) & ~PDRMASK;
                 for (pte = pmap_pde_to_pte(pde, sva); sva != va_next;
pte++,   sva += PAGE_SIZE)
                    {...flip page table bits...}
         current = current->next;
      }

The documentation of variables in the code there is not great.  I'm
guessing at variable purposes...
sva = start virtual address
eva = end virtual address
pte = page table entry
NBPDR=number bytes / page directory

So maybe useful for debugging, but not practical for every FFI call (??)
cheers -ben
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] 28630b: The extra +1 in size parameter passed to mprotect(...

Eliot Miranda-2
 
Hi Ben,

On Fri, Dec 23, 2016 at 8:03 PM, Ben Coman <[hidden email]> wrote:

On Sat, Dec 24, 2016 at 2:33 AM, Holger Freyther <[hidden email]> wrote:
>
>
>
> > On 23 Dec 2016, at 01:46, Ben Coman <[hidden email]> wrote:
> >
> > Just curious about the behaviour and performance of mprotect and VirtualProtect.  Would it be feasible to use on each FFI callout so that aberrant memory access from C code cannot corrupt the Smalltalk Image, but generate an exception that could be handled in-Image?
>
> Hi Ben,
>
> * syscall overhead (e.g. mprotect is not part of the VDSO)
> * lock/unlock of Page Table Entries (PTEs)
> * walking page table entries
> * modifying page table entries
> * merging/combining virtual memory areas now (allocation)
> * cache/tlb being invalidated
>
>
> E.g. for FreeBSD the chain is like this with a lot of details on the way
>
> * sys_mprotect[1] the syscall after the dispatch
> * vm_map_protect[2] for the VM subsystem
> * AMD64's pmap_protect[3] implementation various ways to invalidate the TLB in it
>
> holger
>
>
>
> [1] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/vm/vm_mmap.c#L657
> [2] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/vm/vm_map.c#L1924
> [3] https://github.com/freebsd/freebsd/blob/releng/10.3/sys/amd64/amd64/pmap.c#L3906


Thanks Holger.  A bit hard to soak up in a first sitting, but
interesting to get a feel for how it works under the covers.
I summarised the loops, so it seems it needs to modify every page table entry.

sys_mprotect()

   vm_map_protect()
      /* Make a first pass to check for protection violations.*/
      while ((current != &map->header) && (current->start < end))
      {... current = current->next; ...}

      /* Do an accounting pass for private read-only mappings that now
will do cow due to allowed write (e.g. debugger sets breakpoint on
text segment) */
     for (current = entry; (current != &map->header) &&
(current->start < end); current = current->next)
     {...}

     /* Go back and fix up protections. [Note that clipping is not
necessary the second time.] */
     current = entry;
     while ((current != &map->header) && (current->start < end))
     {
         ...
         pmap_protect()
              for (; sva < eva; sva = va_next) {
                 va_next = (sva + NBPDR) & ~PDRMASK;
                 for (pte = pmap_pde_to_pte(pde, sva); sva != va_next;
pte++,   sva += PAGE_SIZE)
                    {...flip page table bits...}
         current = current->next;
      }

The documentation of variables in the code there is not great.  I'm
guessing at variable purposes...
sva = start virtual address
eva = end virtual address
pte = page table entry
NBPDR=number bytes / page directory

So maybe useful for debugging, but not practical for every FFI call (??)

Well, even if it was only switched on for debugging one would need to /not/ protect pages containing objects passed through the FFI to be written to.  I don't see an easy way of doing this.  One might think one only had to scan the objects in the current FFI call and not protect them.  But how would one distinguish between those just read from and those written to?

Also Spur's support for pinned objects means that one can pass pinned objects out to foreign code and allow the foreign code to access the object after the call returns.  I don't know how to not protect these objects.  If one starts to make exceptions for all objects in the current call and all pinned objects pretty soon one has lots of the heap unprotected.

But there's another principle here.  While a debugging idea may seem really useful its best not to implement it until one really needs it.  Debugging features add complexity and if done wrong can introduce differences between normal and debug behaviour.  There's an opportunity cost I'n implementing them.  So let's not add debug features until we really need them.

Happy solstice!
_,,,^..^,,,_
best, Eliot