From 6804038d728db78026b05507944bd1eb6587e22b Mon Sep 17 00:00:00 2001 From: John Hodge Date: Thu, 23 Aug 2012 13:17:53 +0800 Subject: [PATCH] Kernel/VFS - Fixed mount reference counting and shutdown cleanup - VFS SysFS handles will be removed when / is unmounted - Mountpoint reference counting fixed such that unmount can work --- KernelLand/Kernel/debug.c | 1 + KernelLand/Kernel/drv/proc.c | 56 +++++++++------ KernelLand/Kernel/include/vfs_int.h | 1 + KernelLand/Kernel/syscalls.c | 42 ++++++++---- KernelLand/Kernel/vfs/dir.c | 34 +++++++--- KernelLand/Kernel/vfs/handle.c | 33 ++++++--- KernelLand/Kernel/vfs/main.c | 8 +++ KernelLand/Kernel/vfs/mount.c | 14 +++- KernelLand/Kernel/vfs/open.c | 101 ++++++++++++++++++++++------ 9 files changed, 212 insertions(+), 78 deletions(-) diff --git a/KernelLand/Kernel/debug.c b/KernelLand/Kernel/debug.c index dab20add..31d3b399 100644 --- a/KernelLand/Kernel/debug.c +++ b/KernelLand/Kernel/debug.c @@ -221,6 +221,7 @@ void Panic(const char *Fmt, ...) Debug_Putchar('\n'); Threads_Dump(); + Heap_Dump(); for(;;) ; } diff --git a/KernelLand/Kernel/drv/proc.c b/KernelLand/Kernel/drv/proc.c index 1e0c6bd8..9c7d909a 100644 --- a/KernelLand/Kernel/drv/proc.c +++ b/KernelLand/Kernel/drv/proc.c @@ -276,14 +276,15 @@ int SysFS_RemoveFile(int ID) tSysFS_Ent *ent, *parent, *prev; prev = NULL; - for( ent = gSysFS_FileList; ent; prev = ent, ent = ent->Next ) + for( ent = gSysFS_FileList; ent; prev = ent, ent = ent->ListNext ) { // It's a reverse sorted list - if(ent->Node.Inode < (Uint64)ID) return 0; - if(ent->Node.Inode == (Uint64)ID) break; + if(ent->Node.Inode <= (Uint64)ID) break; + } + if( !ent || ent->Node.Inode != (Uint64)ID) { + Log_Notice("SysFS", "ID %i not present", ID); + return 0; } - - if(!ent) return 0; // Set up for next part file = ent; @@ -297,26 +298,37 @@ int SysFS_RemoveFile(int ID) file->Node.Size = 0; file->Node.ImplPtr = NULL; - // Search parent directory - for( ent = parent->Node.ImplPtr; ent; prev = ent, ent = ent->Next ) + // Clean out of parent directory + while(parent) { - if( ent == file ) break; - } - if(!ent) { - Log_Warning("SysFS", "Bookkeeping Error: File in list, but not in directory"); - return 0; + for( ent = parent->Node.ImplPtr; ent; prev = ent, ent = ent->Next ) + { + if( ent == file ) break; + } + if(!ent) { + Log_Warning("SysFS", "Bookkeeping Error: File in list, but not in directory"); + return 0; + } + + // Remove from parent directory + if(prev) + prev->Next = ent->Next; + else + parent->Node.ImplPtr = ent->Next; + + // Free if not in use + if(file->Node.ReferenceCount == 0) { + free(file); + } + + if( parent->Node.ImplPtr ) + break; + + // Remove parent from the tree + file = parent; + parent = parent->Parent; } - // Remove from parent directory - if(prev) - prev->Next = ent->Next; - else - parent->Node.ImplPtr = ent->Next; - - // Free if not in use - if(file->Node.ReferenceCount == 0) - free(file); - return 1; } diff --git a/KernelLand/Kernel/include/vfs_int.h b/KernelLand/Kernel/include/vfs_int.h index 1f67f4b9..232c55e4 100644 --- a/KernelLand/Kernel/include/vfs_int.h +++ b/KernelLand/Kernel/include/vfs_int.h @@ -51,6 +51,7 @@ extern tVFS_Mount *gVFS_Mounts; extern tVFS_Driver *gVFS_Drivers; // === PROTOTYPES === +extern void VFS_Deinit(void); // --- open.c --- extern char *VFS_GetAbsPath(const char *Path); extern tVFS_Node *VFS_ParsePath(const char *Path, char **TruePath, tVFS_Mount **MountPoint); diff --git a/KernelLand/Kernel/syscalls.c b/KernelLand/Kernel/syscalls.c index d629ae4e..0348d57a 100644 --- a/KernelLand/Kernel/syscalls.c +++ b/KernelLand/Kernel/syscalls.c @@ -275,21 +275,35 @@ void SyscallHandler(tSyscallRegs *Regs) ret = -1; break; } - // Sanity check the paths - if(!Syscall_ValidString((char*)Regs->Arg1) - || !Syscall_ValidString((char*)Regs->Arg2) - || !Syscall_ValidString((char*)Regs->Arg3) - || !Syscall_ValidString((char*)Regs->Arg4) ) { - err = -EINVAL; - ret = -1; - break; + + if( !Regs->Arg1 ) + { + if( !Syscall_ValidString((char*)Regs->Arg2) ) { + err = -EINVAL; + ret = -1; + break; + } + + ret = VFS_Unmount((char*)Regs->Arg2); + } + else + { + // Sanity check the paths + if(!Syscall_ValidString((char*)Regs->Arg1) + || !Syscall_ValidString((char*)Regs->Arg2) + || (Regs->Arg3 && !Syscall_ValidString((char*)Regs->Arg3)) + || !Syscall_ValidString((char*)Regs->Arg4) ) { + err = -EINVAL; + ret = -1; + break; + } + ret = VFS_Mount( + (char*)Regs->Arg1, // Device + (char*)Regs->Arg2, // Mount point + (char*)Regs->Arg3, // Filesystem + (char*)Regs->Arg4 // Options + ); } - ret = VFS_Mount( - (char*)Regs->Arg1, // Device - (char*)Regs->Arg2, // Mount point - (char*)Regs->Arg3, // Filesystem - (char*)Regs->Arg4 // Options - ); break; // Wait on a set of handles diff --git a/KernelLand/Kernel/vfs/dir.c b/KernelLand/Kernel/vfs/dir.c index fab9d563..cd451213 100644 --- a/KernelLand/Kernel/vfs/dir.c +++ b/KernelLand/Kernel/vfs/dir.c @@ -2,6 +2,7 @@ * Acess2 VFS * - Directory Management Functions */ +#define SANITY 1 #define DEBUG 0 #include #include @@ -68,26 +69,22 @@ int VFS_MkNod(const char *Path, Uint Flags) LOG("parent = %p", parent); if(!parent) { - LEAVE('i', -1); - return -1; // Error Check + errno = ENOENT; + goto _error; } // Permissions Check if( !VFS_CheckACL(parent, VFS_PERM_EXECUTE|VFS_PERM_WRITE) ) { - _CloseNode(parent); - mountpt->OpenHandleCount --; - free(absPath); - LEAVE('i', -1); - return -1; + errno = EACCES; + goto _error; } LOG("parent = %p", parent); if(!parent->Type || !parent->Type->MkNod) { Log_Warning("VFS", "VFS_MkNod - Directory has no MkNod method"); - mountpt->OpenHandleCount --; - LEAVE('i', -1); - return -1; + errno = ENOTDIR; + goto _error; } // Create node @@ -98,12 +95,21 @@ int VFS_MkNod(const char *Path, Uint Flags) free(absPath); // Free Parent + ASSERT(mountpt->OpenHandleCount>0); mountpt->OpenHandleCount --; _CloseNode(parent); // Return whatever the driver said LEAVE('i', ret==NULL); return ret==NULL; + +_error: + _CloseNode(parent); + ASSERT(mountpt->OpenHandleCount>0); + mountpt->OpenHandleCount --; + free(absPath); + LEAVE('i', -1); + return -1; } /** @@ -123,6 +129,7 @@ int VFS_Symlink(const char *Name, const char *Link) realLink = VFS_GetAbsPath( Link ); if(!realLink) { Log_Warning("VFS", "Path '%s' is badly formed", Link); + errno = EINVAL; LEAVE('i', -1); return -1; } @@ -133,12 +140,19 @@ int VFS_Symlink(const char *Name, const char *Link) if( VFS_MkNod(Name, VFS_FFLAG_SYMLINK) != 0 ) { Log_Warning("VFS", "Unable to create link node '%s'", Name); free(realLink); + // errno is set by VFS_MkNod LEAVE('i', -2); return -2; // Make link node } // Write link address fp = VFS_Open(Name, VFS_OPENFLAG_WRITE|VFS_OPENFLAG_NOLINK); + if( fp == -1 ) { + Log_Warning("VFS", "Unable to open newly created symlink '%s'", Name); + free(realLink); + LEAVE('i', -3); + return -3; + } VFS_Write(fp, strlen(realLink), realLink); VFS_Close(fp); diff --git a/KernelLand/Kernel/vfs/handle.c b/KernelLand/Kernel/vfs/handle.c index 8cae8040..88877aba 100644 --- a/KernelLand/Kernel/vfs/handle.c +++ b/KernelLand/Kernel/vfs/handle.c @@ -2,6 +2,7 @@ * Acess2 VFS * - AllocHandle, GetHandle */ +#define SANITY 1 #define DEBUG 0 #include #include @@ -25,6 +26,19 @@ tVFS_Handle *gaUserHandles = (void*)MM_PPD_HANDLES; tVFS_Handle *gaKernelHandles = (void*)MM_KERNEL_VFS; // === CODE === +inline void _ReferenceNode(tVFS_Node *Node) +{ + if( !MM_GetPhysAddr(Node->Type) ) { + Log_Error("VFS", "Node %p's type is invalid (%p bad pointer) - %P corrupted", + Node, Node->Type, MM_GetPhysAddr(&Node->Type)); + return ; + } + if( Node->Type && Node->Type->Reference ) + Node->Type->Reference( Node ); + else + Node->ReferenceCount ++; +} + /** * \fn tVFS_Handle *VFS_GetHandle(int FD) * \brief Gets a pointer to the handle information structure @@ -130,8 +144,8 @@ void VFS_ReferenceUserHandles(void) h = &gaUserHandles[i]; if( !h->Node ) continue ; - if( h->Node->Type && h->Node->Type->Reference ) - h->Node->Type->Reference( h->Node ); + _ReferenceNode(h->Node); + h->Mount->OpenHandleCount ++; } } @@ -150,8 +164,7 @@ void VFS_CloseAllUserHandles(void) h = &gaUserHandles[i]; if( !h->Node ) continue ; - if( h->Node->Type && h->Node->Type->Close ) - h->Node->Type->Close( h->Node ); + _CloseNode(h->Node); } } @@ -196,8 +209,7 @@ void *VFS_SaveHandles(int NumFDs, int *FDs) // Reference node if( !h->Node ) continue ; - if( h->Node->Type && h->Node->Type->Reference ) - h->Node->Type->Reference( h->Node ); + _ReferenceNode(h->Node); h->Mount->OpenHandleCount ++; } @@ -243,8 +255,7 @@ void VFS_RestoreHandles(int NumFDs, void *Handles) if( !h->Node ) continue ; - if( h->Node->Type && h->Node->Type->Reference ) - h->Node->Type->Reference( h->Node ); + _ReferenceNode(h->Node); h->Mount->OpenHandleCount ++; } } @@ -265,8 +276,10 @@ void VFS_FreeSavedHandles(int NumFDs, void *Handles) if( !h->Node ) continue ; - if( h->Node->Type && h->Node->Type->Close ) - h->Node->Type->Close( h->Node ); + _CloseNode(h->Node); + + ASSERT(h->Mount->OpenHandleCount > 0); + LOG("dec. mntpt '%s' to %i", h->Mount->MountPoint, h->Mount->OpenHandleCount-1); h->Mount->OpenHandleCount --; } free( Handles ); diff --git a/KernelLand/Kernel/vfs/main.c b/KernelLand/Kernel/vfs/main.c index 38f7c808..fb1da898 100644 --- a/KernelLand/Kernel/vfs/main.c +++ b/KernelLand/Kernel/vfs/main.c @@ -65,6 +65,14 @@ int VFS_Init(void) return 0; } +void VFS_Deinit(void) +{ + SysFS_RemoveFile(giVFS_MountFileID); + free(gsVFS_MountFile); + SysFS_RemoveFile(giVFS_DriverFileID); + free(gsVFS_DriverFile); +} + /** * \fn char *VFS_GetTruePath(const char *Path) * \brief Gets the true path (non-symlink) of a file diff --git a/KernelLand/Kernel/vfs/mount.c b/KernelLand/Kernel/vfs/mount.c index 1832ac16..1823857f 100644 --- a/KernelLand/Kernel/vfs/mount.c +++ b/KernelLand/Kernel/vfs/mount.c @@ -1,6 +1,7 @@ /* * Acess Micro - VFS Server version 1 */ +#define SANITY 1 #define DEBUG 0 #include #include @@ -104,6 +105,7 @@ int VFS_Mount(const char *Device, const char *MountPoint, const char *Filesystem // Create mount information mnt = malloc( sizeof(tVFS_Mount)+deviceLen+1+mountLen+1+argLen+1 ); if(!mnt) { + ASSERT(parent_mnt->OpenHandleCount > 0); parent_mnt->OpenHandleCount --; return -2; } @@ -148,6 +150,7 @@ int VFS_Mount(const char *Device, const char *MountPoint, const char *Filesystem mnt->RootNode = fs->InitDevice(Device, (const char **)args); if(!mnt->RootNode) { free(mnt); + ASSERT(parent_mnt->OpenHandleCount>0); parent_mnt->OpenHandleCount --; return -2; } @@ -204,7 +207,8 @@ void VFS_int_Unmount(tVFS_Mount *Mount) break; } if(mpmnt) { - mpmnt->OpenHandleCount -= 1; + ASSERT(mpmnt->OpenHandleCount>0); + mpmnt->OpenHandleCount --; } else { Log_Notice("VFS", "Mountpoint '%s' has no parent", Mount->MountPoint); @@ -227,6 +231,8 @@ int VFS_Unmount(const char *Mountpoint) if( mount->OpenHandleCount ) { LOG("Mountpoint busy"); RWLock_Release(&glVFS_MountList); + Log_Log("VFS", "Unmount of '%s' deferred, still busy (%i open handles)", + Mountpoint, mount->OpenHandleCount); return EBUSY; } if(prev) @@ -258,12 +264,18 @@ int VFS_UnmountAll(void) // If we've unmounted the final filesystem, all good if( gVFS_Mounts == NULL) { RWLock_Release( &glVFS_MountList ); + + // Final unmount means VFS completely deinited + VFS_Deinit(); return -1; } for( mount = gVFS_Mounts; mount; prev = mount, mount = next ) { next = mount->Next; + + ASSERT(mount->OpenHandleCount >= 0); + // Can't unmount stuff with open handles if( mount->OpenHandleCount > 0 ) { LOG("%p (%s) has open handles (%i of them)", diff --git a/KernelLand/Kernel/vfs/open.c b/KernelLand/Kernel/vfs/open.c index 5ea67c19..e83ace8f 100644 --- a/KernelLand/Kernel/vfs/open.c +++ b/KernelLand/Kernel/vfs/open.c @@ -2,6 +2,7 @@ * Acess2 VFS * - Open, Close and ChDir */ +#define SANITY 1 #define DEBUG 0 #include #include "vfs.h" @@ -21,9 +22,22 @@ extern int VFS_AllocHandle(int bIsUser, tVFS_Node *Node, int Mode); extern tVFS_Node *VFS_MemFile_Create(const char *Path); // === PROTOTYPES === +void _ReferenceMount(tVFS_Mount *Mount, const char *DebugTag); +void _DereferenceMount(tVFS_Mount *Mount, const char *DebugTag); int VFS_int_CreateHandle( tVFS_Node *Node, tVFS_Mount *Mount, int Mode ); // === CODE === +void _ReferenceMount(tVFS_Mount *Mount, const char *DebugTag) +{ +// Log_Debug("VFS", "%s: inc. mntpt '%s' to %i", DebugTag, Mount->MountPoint, Mount->OpenHandleCount+1); + Mount->OpenHandleCount ++; +} +void _DereferenceMount(tVFS_Mount *Mount, const char *DebugTag) +{ +// Log_Debug("VFS", "%s: dec. mntpt '%s' to %i", DebugTag, Mount->MountPoint, Mount->OpenHandleCount-1); + ASSERT(Mount->OpenHandleCount > 0); + Mount->OpenHandleCount --; +} /** * \fn char *VFS_GetAbsPath(const char *Path) * \brief Create an absolute path from a relative one @@ -201,7 +215,7 @@ restart_parse: *TruePath = malloc( gVFS_RootMount->MountPointLen+1 ); strcpy(*TruePath, gVFS_RootMount->MountPoint); } - gVFS_RootMount->OpenHandleCount ++; + _ReferenceMount(gVFS_RootMount, "ParsePath - Fast Tree Root"); if(MountPoint) *MountPoint = gVFS_RootMount; LEAVE('p', gVFS_RootMount->RootNode); return gVFS_RootMount->RootNode; @@ -239,13 +253,19 @@ restart_parse: *MountPoint = mnt; RWLock_Release( &glVFS_MountList ); LOG("Mount %p root", mnt); + _ReferenceMount(mnt, "ParsePath - Mount Root"); LEAVE('p', mnt->RootNode); return mnt->RootNode; } #endif longestMount = mnt; } - longestMount->OpenHandleCount ++; // Increment assuimg it worked + if(!longestMount) { + Log_Panic("VFS", "VFS_ParsePath - No mount for '%s'", Path); + return NULL; + } + + _ReferenceMount(longestMount, "ParsePath"); RWLock_Release( &glVFS_MountList ); // Save to shorter variable @@ -347,7 +367,7 @@ restart_parse: // EVIL: Goto :) LOG("Symlink -> '%s', restart", Path); - mnt->OpenHandleCount --; // Not in this mountpoint + _DereferenceMount(mnt, "ParsePath - sym"); goto restart_parse; } @@ -430,7 +450,7 @@ _error: *TruePath = NULL; } // Open failed, so decrement the open handle count - mnt->OpenHandleCount --; + _DereferenceMount(mnt, "ParsePath - error"); LEAVE('n'); return NULL; @@ -459,6 +479,13 @@ int VFS_int_CreateHandle( tVFS_Node *Node, tVFS_Mount *Mount, int Mode ) errno = EACCES; LEAVE_RET('i', -1); } + + if( MM_GetPhysAddr(Node->Type) == 0 ) { + Log_Error("VFS", "Node %p from mount '%s' (%s) has a bad type (%p)", + Node, Mount->MountPoint, Mount->Filesystem->Name, Node->Type); + errno = EINTERNAL; + LEAVE_RET('i', -1); + } i = VFS_AllocHandle( !!(Mode & VFS_OPENFLAG_USER), Node, Mode ); if( i < 0 ) { @@ -521,28 +548,44 @@ int VFS_OpenEx(const char *Path, Uint Flags, Uint Mode) LEAVE_RET('i', -1); } - // TODO: Check ACLs on the parent + // Check ACLs on the parent if( !VFS_CheckACL(pnode, VFS_PERM_EXECUTE|VFS_PERM_WRITE) ) { - _CloseNode(pnode); - pmnt->OpenHandleCount --; - free(absPath); - LEAVE('i', -1); - return -1; + errno = EACCES; + goto _pnode_err; } // Check that there's a MkNod method if( !pnode->Type || !pnode->Type->MkNod ) { Log_Warning("VFS", "VFS_Open - Directory has no MkNod method"); errno = EINVAL; - LEAVE_RET('i', -1); + goto _pnode_err; } node = pnode->Type->MkNod(pnode, file, new_flags); + if( !node ) { + LOG("Cannot create node '%s' in '%s'", file, absPath); + errno = ENOENT; + goto _pnode_err; + } + // Set mountpoint (and increment open handle count) + mnt = pmnt; + _ReferenceMount(mnt, "Open - create"); // Fall through on error check _CloseNode(pnode); - pmnt->OpenHandleCount --; + _DereferenceMount(pmnt, "Open - create"); + goto _pnode_ok; + + _pnode_err: + if( pnode ) { + _CloseNode(pnode); + _DereferenceMount(pmnt, "Open - create,fail"); + free(absPath); + } + LEAVE('i', -1); + return -1; } + _pnode_ok: // Free generated path free(absPath); @@ -552,7 +595,7 @@ int VFS_OpenEx(const char *Path, Uint Flags, Uint Mode) { LOG("Cannot find node"); errno = ENOENT; - LEAVE_RET('i', -1); + goto _error; } // Check for symlinks @@ -561,26 +604,35 @@ int VFS_OpenEx(const char *Path, Uint Flags, Uint Mode) char tmppath[node->Size+1]; if( node->Size > MAX_PATH_LEN ) { Log_Warning("VFS", "VFS_Open - Symlink is too long (%i)", node->Size); - LEAVE_RET('i', -1); + goto _error; } if( !node->Type || !node->Type->Read ) { Log_Warning("VFS", "VFS_Open - No read method on symlink"); - LEAVE_RET('i', -1); + goto _error; } // Read symlink's path node->Type->Read( node, 0, node->Size, tmppath ); tmppath[ node->Size ] = '\0'; _CloseNode( node ); + _DereferenceMount(mnt, "Open - symlink"); // Open the target node = VFS_ParsePath(tmppath, NULL, &mnt); if(!node) { LOG("Cannot find symlink target node (%s)", tmppath); errno = ENOENT; - LEAVE_RET('i', -1); + goto _error; } } - LEAVE_RET('x', VFS_int_CreateHandle(node, mnt, Flags)); + int ret = VFS_int_CreateHandle(node, mnt, Flags); + LEAVE_RET('x', ret); +_error: + if( node ) + { + _DereferenceMount(mnt, "Open - error"); + _CloseNode(node); + } + LEAVE_RET('i', -1); } @@ -624,7 +676,7 @@ int VFS_OpenChild(int FD, const char *Name, Uint Mode) } // Increment open handle count, no problems with the mount going away as `h` is already open on it - h->Mount->OpenHandleCount ++; + _ReferenceMount(h->Mount, "OpenChild"); LEAVE_RET('x', VFS_int_CreateHandle(node, h->Mount, Mode)); } @@ -676,7 +728,12 @@ void VFS_Close(int FD) Log_Warning("VFS", "Invalid file handle passed to VFS_Close, 0x%x", FD); return; } - + + if( h->Node == NULL ) { + Log_Warning("VFS", "Non-open handle passed to VFS_Close, 0x%x", FD); + return ; + } + #if VALIDATE_VFS_FUNCTIPONS if(h->Node->Close && !MM_GetPhysAddr(h->Node->Close)) { Log_Warning("VFS", "Node %p's ->Close method is invalid (%p)", @@ -685,10 +742,12 @@ void VFS_Close(int FD) } #endif + LOG("Handle %x", FD); _CloseNode(h->Node); - if( h->Mount ) - h->Mount->OpenHandleCount --; + if( h->Mount ) { + _DereferenceMount(h->Mount, "Close"); + } h->Node = NULL; } -- 2.20.1