Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7677036:
Enable the service runtime to use a zero-based sandbox on Linux. (Closed)
|
Created:
9 years, 4 months ago by Brad Chen Modified:
9 years, 4 months ago CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionEnable the service runtime to use a zero-based sandbox on Linux.
32-bit ARM and x86 only for now. I will do x86-64 once these changes have settled. I have tested these changes by copying the modified .c files into a Chromium build. There are still some tests that are not building properly, but I didn't want to delay getting this part out for review.
BUG=chromium:92964, nativeclient:480
TEST=manual for now
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=6521
Patch Set 1 #
Total comments: 23
Patch Set 2 : Answer early feedback, fix known build problems #Patch Set 3 : Better protection for low pages #Patch Set 4 : Now should green all nacl bots #Patch Set 5 : Use mmap instead of mprotect on low pages #Patch Set 6 : Fix usage of mmap to protect lower memory #Patch Set 7 : minor fixes for the bots #Patch Set 8 : more fixes for bots #
Total comments: 48
Patch Set 9 : Fixes for Mark and Bennet's reviews #
Total comments: 8
Patch Set 10 : Fixes for Bennet's review #
Messages
Total messages: 20 (0 generated)
I will have some additional fixes for building tests before this is ready for commit.
On 18 August 2011 15:42, <bradchen@google.com> wrote: > Reviewers: bsy, bbudge1, Mark Seaborn, > > Message: > I will have some additional fixes for building tests before this is ready > for > commit. > > Description: > Enable the service runtime to use a zero-based sandbox on Linux. > 32-bit ARM and x86 only for now. I will do x86-64 once these changes have > settled. > Why would you change x86-64? Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
Mostly because I've been pushing that we can get a significantly higher performance x86-64 sandbox scheme that way, I suspect. On Thu, Aug 18, 2011 at 3:54 PM, Mark Seaborn <mseaborn@chromium.org> wrote: > On 18 August 2011 15:42, <bradchen@google.com> wrote: > >> Reviewers: bsy, bbudge1, Mark Seaborn, >> >> Message: >> I will have some additional fixes for building tests before this is ready >> for >> commit. >> >> Description: >> Enable the service runtime to use a zero-based sandbox on Linux. >> 32-bit ARM and x86 only for now. I will do x86-64 once these changes have >> settled. >> > > Why would you change x86-64? > > Mark > > -- > You received this message because you are subscribed to the Google Groups > "Native-Client-Reviews" group. > To post to this group, send email to > native-client-reviews@googlegroups.com. > To unsubscribe from this group, send email to > native-client-reviews+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/native-client-reviews?hl=en. > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
> I will have some additional fixes for building tests before this is > ready for commit. OK, I won't do a full review yet if this is not complete. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:24: const void* ONE_MEGABYTE = (void *)1024*1024; Nit: " *" style not "* " in this code. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:39: /* zero-based sandbox not pre-reserved */ Capitalise the start of the sentence, please. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:42: /* sanity check zero sandbox base address */ Capitalise. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:27: if (!NaCl_find_prereserved_sandbox_memory(mem, addrsp_size)) { It would be more readable to put the success branch for this function call first. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:32: if (0 == *mem || ONE_MEGABYTE < *mem) { Why are you checking for 1MB? What's the significance of this value? http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:94: if (0 == start_addr) { Wrong indentation http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:95: /* Attempt to protect one page at a time. It is normal for */ Use the commenting style: /* * ... */ http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:96: /* these attempts to fail if the page is already protected. */ I don't understand the purpose of this block of code. What is the state of the bottom 64k of address space before and after this block? Why is it OK to ignore errors here? http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:113: NaClLog(LOG_ERROR, ("NaClMemoryProtection: " This is not indented from the "if" http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... File src/trusted/service_runtime/linux/sel_memory.c (right): http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... src/trusted/service_runtime/linux/sel_memory.c:48: void* nacl_helper_so = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD); " *" style http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... src/trusted/service_runtime/linux/sel_memory.c:49: base_addr_func* nacl_helper_get_base_addr; " *" style http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... src/trusted/service_runtime/linux/sel_memory.c:56: if (!nacl_helper_so) return 0; Prefer: if (x) { blah }
On 18 August 2011 16:03, David Sehr <sehr@google.com> wrote: > Mostly because I've been pushing that we can get a significantly higher > performance x86-64 sandbox scheme that way, I suspect. > So that would involve generating different code for x86-64, without the %r15-indexed accesses? I assume just setting %r15 to zero doesn't make adding %r15 to addresses go faster. I assume we can't reserve the bottom 4GB on x86-64 Windows so we'd be using different sandboxed code on Windows and Linux. Mark > On Thu, Aug 18, 2011 at 3:54 PM, Mark Seaborn <mseaborn@chromium.org>wrote: > >> On 18 August 2011 15:42, <bradchen@google.com> wrote: >> >>> Reviewers: bsy, bbudge1, Mark Seaborn, >>> >>> Message: >>> I will have some additional fixes for building tests before this is ready >>> for >>> commit. >>> >>> Description: >>> Enable the service runtime to use a zero-based sandbox on Linux. >>> 32-bit ARM and x86 only for now. I will do x86-64 once these changes have >>> settled. >>> >> >> Why would you change x86-64? >> >> Mark >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
Yes, precisely. Removing basically all the overhead of data sandboxing by removing the r15 constraint and address arithmetic. Apparently we can use the bottom 4GB on Win7, with some other new contortions. I don't know about Mac. On Thu, Aug 18, 2011 at 4:14 PM, Mark Seaborn <mseaborn@chromium.org> wrote: > On 18 August 2011 16:03, David Sehr <sehr@google.com> wrote: > >> Mostly because I've been pushing that we can get a significantly higher >> performance x86-64 sandbox scheme that way, I suspect. >> > > So that would involve generating different code for x86-64, without the > %r15-indexed accesses? I assume just setting %r15 to zero doesn't make > adding %r15 to addresses go faster. > > I assume we can't reserve the bottom 4GB on x86-64 Windows so we'd be using > different sandboxed code on Windows and Linux. > > Mark > > >> On Thu, Aug 18, 2011 at 3:54 PM, Mark Seaborn <mseaborn@chromium.org>wrote: >> >>> On 18 August 2011 15:42, <bradchen@google.com> wrote: >>> >>>> Reviewers: bsy, bbudge1, Mark Seaborn, >>>> >>>> Message: >>>> I will have some additional fixes for building tests before this is >>>> ready for >>>> commit. >>>> >>>> Description: >>>> Enable the service runtime to use a zero-based sandbox on Linux. >>>> 32-bit ARM and x86 only for now. I will do x86-64 once these changes >>>> have >>>> settled. >>>> >>> >>> Why would you change x86-64? >>> >>> Mark >>> >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
On 18 August 2011 16:17, David Sehr <sehr@google.com> wrote: > Yes, precisely. Removing basically all the overhead of data sandboxing by > removing the r15 constraint and address arithmetic. > Apparently we can use the bottom 4GB on Win7, with some other new > contortions. I don't know about Mac. > So we can't on 64-bit Windows XP? Do you know what the difference is in Windows 7? Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
Thanks for the early feedback. PTAL I've uploaded additional changes such that "scons --mode=opt-linux,nacl" now builds on Linux. Try jobs are in progress. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:24: const void* ONE_MEGABYTE = (void *)1024*1024; On 2011/08/18 23:09:45, Mark Seaborn wrote: > Nit: " *" style not "* " in this code. Done. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:39: /* zero-based sandbox not pre-reserved */ On 2011/08/18 23:09:45, Mark Seaborn wrote: > Capitalise the start of the sentence, please. Done. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:42: /* sanity check zero sandbox base address */ On 2011/08/18 23:09:45, Mark Seaborn wrote: > Capitalise. Done. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:32: if (0 == *mem || ONE_MEGABYTE < *mem) { I've added a better explanation in the comment. On 2011/08/18 23:09:45, Mark Seaborn wrote: > Why are you checking for 1MB? What's the significance of this value? http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:94: if (0 == start_addr) { Oops! Fixed. On 2011/08/18 23:09:45, Mark Seaborn wrote: > Wrong indentation http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:95: /* Attempt to protect one page at a time. It is normal for */ On 2011/08/18 23:09:45, Mark Seaborn wrote: > Use the commenting style: > /* > * ... > */ Done. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:96: /* these attempts to fail if the page is already protected. */ Sounds like the comment above didn't adequate explain things for you. Let's talk F2F so I can figure out what would. On 2011/08/18 23:09:45, Mark Seaborn wrote: > I don't understand the purpose of this block of code. > > What is the state of the bottom 64k of address space before and after this > block? Why is it OK to ignore errors here? http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:113: NaClLog(LOG_ERROR, ("NaClMemoryProtection: " On 2011/08/18 23:09:45, Mark Seaborn wrote: > This is not indented from the "if" Done. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... File src/trusted/service_runtime/linux/sel_memory.c (right): http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... src/trusted/service_runtime/linux/sel_memory.c:48: void* nacl_helper_so = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD); On 2011/08/18 23:09:45, Mark Seaborn wrote: > " *" style Done. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... src/trusted/service_runtime/linux/sel_memory.c:49: base_addr_func* nacl_helper_get_base_addr; On 2011/08/18 23:09:45, Mark Seaborn wrote: > " *" style Done. http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... src/trusted/service_runtime/linux/sel_memory.c:56: if (!nacl_helper_so) return 0; On 2011/08/18 23:09:45, Mark Seaborn wrote: > Prefer: > if (x) { > blah > } Done.
I've uploaded improved code to protect low pages. On 2011/08/19 00:24:35, Brad Chen wrote: > Thanks for the early feedback. PTAL > > I've uploaded additional changes such that "scons --mode=opt-linux,nacl" now > builds on Linux. Try jobs are in progress. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:24: const void* > ONE_MEGABYTE = (void *)1024*1024; > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > Nit: " *" style not "* " in this code. > > Done. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:39: /* zero-based > sandbox not pre-reserved */ > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > Capitalise the start of the sentence, please. > > Done. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:42: /* sanity check > zero sandbox base address */ > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > Capitalise. > > Done. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:32: if (0 == *mem > || ONE_MEGABYTE < *mem) { > I've added a better explanation in the comment. > > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > Why are you checking for 1MB? What's the significance of this value? > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:94: if (0 == > start_addr) { > Oops! Fixed. > > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > Wrong indentation > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:95: /* Attempt to > protect one page at a time. It is normal for */ > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > Use the commenting style: > > /* > > * ... > > */ > > Done. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:96: /* these > attempts to fail if the page is already protected. */ > Sounds like the comment above didn't adequate explain things for you. Let's talk > F2F so I can figure out what would. > > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > I don't understand the purpose of this block of code. > > > > What is the state of the bottom 64k of address space before and after this > > block? Why is it OK to ignore errors here? > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arc... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:113: > NaClLog(LOG_ERROR, ("NaClMemoryProtection: " > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > This is not indented from the "if" > > Done. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... > File src/trusted/service_runtime/linux/sel_memory.c (right): > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... > src/trusted/service_runtime/linux/sel_memory.c:48: void* nacl_helper_so = > dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD); > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > " *" style > > Done. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... > src/trusted/service_runtime/linux/sel_memory.c:49: base_addr_func* > nacl_helper_get_base_addr; > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > " *" style > > Done. > > http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/lin... > src/trusted/service_runtime/linux/sel_memory.c:56: if (!nacl_helper_so) return > 0; > On 2011/08/18 23:09:45, Mark Seaborn wrote: > > Prefer: > > if (x) { > > blah > > } > > Done.
Bennet, Mark, please prepare yourselves psychologically to spend some time reviewing this early next week. There are a few more steps to go and this really needs to be in for M15. Brad On Fri, Aug 19, 2011 at 9:14 AM, <bradchen@google.com> wrote: > I've uploaded improved code to protect low pages. > > > On 2011/08/19 00:24:35, Brad Chen wrote: > >> Thanks for the early feedback. PTAL >> > > I've uploaded additional changes such that "scons --mode=opt-linux,nacl" >> now >> builds on Linux. Try jobs are in progress. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c> > >> File src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c (right): >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c#**newcode24<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c#newcode24> > >> src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c:**24: const >> void* >> ONE_MEGABYTE = (void *)1024*1024; >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > Nit: " *" style not "* " in this code. >> > > Done. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c#**newcode39<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c#newcode39> > >> src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c:**39: /* >> zero-based >> sandbox not pre-reserved */ >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > Capitalise the start of the sentence, please. >> > > Done. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c#**newcode42<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c#newcode42> > >> src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c:**42: /* >> sanity check >> zero sandbox base address */ >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > Capitalise. >> > > Done. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c> > >> File src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c >> (right): >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode32<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode32> > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:32: if >> (0 == >> > *mem > >> || ONE_MEGABYTE < *mem) { >> I've added a better explanation in the comment. >> > > On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > Why are you checking for 1MB? What's the significance of this value? >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode94<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode94> > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:94: if >> (0 == >> start_addr) { >> Oops! Fixed. >> > > On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > Wrong indentation >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode95<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode95> > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:95: /* >> Attempt >> > to > >> protect one page at a time. It is normal for */ >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > Use the commenting style: >> > /* >> > * ... >> > */ >> > > Done. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode96<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode96> > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:96: /* >> these >> attempts to fail if the page is already protected. */ >> Sounds like the comment above didn't adequate explain things for you. >> Let's >> > talk > >> F2F so I can figure out what would. >> > > On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > I don't understand the purpose of this block of code. >> > >> > What is the state of the bottom 64k of address space before and after >> this >> > block? Why is it OK to ignore errors here? >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode113<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode113> > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:113: >> NaClLog(LOG_ERROR, ("NaClMemoryProtection: " >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > This is not indented from the "if" >> > > Done. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/linux/**sel_memory.c<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c> > >> File src/trusted/service_runtime/**linux/sel_memory.c (right): >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/linux/**sel_memory.c#newcode48<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c#newcode48> > >> src/trusted/service_runtime/**linux/sel_memory.c:48: void* nacl_helper_so >> = >> dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD); >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > " *" style >> > > Done. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/linux/**sel_memory.c#newcode49<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c#newcode49> > >> src/trusted/service_runtime/**linux/sel_memory.c:49: base_addr_func* >> nacl_helper_get_base_addr; >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > " *" style >> > > Done. >> > > > http://codereview.chromium.**org/7677036/diff/1/src/** > trusted/service_runtime/linux/**sel_memory.c#newcode56<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c#newcode56> > >> src/trusted/service_runtime/**linux/sel_memory.c:56: if (!nacl_helper_so) >> return >> 0; >> On 2011/08/18 23:09:45, Mark Seaborn wrote: >> > Prefer: >> > if (x) { >> > blah >> > } >> > > Done. >> > > > > http://codereview.chromium.**org/7677036/<http://codereview.chromium.org/7677... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
The problems I noted with my mmap calls are now fixed. Please have a look. On 2011/08/20 14:57:18, Brad Chen wrote: > Bennet, Mark, please prepare yourselves psychologically to spend some time > reviewing this early next week. There are a few more steps to go and this > really needs to be in for M15. > > Brad > > On Fri, Aug 19, 2011 at 9:14 AM, <mailto:bradchen@google.com> wrote: > > > I've uploaded improved code to protect low pages. > > > > > > On 2011/08/19 00:24:35, Brad Chen wrote: > > > >> Thanks for the early feedback. PTAL > >> > > > > I've uploaded additional changes such that "scons --mode=opt-linux,nacl" > >> now > >> builds on Linux. Try jobs are in progress. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c> > > > >> File src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c (right): > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c#**newcode24<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c#newcode24> > > > >> src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c:**24: const > >> void* > >> ONE_MEGABYTE = (void *)1024*1024; > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > Nit: " *" style not "* " in this code. > >> > > > > Done. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c#**newcode39<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c#newcode39> > > > >> src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c:**39: /* > >> zero-based > >> sandbox not pre-reserved */ > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > Capitalise the start of the sentence, please. > >> > > > > Done. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**arm/sel_addrspace_arm.c#**newcode42<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c#newcode42> > > > >> src/trusted/service_runtime/**arch/arm/sel_addrspace_arm.c:**42: /* > >> sanity check > >> zero sandbox base address */ > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > Capitalise. > >> > > > > Done. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c> > > > >> File src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c > >> (right): > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode32<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode32> > > > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:32: if > >> (0 == > >> > > *mem > > > >> || ONE_MEGABYTE < *mem) { > >> I've added a better explanation in the comment. > >> > > > > On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > Why are you checking for 1MB? What's the significance of this value? > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode94<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode94> > > > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:94: if > >> (0 == > >> start_addr) { > >> Oops! Fixed. > >> > > > > On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > Wrong indentation > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode95<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode95> > > > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:95: /* > >> Attempt > >> > > to > > > >> protect one page at a time. It is normal for */ > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > Use the commenting style: > >> > /* > >> > * ... > >> > */ > >> > > > > Done. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode96<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode96> > > > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:96: /* > >> these > >> attempts to fail if the page is already protected. */ > >> Sounds like the comment above didn't adequate explain things for you. > >> Let's > >> > > talk > > > >> F2F so I can figure out what would. > >> > > > > On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > I don't understand the purpose of this block of code. > >> > > >> > What is the state of the bottom 64k of address space before and after > >> this > >> > block? Why is it OK to ignore errors here? > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/arch/**x86_32/sel_addrspace_x86_32.c#**newcode113<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c#newcode113> > > > >> src/trusted/service_runtime/**arch/x86_32/sel_addrspace_x86_**32.c:113: > >> NaClLog(LOG_ERROR, ("NaClMemoryProtection: " > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > This is not indented from the "if" > >> > > > > Done. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/linux/**sel_memory.c<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c> > > > >> File src/trusted/service_runtime/**linux/sel_memory.c (right): > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/linux/**sel_memory.c#newcode48<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c#newcode48> > > > >> src/trusted/service_runtime/**linux/sel_memory.c:48: void* nacl_helper_so > >> = > >> dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD); > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > " *" style > >> > > > > Done. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/linux/**sel_memory.c#newcode49<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c#newcode49> > > > >> src/trusted/service_runtime/**linux/sel_memory.c:49: base_addr_func* > >> nacl_helper_get_base_addr; > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > " *" style > >> > > > > Done. > >> > > > > > > http://codereview.chromium.**org/7677036/diff/1/src/** > > > trusted/service_runtime/linux/**sel_memory.c#newcode56<http://codereview.chromium.org/7677036/diff/1/src/trusted/service_runtime/linux/sel_memory.c#newcode56> > > > >> src/trusted/service_runtime/**linux/sel_memory.c:56: if (!nacl_helper_so) > >> return > >> 0; > >> On 2011/08/18 23:09:45, Mark Seaborn wrote: > >> > Prefer: > >> > if (x) { > >> > blah > >> > } > >> > > > > Done. > >> > > > > > > > > > http://codereview.chromium.**org/7677036/%3Chttp://codereview.chromium.org/76...> > > > > -- > You received this message because you are subscribed to the Google Groups > "Native-Client-Reviews" group. > To post to this group, send email to mailto:native-client-reviews@googlegroups.com. > To unsubscribe from this group, send email to > mailto:native-client-reviews+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/native-client-reviews?hl=en. >
Just style issues really... http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:50: /* It should be within a few pages above the 64KB boundary. See */ Nit: Comment style http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:20: #if NACL_LINUX && NACL_BUILD_SUBARCH == 32 You're already in the x86_32 file. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:27: #if NACL_LINUX && NACL_BUILD_SUBARCH == 32 You're already in the x86_32 file. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:36: /* Sanity check zero sandbox base address. */ Nit: Comment style is /* * blah */ http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:113: /* Attempt to protect one page at a time. It is normal for Nit: Comment style is /* * blah */ http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:118: for (page_addr = NACL_PAGESIZE; page_addr < NACL_SYSCALL_START_ADDR; I'd put each for() clause on a different line if they overflow. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:121: MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, -1, 0); Maybe MAP_NORESERVE too, just in case? http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:125: /* Normal; this is an invalid page for this process and */ Comment style http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/linux/sel_memory.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:48: void *nacl_helper_so = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD); I think it would be been neater to pass the sandbox address as an argument via NaClMainForChromium() in sel_main_chrome.c rather than using global state via dlopen(), but the latter is OK. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:59: tmpint = (uintptr_t)dlsym(nacl_helper_so, "nacl_helper_get_1G_address"); BTW, the usual style in service_runtime is a space after the cast. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:60: nacl_helper_get_base_addr = (base_addr_func*)tmpint; Nit: Space before '*' You could just do a double cast and remove tmpint. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:66: if (!base_addr) { This is a bit odd because one might expect the base address to be zero if the zero-based sandbox were successfully reserved. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/service_runtime.gyp (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/service_runtime.gyp:261: "-ldl", Nit: use single quotes. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/service_runtime.gyp:265: ] Nit: add trailing comma
a bug and some nits. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:53: if (0 == *mem || ONE_MEGABYTE < *mem) { just assigned to *mem NACL_TRAMPOLINE_START, so always false? *unless* NaCl_find_prereserved_sandbox_memory changed it? this seems confusing. and looking at NaCl_f_p_s_m, *p = 0 occurs early on, so it *always* overwrites, and the initialization above is a no-op. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:64: result = NaCl_page_alloc_at_addr(mem, addrsp_size); since NaCl_find_prereserved_sandbox_memory had zeroed out *mem, we aren't allocating at NACL_TRAMPOLINE_START like it used to. this probably breaks arm code. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/linux/sel_memory.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:46: size_t num_bytes) { UNREFERENCED_PARAMETER(num_bytes); http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:59: tmpint = (uintptr_t)dlsym(nacl_helper_so, "nacl_helper_get_1G_address"); style: space between (type) and expr http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:60: nacl_helper_get_base_addr = (base_addr_func*)tmpint; style: spaces in (type *) expr; http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:62: if (!nacl_helper_get_base_addr) { if (NULL == nacl_helper_get_base_addr) { http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:71: *p = (void *)base_addr; style nit: space in (void *) base_addr;
another suggestion http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:59: } if we find memory, we should ensure squatting between vm.mmap_min_addr the same as for x86-32 linux. at least a TODO here please http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:113: /* Attempt to protect one page at a time. It is normal for there's no reason to do it one page at a time. we need to cover vm_min_addr up to NACL_SYSCALL_START_ADDR (or equivalently NACL_TRAMPOLINE_START -- we really shouldn't have both), so why not: for (page_addr = NACL_PAGESIZE; page_addr < NACL_SYSCALL_START_ADDR; page_addr += NACL_PAGESIZE) { mmap_rval = mmap((void *) page_addr, NACL_SYSCALL_START_ADDR - page_addr, PROT_NONE, MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, -1, 0); if (page_addr == (uintptr_t) mmap_rval) { /* Success */ break; } else if (MAP_FAILED .... etc etc... so that the whole region is squatted upon by the first successful mmap call? there's no need to map in 64K at a time on linux. (actually, probably not on windows either in this particular case, since we don't allow user code to map over this region of memory.)
One small code style suggestion. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:136: } else { You could move the 'if' statement here: else if ((err = .. Or for better readability, you could move the assignment out of the 'if', making it the first statement in the block, followed by "if (err != 0) {...
PTAL http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:50: /* It should be within a few pages above the 64KB boundary. See */ On 2011/08/23 18:26:36, Mark Seaborn wrote: > Nit: Comment style Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:53: if (0 == *mem || ONE_MEGABYTE < *mem) { I moved initialization into else clause where it is useful. On 2011/08/23 18:31:23, bsy wrote: > just assigned to *mem NACL_TRAMPOLINE_START, so always false? *unless* > NaCl_find_prereserved_sandbox_memory changed it? this seems confusing. > > and looking at NaCl_f_p_s_m, *p = 0 occurs early on, so it *always* overwrites, > and the initialization above is a no-op. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:59: } I think this is already taken care of. You need to look at the Chrome side as well, chrome/nacl/nacl_helper_bootstrap_linux.c and chrome/nacl.gypi. The nacl_helper_bootstrap loads at 64K, and the code below protects up to 64K, so everything should be covered. On 2011/08/23 18:43:19, bsy wrote: > if we find memory, we should ensure squatting between vm.mmap_min_addr the same > as for x86-32 linux. at least a TODO here please http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:64: result = NaCl_page_alloc_at_addr(mem, addrsp_size); If *mem is zero after NaCl_fpsm(), then we will return above with LOAD_NO_MEMORY. *mem is non-zero for all other paths. I can add an assert (0 != *mem) here if you like. On 2011/08/23 18:31:23, bsy wrote: > since NaCl_find_prereserved_sandbox_memory had zeroed out *mem, we aren't > allocating at NACL_TRAMPOLINE_START like it used to. this probably breaks arm > code. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:20: #if NACL_LINUX && NACL_BUILD_SUBARCH == 32 On 2011/08/23 18:26:36, Mark Seaborn wrote: > You're already in the x86_32 file. Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:27: #if NACL_LINUX && NACL_BUILD_SUBARCH == 32 On 2011/08/23 18:26:36, Mark Seaborn wrote: > You're already in the x86_32 file. Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:36: /* Sanity check zero sandbox base address. */ On 2011/08/23 18:26:36, Mark Seaborn wrote: > Nit: Comment style is > /* > * blah > */ Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:113: /* Attempt to protect one page at a time. It is normal for On 2011/08/23 18:26:36, Mark Seaborn wrote: > Nit: Comment style is > /* > * blah > */ Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:113: /* Attempt to protect one page at a time. It is normal for On 2011/08/23 18:43:19, bsy wrote: > there's no reason to do it one page at a time. we need to cover vm_min_addr up > to NACL_SYSCALL_START_ADDR (or equivalently NACL_TRAMPOLINE_START -- we really > shouldn't have both), so why not: > > for (page_addr = NACL_PAGESIZE; > page_addr < NACL_SYSCALL_START_ADDR; > page_addr += NACL_PAGESIZE) { > mmap_rval = mmap((void *) page_addr, NACL_SYSCALL_START_ADDR - page_addr, > PROT_NONE, MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, -1, 0); > if (page_addr == (uintptr_t) mmap_rval) { > /* Success */ > break; > } else if (MAP_FAILED .... etc etc... > so that the whole region is squatted upon by the first successful mmap call? > there's no need to map in 64K at a time on linux. (actually, probably not on > windows either in this particular case, since we don't allow user code to map > over this region of memory.) Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:118: for (page_addr = NACL_PAGESIZE; page_addr < NACL_SYSCALL_START_ADDR; On 2011/08/23 18:26:36, Mark Seaborn wrote: > I'd put each for() clause on a different line if they overflow. Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:121: MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, -1, 0); On 2011/08/23 18:26:36, Mark Seaborn wrote: > Maybe MAP_NORESERVE too, just in case? Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:125: /* Normal; this is an invalid page for this process and */ On 2011/08/23 18:26:36, Mark Seaborn wrote: > Comment style Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:136: } else { On 2011/08/23 18:50:21, bbudge1 wrote: > You could move the 'if' statement here: else if ((err = .. > Or for better readability, you could move the assignment out of the 'if', making > it the first statement in the block, followed by "if (err != 0) {... Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/linux/sel_memory.c (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:46: size_t num_bytes) { On 2011/08/23 18:31:23, bsy wrote: > UNREFERENCED_PARAMETER(num_bytes); Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:48: void *nacl_helper_so = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD); The reason I liked this way of doing it is it works with or without the Chrome support, eliminating an irritating dependency. I will leave it as is unless you insist. On 2011/08/23 18:26:36, Mark Seaborn wrote: > I think it would be been neater to pass the sandbox address as an argument via > NaClMainForChromium() in sel_main_chrome.c rather than using global state via > dlopen(), but the latter is OK. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:59: tmpint = (uintptr_t)dlsym(nacl_helper_so, "nacl_helper_get_1G_address"); On 2011/08/23 18:26:36, Mark Seaborn wrote: > BTW, the usual style in service_runtime is a space after the cast. Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:59: tmpint = (uintptr_t)dlsym(nacl_helper_so, "nacl_helper_get_1G_address"); On 2011/08/23 18:31:23, bsy wrote: > style: space between (type) and expr Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:60: nacl_helper_get_base_addr = (base_addr_func*)tmpint; On 2011/08/23 18:26:36, Mark Seaborn wrote: > Nit: Space before '*' > > You could just do a double cast and remove tmpint. Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:60: nacl_helper_get_base_addr = (base_addr_func*)tmpint; On 2011/08/23 18:31:23, bsy wrote: > style: spaces in (type *) expr; Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:62: if (!nacl_helper_get_base_addr) { On 2011/08/23 18:31:23, bsy wrote: > if (NULL == nacl_helper_get_base_addr) { Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:66: if (!base_addr) { Agreed. However the Windows version of this same routine is returning 0 for failure, and it seems better to keep things consistent. On 2011/08/23 18:26:36, Mark Seaborn wrote: > This is a bit odd because one might expect the base address to be zero if the > zero-based sandbox were successfully reserved. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/linux/sel_memory.c:71: *p = (void *)base_addr; On 2011/08/23 18:31:23, bsy wrote: > style nit: space in (void *) base_addr; Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... File src/trusted/service_runtime/service_runtime.gyp (right): http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/service_runtime.gyp:261: "-ldl", On 2011/08/23 18:26:36, Mark Seaborn wrote: > Nit: use single quotes. Done. http://codereview.chromium.org/7677036/diff/18001/src/trusted/service_runtime... src/trusted/service_runtime/service_runtime.gyp:265: ] On 2011/08/23 18:26:36, Mark Seaborn wrote: > Nit: add trailing comma Done.
nits. o/w lgtm if bots are happy. http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:55: "Can't handle sandbox at high address" " Can't handle" " 0x%08"... so words in format string doesn't run into each other. http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:43: "Can't handle sandbox at high address" ditto http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:114: /* Attempt to protect one page at a time. It is normal for maybe update to something along the lines of: Attempt to protect region starting at lowest possible address and ending at NACL_SYSCALL_START_ADDR, advancing one page at a time until we are at vm.mmap_min_addr. ... http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:128: /* Success; the page is now protected. */ pages are
Done. Waiting for try jobs to finish. Also, will have to revisit Chrome-side code to switch to chain loader instead of the current scheme. On 2011/08/23 21:34:20, bsy wrote: > nits. o/w lgtm if bots are happy. > > http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... > File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): > > http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... > src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:55: "Can't handle > sandbox at high address" > " Can't handle" > " 0x%08"... > so words in format string doesn't run into each other. > > http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... > File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): > > http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:43: "Can't handle > sandbox at high address" > ditto > > http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:114: /* Attempt > to protect one page at a time. It is normal for > maybe update to something along the lines of: > > Attempt to protect region starting at lowest possible address and ending at > NACL_SYSCALL_START_ADDR, advancing one page at a time until we are at > vm.mmap_min_addr. ... > > http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... > src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:128: /* Success; > the page is now protected. */ > pages are
... including review feedback. http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... File src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c (right): http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/arm/sel_addrspace_arm.c:55: "Can't handle sandbox at high address" On 2011/08/23 21:34:20, bsy wrote: > " Can't handle" > " 0x%08"... > so words in format string doesn't run into each other. Done. http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:43: "Can't handle sandbox at high address" On 2011/08/23 21:34:20, bsy wrote: > ditto Done. http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:114: /* Attempt to protect one page at a time. It is normal for On 2011/08/23 21:34:20, bsy wrote: > maybe update to something along the lines of: > > Attempt to protect region starting at lowest possible address and ending at > NACL_SYSCALL_START_ADDR, advancing one page at a time until we are at > vm.mmap_min_addr. ... Done. http://codereview.chromium.org/7677036/diff/18002/src/trusted/service_runtime... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:128: /* Success; the page is now protected. */ On 2011/08/23 21:34:20, bsy wrote: > pages are Done.
Change committed as 6521 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
