Skip to content

Commit

Permalink
Mac kext: Use only VirtualizationRoot handles, not direct pointers
Browse files Browse the repository at this point in the history 
This change moves the VirtualizationRoot structure definition out of the header file, and unifies the API to only use integer handles, which internally are array indices for non-negative values, and negative values from an enum of special cases. Where indices were previously used outside the VirtualizationRoot implementation functions, the terminology has been changed to "handles."

An immediate advantage of the move away from pointers is an improvement in the ability to control thread safety of the root structures; additionally, unlike pointers, handles remain valid outside of held locks, so in a follow-on change, we can reallocate the array for resizing.
  • Loading branch information
@pmj
pmj committed Oct 5, 2018
1 parent e50e488 commit 3211ed1
Show file tree
Hide file tree
Showing 5 changed files with 186 additions and 107 deletions.
70 changes: 28 additions & 42 deletions ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,9 @@ static inline bool ActionBitIsSet(kauth_action_t action, kauth_action_t mask);

static bool IsFileSystemCrawler(char* procname);

static const char* GetRelativePath(const char* path, const char* root);

static void Sleep(int seconds, void* channel);
static bool TrySendRequestAndWaitForResponse(
const VirtualizationRoot* root,
VirtualizationRootHandle root,
MessageType messageType,
const vnode_t vnode,
const FsidInode& vnodeFsidInode,
Expand All @@ -65,7 +63,7 @@ static bool ShouldHandleVnodeOpEvent(
kauth_action_t action,

// Out params:
VirtualizationRoot** root,
VirtualizationRootHandle* root,
vtype* vnodeType,
uint32_t* vnodeFileFlags,
FsidInode* vnodeFsidInode,
Expand All @@ -80,7 +78,7 @@ static bool ShouldHandleFileOpEvent(
kauth_action_t action,

// Out params:
VirtualizationRoot** root,
VirtualizationRootHandle* root,
FsidInode* vnodeFsidInode,
int* pid);

Expand Down Expand Up @@ -277,7 +275,7 @@ static int HandleVnodeOperation(
char vnodePathBuffer[PrjFSMaxPath];
int vnodePathLength = PrjFSMaxPath;

VirtualizationRoot* root = nullptr;
VirtualizationRootHandle root = RootHandle_None;
vtype vnodeType;
uint32_t currentVnodeFileFlags;
FsidInode vnodeFsidInode;
Expand Down Expand Up @@ -435,7 +433,7 @@ static int HandleFileOpOperation(
goto CleanupAndReturn;
}

VirtualizationRoot* root = nullptr;
VirtualizationRootHandle root = RootHandle_None;
FsidInode vnodeFsidInode;
int pid;
if (!ShouldHandleFileOpEvent(
Expand Down Expand Up @@ -495,7 +493,7 @@ static int HandleFileOpOperation(
goto CleanupAndReturn;
}

VirtualizationRoot* root = nullptr;
VirtualizationRootHandle root = RootHandle_None;
FsidInode vnodeFsidInode;
int pid;
if (!ShouldHandleFileOpEvent(
Expand Down Expand Up @@ -571,16 +569,16 @@ static bool ShouldHandleVnodeOpEvent(
kauth_action_t action,

// Out params:
VirtualizationRoot** root,
VirtualizationRootHandle* root,
vtype* vnodeType,
uint32_t* vnodeFileFlags,
FsidInode* vnodeFsidInode,
int* pid,
char procname[MAXCOMLEN + 1],
int* kauthResult)
{
*root = nullptr;
*kauthResult = KAUTH_RESULT_DEFER;
*root = RootHandle_None;

if (!VirtualizationRoot_VnodeIsOnAllowedFilesystem(vnode))
{
Expand Down Expand Up @@ -626,19 +624,22 @@ static bool ShouldHandleVnodeOpEvent(
}

*vnodeFsidInode = Vnode_GetFsidAndInode(vnode, context);
*root = VirtualizationRoots_FindForVnode(vnode, *vnodeFsidInode);
*root = VirtualizationRoot_FindForVnode(vnode, *vnodeFsidInode);

if (nullptr == *root)
if (RootHandle_ProviderTemporaryDirectory == *root)
{
*kauthResult = KAUTH_RESULT_DEFER;
return false;
}
else if (RootHandle_None == *root)
{
KextLog_FileNote(vnode, "No virtualization root found for file with set flag.");

*kauthResult = KAUTH_RESULT_DEFER;
return false;
}
else if (nullptr == (*root)->providerUserClient)
else if (!VirtualizationRoot_IsOnline(*root))
{
// There is no registered provider for this root

// TODO(Mac): Protect files in the worktree from modification (and prevent
// the creation of new files) when the provider is offline

Expand All @@ -647,7 +648,7 @@ static bool ShouldHandleVnodeOpEvent(
}

// If the calling process is the provider, we must exit right away to avoid deadlocks
if (*pid == (*root)->providerPid)
if (VirtualizationRoot_PIDMatchesProvider(*root, *pid))
{
*kauthResult = KAUTH_RESULT_DEFER;
return false;
Expand All @@ -663,40 +664,38 @@ static bool ShouldHandleFileOpEvent(
kauth_action_t action,

// Out params:
VirtualizationRoot** root,
VirtualizationRootHandle* root,
FsidInode* vnodeFsidInode,
int* pid)
{
*root = RootHandle_None;

vtype vnodeType = vnode_vtype(vnode);
if (ShouldIgnoreVnodeType(vnodeType, vnode))
{
return false;
}

*vnodeFsidInode = Vnode_GetFsidAndInode(vnode, context);
*root = VirtualizationRoots_FindForVnode(vnode, *vnodeFsidInode);
if (nullptr == *root)
{
return false;
}
else if (nullptr == (*root)->providerUserClient)
*root = VirtualizationRoot_FindForVnode(vnode, *vnodeFsidInode);
if (!VirtualizationRoot_IsValidRootHandle(*root))
{
// There is no registered provider for this root
// This VNode is not part of a root
return false;
}

// If the calling process is the provider, we must exit right away to avoid deadlocks
*pid = GetPid(context);
if (*pid == (*root)->providerPid)
if (VirtualizationRoot_PIDMatchesProvider(*root, *pid))
{
// If the calling process is the provider, we must exit right away to avoid deadlocks
return false;
}

return true;
}

static bool TrySendRequestAndWaitForResponse(
const VirtualizationRoot* root,
VirtualizationRootHandle root,
MessageType messageType,
const vnode_t vnode,
const FsidInode& vnodeFsidInode,
Expand All @@ -719,7 +718,7 @@ static bool TrySendRequestAndWaitForResponse(
return false;
}

const char* relativePath = GetRelativePath(vnodePath, root->path);
const char* relativePath = VirtualizationRoot_GetRootRelativePath(root, vnodePath);

int nextMessageId = OSIncrementAtomic(&s_nextMessageId);

Expand Down Expand Up @@ -748,7 +747,7 @@ static bool TrySendRequestAndWaitForResponse(

// TODO(Mac): Should we pass in the root directly, rather than root->index?
// The index seems more like a private implementation detail.
if (!isShuttingDown && 0 != ActiveProvider_SendMessage(root->index, messageSpec))
if (!isShuttingDown && 0 != ActiveProvider_SendMessage(root, messageSpec))
{
// TODO: appropriately handle unresponsive providers

Expand Down Expand Up @@ -885,19 +884,6 @@ static bool IsFileSystemCrawler(char* procname)
return false;
}

static const char* GetRelativePath(const char* path, const char* root)
{
assert(strlen(path) >= strlen(root));

const char* relativePath = path + strlen(root);
if (relativePath[0] == '/')
{
relativePath++;
}

return relativePath;
}

static bool ShouldIgnoreVnodeType(vtype vnodeType, vnode_t vnode)
{
switch (vnodeType)
Expand Down
20 changes: 12 additions & 8 deletions ProjFS.Mac/PrjFSKext/PrjFSKext/PrjFSProviderUserClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ static const IOExternalMethodDispatch ProviderUserClientDispatch[] =
{
[ProviderSelector_RegisterVirtualizationRootPath] =
{
&PrjFSProviderUserClient::registerVirtualizationRoot, 0, kIOUCVariableStructureSize, 1, 0
.function = &PrjFSProviderUserClient::registerVirtualizationRoot,
.checkScalarInputCount = 0,
.checkStructureInputSize = kIOUCVariableStructureSize, // null-terminated string: virtualisation root path
.checkScalarOutputCount = 1, // returned errno
.checkStructureOutputSize = 0
},
[ProviderSelector_KernelMessageResponse] =
{
Expand All @@ -37,7 +41,7 @@ bool PrjFSProviderUserClient::initWithTask(
UInt32 type,
OSDictionary* properties)
{
this->virtualizationRootIndex = -1;
this->virtualizationRootHandle = RootHandle_None;
this->pid = proc_selfpid();

if (!this->super::initWithTask(owningTask, securityToken, type, properties))
Expand Down Expand Up @@ -92,9 +96,9 @@ void PrjFSProviderUserClient::free()
// the connection.
IOReturn PrjFSProviderUserClient::clientClose()
{
int32_t root = this->virtualizationRootIndex;
this->virtualizationRootIndex = -1;
if (-1 != root)
VirtualizationRootHandle root = this->virtualizationRootHandle;
this->virtualizationRootHandle = RootHandle_None;
if (RootHandle_None != root)
{
ActiveProvider_Disconnect(root);
}
Expand Down Expand Up @@ -210,7 +214,7 @@ IOReturn PrjFSProviderUserClient::registerVirtualizationRoot(const char* rootPat
*outError = EINVAL;
return kIOReturnSuccess;
}
else if (this->virtualizationRootIndex != -1)
else if (this->virtualizationRootHandle != RootHandle_None)
{
// Already set
*outError = EBUSY;
Expand All @@ -220,11 +224,11 @@ IOReturn PrjFSProviderUserClient::registerVirtualizationRoot(const char* rootPat
VirtualizationRootResult result = VirtualizationRoot_RegisterProviderForPath(this, this->pid, rootPath);
if (0 == result.error)
{
this->virtualizationRootIndex = result.rootIndex;
this->virtualizationRootHandle = result.root;

// Sets the root index in the IORegistry for diagnostic purposes
char location[5] = "";
snprintf(location, sizeof(location), "%d", result.rootIndex);
snprintf(location, sizeof(location), "%d", result.root);
this->setLocation(location);
}

Expand Down
5 changes: 3 additions & 2 deletions ProjFS.Mac/PrjFSKext/PrjFSKext/PrjFSProviderUserClient.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "PrjFSClasses.hpp"
#include "Locks.hpp"
#include "Message.h"
#include "VirtualizationRoots.hpp"
#include <IOKit/IOUserClient.h>

struct MessageHeader;
Expand All @@ -18,8 +19,8 @@ class PrjFSProviderUserClient : public IOUserClient
Mutex dataQueueWriterMutex;
public:
pid_t pid;
// The root for which this is the provider; -1 prior to registration
int32_t virtualizationRootIndex;
// The root for which this is the provider; RootHandle_None prior to registration
VirtualizationRootHandle virtualizationRootHandle;

// IOUserClient methods:
virtual bool initWithTask(
Expand Down
Loading

0 comments on commit 3211ed1

Please sign in to comment.