[haiku-commits] haiku: hrev52185 - in src: system/kernel/vm add-ons/kernel/file_systems/packagefs/nodes

  • From: waddlesplash <waddlesplash@xxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Mon, 6 Aug 2018 23:47:33 -0400 (EDT)

hrev52185 adds 2 changesets to branch 'master'
old head: 91bc3a279a010d5a42ec94238777b7ba744a3466
new head: 247f2fb6917ce95d6a0535a42c16d954be6ad165
overview: 
https://git.haiku-os.org/haiku/log/?qt=range&q=247f2fb6917c+%5E91bc3a279a01

----------------------------------------------------------------------------

023a547dc698: vm: Enable B_USER_CLONEABLE_AREA protection.
  
  Spotted while reading through the VM code while thinking about how to
  implement vfork().
  
  When axeld disabled this in 2005 (!), Haiku's kernel was still young,
  BeOS drivers were still "a thing," and there was no distinction in this
  function from being called by the kernel / not by the kernel.
  
  Now, it's 2018, we manage all drivers ourselves, have SMAP enabled by
  default when available, and as axeld recently noted on the mailing lists,
  "there's not much reason we still use GCC2 for the kernel anyway." So we
  probably don't care about any BeOS drivers that may be broken by this
  (are there any still around?)
  
  Besides the usual fixes to get this 13-year-old chunk to work again, there
  are two functional changes:
    1) Allow the kernel to clone whatever it likes into the user's address 
space.
       It seems that this is often done legitimately (e.g. team creation), and
       so attempting to distinguish those cases seems more work than it may be
       worth right now.
  
       The disadvantage is that drivers without proper checks may be "tricked"
       into cloning areas they shouldn't; but I'm guessing if that's the case,
       then something else is probably broken and the driver should be fixed.
       It seems the reverse case (cloning a userland area into the kernel)
       is much more common (in fact, it looks like all 4 of the 4 places
       where clone_area is used in kernel-space outside the kernel itself
       are doing this.)
  
   2) At KDEBUG_LEVEL 2 and higher, throw a panic when attempting to clone
      an area that does not have the protection flag set. This should make
      finding any bugs exposed by this change much easier than "hardware doesn't
      work" / "black screen on boot" / etc., as well as any potential future
      bugs introduced in the process of driver development.

247f2fb6917c: packagefs: Rename isNewest to overridesHead.
  
  As pointed out by Adrien. Thanks for the review!
  
  Also fixed missing dereference operators.

                              [ Augustin Cavalier <waddlesplash@xxxxxxxxx> ]

----------------------------------------------------------------------------

3 files changed, 11 insertions(+), 15 deletions(-)
.../packagefs/nodes/UnpackingDirectory.cpp          |  4 ++--
.../packagefs/nodes/UnpackingLeafNode.cpp           |  5 ++---
src/system/kernel/vm/vm.cpp                         | 17 +++++++----------

############################################################################

Commit:      023a547dc6981f10bb8437cfba7fcaa3fe3b0d01
URL:         https://git.haiku-os.org/haiku/commit/?id=023a547dc698
Author:      Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date:        Tue Aug  7 03:27:50 2018 UTC

vm: Enable B_USER_CLONEABLE_AREA protection.

Spotted while reading through the VM code while thinking about how to
implement vfork().

When axeld disabled this in 2005 (!), Haiku's kernel was still young,
BeOS drivers were still "a thing," and there was no distinction in this
function from being called by the kernel / not by the kernel.

Now, it's 2018, we manage all drivers ourselves, have SMAP enabled by
default when available, and as axeld recently noted on the mailing lists,
"there's not much reason we still use GCC2 for the kernel anyway." So we
probably don't care about any BeOS drivers that may be broken by this
(are there any still around?)

Besides the usual fixes to get this 13-year-old chunk to work again, there
are two functional changes:
  1) Allow the kernel to clone whatever it likes into the user's address space.
     It seems that this is often done legitimately (e.g. team creation), and
     so attempting to distinguish those cases seems more work than it may be
     worth right now.

     The disadvantage is that drivers without proper checks may be "tricked"
     into cloning areas they shouldn't; but I'm guessing if that's the case,
     then something else is probably broken and the driver should be fixed.
     It seems the reverse case (cloning a userland area into the kernel)
     is much more common (in fact, it looks like all 4 of the 4 places
     where clone_area is used in kernel-space outside the kernel itself
     are doing this.)

 2) At KDEBUG_LEVEL 2 and higher, throw a panic when attempting to clone
    an area that does not have the protection flag set. This should make
    finding any bugs exposed by this change much easier than "hardware doesn't
    work" / "black screen on boot" / etc., as well as any potential future
    bugs introduced in the process of driver development.

----------------------------------------------------------------------------

diff --git a/src/system/kernel/vm/vm.cpp b/src/system/kernel/vm/vm.cpp
index 478a0e5ba7..8152e09fad 100644
--- a/src/system/kernel/vm/vm.cpp
+++ b/src/system/kernel/vm/vm.cpp
@@ -2102,21 +2102,18 @@ vm_clone_area(team_id team, const char* name, void** 
address,
 
        VMCache* cache = vm_area_get_locked_cache(sourceArea);
 
-       // TODO: for now, B_USER_CLONEABLE is disabled, until all drivers
-       //      have been adapted. Maybe it should be part of the kernel 
settings,
-       //      anyway (so that old drivers can always work).
-#if 0
-       if (sourceArea->aspace == VMAddressSpace::Kernel()
-               && addressSpace != VMAddressSpace::Kernel()
+       if (!kernel && sourceAddressSpace == VMAddressSpace::Kernel()
+               && targetAddressSpace != VMAddressSpace::Kernel()
                && !(sourceArea->protection & B_USER_CLONEABLE_AREA)) {
                // kernel areas must not be cloned in userland, unless 
explicitly
                // declared user-cloneable upon construction
-               status = B_NOT_ALLOWED;
-       } else
+#if KDEBUG_LEVEL_2
+               panic("attempting to clone non-user-cloneable kernel area!");
 #endif
-       if (sourceArea->cache_type == CACHE_TYPE_NULL)
                status = B_NOT_ALLOWED;
-       else {
+       } else if (sourceArea->cache_type == CACHE_TYPE_NULL) {
+               status = B_NOT_ALLOWED;
+       } else {
                virtual_address_restrictions addressRestrictions = {};
                addressRestrictions.address = *address;
                addressRestrictions.address_specification = addressSpec;

############################################################################

Revision:    hrev52185
Commit:      247f2fb6917ce95d6a0535a42c16d954be6ad165
URL:         https://git.haiku-os.org/haiku/commit/?id=247f2fb6917c
Author:      Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date:        Tue Aug  7 03:46:24 2018 UTC

packagefs: Rename isNewest to overridesHead.

As pointed out by Adrien. Thanks for the review!

Also fixed missing dereference operators.

----------------------------------------------------------------------------

diff --git 
a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp 
b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp
index 84cb86fced..0f322453e5 100644
--- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp
+++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp
@@ -108,9 +108,9 @@ UnpackingDirectory::AddPackageNode(PackageNode* 
packageNode, dev_t deviceID)
                = dynamic_cast<PackageDirectory*>(packageNode);
 
        PackageDirectory* other = fPackageDirectories.Head();
-       bool isNewest = other == NULL || *packageDirectory > *other;
+       bool overridesHead = other == NULL || *packageDirectory > *other;
 
-       if (isNewest) {
+       if (overridesHead) {
                fPackageDirectories.Insert(other, packageDirectory);
                NodeReinitVFS(deviceID, fID, packageDirectory, other, fFlags);
        } else
diff --git 
a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp 
b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp
index fc7a0e2627..86ddcafd27 100644
--- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp
+++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp
@@ -116,10 +116,9 @@ UnpackingLeafNode::AddPackageNode(PackageNode* 
packageNode, dev_t deviceID)
                = dynamic_cast<PackageLeafNode*>(packageNode);
 
        PackageLeafNode* headNode = fPackageNodes.Head();
-       bool isNewest = headNode == NULL
-               || packageLeafNode > headNode;
+       bool overridesHead = headNode == NULL || *packageLeafNode > *headNode;
 
-       if (isNewest) {
+       if (overridesHead) {
                fPackageNodes.Add(packageLeafNode);
                NodeReinitVFS(deviceID, fID, packageLeafNode, headNode, fFlags);
        } else {


Other related posts:

  • » [haiku-commits] haiku: hrev52185 - in src: system/kernel/vm add-ons/kernel/file_systems/packagefs/nodes - waddlesplash