Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mac kext: Virtualisation root refactoring & array resizing #322

Merged
merged 6 commits into from
Oct 6, 2018
Merged

Mac kext: Virtualisation root refactoring & array resizing #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
755dda6
Mac kext: Handle named streams
pmj Oct 1, 2018
f147f8c
Mac kext: Replaces some C-style casts with C++ style.
pmj Sep 28, 2018
d21d165
Mac kext: Enables warning for shadowed variables.
pmj Oct 1, 2018
e50e488
Mac kext: Adds typed array memory allocation function.
pmj Oct 1, 2018
3211ed1
Mac kext: Use only VirtualizationRoot handles, not direct pointers
pmj Sep 28, 2018
0a0ac88
Mac kext: Dynamically alloc virtualisation root array & resize when f…
pmj Oct 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ProjFS.Mac/PrjFSKext/PrjFSKext.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@
"$(inherited)",
"MACH_ASSERT=1",
);
GCC_WARN_SHADOW = YES;
INFOPLIST_FILE = PrjFSKext/Info.plist;
MODULE_NAME = io.gvfs.PrjFSKext;
MODULE_START = PrjFSKext_Start;
Expand All @@ -430,7 +431,11 @@
buildSettings = {
CODE_SIGN_STYLE = Automatic;
DEVELOPMENT_TEAM = UBF8T346G9;
GCC_PREPROCESSOR_DEFINITIONS = "MACH_ASSERT=1";
GCC_PREPROCESSOR_DEFINITIONS = (
"MACH_ASSERT=1",
"$(inherited)",
);
GCC_WARN_SHADOW = YES;
INFOPLIST_FILE = PrjFSKext/Info.plist;
MODULE_NAME = io.gvfs.PrjFSKext;
MODULE_START = PrjFSKext_Start;
Expand Down
91 changes: 47 additions & 44 deletions ProjFS.Mac/PrjFSKext/PrjFSKext/KauthHandler.cpp
View file
Open in desktop
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 @@ -260,12 +258,24 @@ static int HandleVnodeOperation(
// arg2 is the (vnode_t) parent vnode
int* kauthError = reinterpret_cast<int*>(arg3);
int kauthResult = KAUTH_RESULT_DEFER;
bool putVnodeWhenDone = false;

// A lot of our file checks such as attribute tests behave oddly if the vnode
// refers to a named fork/stream; apply the logic as if the vnode operation was
// occurring on the file itself. (/path/to/file/..namedfork/rsrc)
if (vnode_isnamedstream(currentVnode))
{
vnode_t mainFileFork = vnode_getparent(currentVnode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should we do if this call fails and returns NULLVP? We now know that currentVnode is referring to a named stream and that we failed to resolve to the actual file, so should we give up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: is there any way to guarantee that we can find the parent? The vnode_getparent function is racy since it only resolves vnodes that are still in the name cache. Is there an alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had a chance to investigate the vnode_parent issues in depth yet, so for now I can't really answer that question. In the named stream case, I don't see how the main fork vnode could possibly disappear, but at the same time I have no evidence that we're guaranteed to be able to grab it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to leave the bigger investigation for a separate change, but I think we should either assert or return a failure here if this call fails. Otherwise we know we will misbehave downstream of this code.

Copy link
Contributor Author

@pmj pmj Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, an assertion sounds the most sensible approach here until we get a chance to look into it. If the assert starts firing, we'll know it's a pressing issue.

assert(NULLVP != mainFileFork);
currentVnode = mainFileFork;
putVnodeWhenDone = true;
}

const char* vnodePath = nullptr;
char vnodePathBuffer[PrjFSMaxPath];
int vnodePathLength = PrjFSMaxPath;

VirtualizationRoot* root = nullptr;
VirtualizationRootHandle root = RootHandle_None;
vtype vnodeType;
uint32_t currentVnodeFileFlags;
FsidInode vnodeFsidInode;
Expand Down Expand Up @@ -384,6 +394,11 @@ static int HandleVnodeOperation(
}

CleanupAndReturn:
if (putVnodeWhenDone)
{
vnode_put(currentVnode);
}

atomic_fetch_sub(&s_numActiveKauthEvents, 1);
return kauthResult;
}
Expand All @@ -408,7 +423,7 @@ static int HandleFileOpOperation(
KAUTH_FILEOP_LINK == action)
{
// arg0 is the (const char *) fromPath (or the file being linked to)
const char* newPath = (const char*)arg1;
const char* newPath = reinterpret_cast<const char*>(arg1);

// TODO(Mac): We need to handle failures to lookup the vnode. If we fail to lookup the vnode
// it's possible that we'll miss notifications
Expand All @@ -418,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 @@ -464,7 +479,7 @@ static int HandleFileOpOperation(
else if (KAUTH_FILEOP_CLOSE == action)
{
vnode_t currentVnode = reinterpret_cast<vnode_t>(arg0);
const char* path = (const char*)arg1;
const char* path = reinterpret_cast<const char*>(arg1);
int closeFlags = static_cast<int>(arg2);

if (vnode_isdir(currentVnode))
Expand All @@ -478,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 @@ -554,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 @@ -609,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 @@ -630,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 @@ -646,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 @@ -702,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 @@ -731,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 @@ -868,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
13 changes: 13 additions & 0 deletions ProjFS.Mac/PrjFSKext/PrjFSKext/Memory.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,17 @@ kern_return_t Memory_Cleanup();
void* Memory_Alloc(uint32_t size);
void Memory_Free(void* buffer, uint32_t size);

template <typename T>
T* Memory_AllocArray(uint32_t arrayLength)
{
size_t allocBytes = arrayLength * sizeof(T);
if (allocBytes > UINT32_MAX)
{
return nullptr;
}

return static_cast<T*>(Memory_Alloc(static_cast<uint32_t>(allocBytes)));
}


#endif /* Memory_h */
20 changes: 12 additions & 8 deletions ProjFS.Mac/PrjFSKext/PrjFSKext/PrjFSProviderUserClient.cpp
View file
Open in desktop
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
Open in desktop
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