riscv-mit / riscv-isa-sim

RISC-V Functional ISA Simulator
Other
4 stars 1 forks source link

fork() causes spurious traps with new policy #14

Closed samfin closed 9 years ago

samfin commented 9 years ago

Return address is stored once, but loaded twice. Second load does not have correct tag.

ievans commented 9 years ago

Do we have a strategy to handle this? In Linux it seems that copy_process calls copy_thread with the stack pointer and stack size, but that doesn't perform the copy immediately (I assume it's COW?)

in arch/riscv/kernel/process.c:

           *childregs = *(current_pt_regs());                              
            if (usp) /* User fork */                                        
                    childregs->sp = usp;                                    
            if (clone_flags & CLONE_SETTLS)                                 
                    childregs->tp = childregs->a5;                          
            childregs->a0 = 0; /* Return value of fork() */                 
            p->thread.ra = (unsigned long)ret_from_fork;                    

So should we say that whenever your page is written by a different process and copied by the kernel because it is COW, the tags should be preserved by that kernel copy function?

ievans commented 9 years ago

http://lxr.free-electrons.com/source/kernel/fork.c#L1390

handled by the page fault handler http://lxr.free-electrons.com/source/mm/memory.c#L3269

which uses copy_user_page http://lxr.free-electrons.com/source/mm/memory.c#L2199

in x86 http://lxr.free-electrons.com/source/arch/x86/include/asm/page_32.h#L29 in riscv linux: arch/riscv/include/asm/page.h

#define clear_page(pgaddr)                      memset((pgaddr), 0, PAGE_SIZE)  |
#define copy_page(to, from)                     memcpy((to), (from), PAGE_SIZE) |
                                                                            |
#define clear_user_page(pgaddr, vaddr, page)    memset((pgaddr), 0, PAGE_SIZE)  |
#define copy_user_page(vto, vfrom, vaddr, topg) \                               |
                    memcpy((vto), (vfrom), PAGE_SIZE)

memcpy is riscv is under arch/riscv/lib/memcpy.S

My personal thought is that for now for readability we should have our own tagcpy which copies only tags and put a wrapper around the memcpy in copy_user_page.

jugonz commented 9 years ago

hmmm...can't we modify fork() in glibc? We could use settag to set the tag on %ra there and skip kernelspace entirely. I wonder if we should find other issues before modifying the OS. I like tagcpy().

ievans commented 9 years ago

I agree it's better not to modify the OS, but I think the answer is we can't/shouldn't do it in glibc. First off we want only the kernel to be making tagcpy() instructions so the security boundary is good for us, and second I don't think userspace actually has the visibility or permissions to copy all the state required. But it sounds like you're talking about one tag--isn't it true that the child process can return all the way up the stack and thus we want all of the tags to be maintained? (It also seems like we will want that later on when we have function pointer tags + such).

jugonz commented 9 years ago

I think this is resolved with only allowing the kernel to copy tags...anyone disagree? The use of a tagcpy is still to be determined...