From ed8ff1ff9c4c0f847ffc2ab4624bd999539a0890 Mon Sep 17 00:00:00 2001 From: dzwdz Date: Thu, 11 Jul 2024 21:43:50 +0200 Subject: kernel: start cleaning up VfsRequest * I'm being more strict about the linked list state to hopefully ensure I'm not leaking any references. * vfsreq_create was renamed to vfsreq_dispatchcopy as that name feels more clear. It copies its argument, and dispatches it. * Requests for user backends are now handled more like requests for kernel backends - there's an accept() function that accepts a request. --- src/kernel/arch/amd64/driver/util.c | 2 + src/kernel/handle.c | 5 +- src/kernel/syscalls.c | 15 +++-- src/kernel/vfs/request.c | 127 +++++++++++++++++++++++------------- src/kernel/vfs/request.h | 6 +- 5 files changed, 102 insertions(+), 53 deletions(-) (limited to 'src/kernel') diff --git a/src/kernel/arch/amd64/driver/util.c b/src/kernel/arch/amd64/driver/util.c index 0f0ee34..2b33849 100644 --- a/src/kernel/arch/amd64/driver/util.c +++ b/src/kernel/arch/amd64/driver/util.c @@ -26,6 +26,7 @@ bool postqueue_pop(VfsReq **queue, void (*accept)(VfsReq *)) { VfsReq *req = *queue; if (req == NULL) return false; *queue = req->postqueue_next; + req->postqueue_next = NULL; accept(req); return true; } @@ -45,6 +46,7 @@ void postqueue_ringreadall(VfsReq **queue, ring_t *r) { while (*queue) { req = *queue; *queue = req->postqueue_next; + req->postqueue_next = NULL; size_t ret = min(mlen, req->output.len); assert(req->type == VFSOP_READ); diff --git a/src/kernel/handle.c b/src/kernel/handle.c index f216c13..06a810c 100644 --- a/src/kernel/handle.c +++ b/src/kernel/handle.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -22,7 +23,7 @@ void handle_close(Handle *h) { if (h->base) { handle_close(h->base); } else { - vfsreq_create((VfsReq) { + vfsreq_dispatchcopy((VfsReq) { .type = VFSOP_CLOSE, .id = h->file_id, .caller = NULL, @@ -36,7 +37,7 @@ void handle_close(Handle *h) { h->pipe.sister->pipe.sister = NULL; } } else if (h->type == HANDLE_FS_REQ) { - if (h->req) vfsreq_finish_short(h->req, -1); + if (h->req) vfsreq_finish_short(h->req, -EPIPE); } if (h->backend) { diff --git a/src/kernel/syscalls.c b/src/kernel/syscalls.c index 9b0746a..5771b6a 100644 --- a/src/kernel/syscalls.c +++ b/src/kernel/syscalls.c @@ -85,7 +85,7 @@ hid_t _sys_open(const char __user *path, long len, int flags) { memcpy(path_buf, path_buf + mount->prefix_len, len); } - vfsreq_create((VfsReq) { + vfsreq_dispatchcopy((VfsReq) { .type = VFSOP_OPEN, .input = { .kern = true, @@ -198,7 +198,7 @@ static long simple_vfsop( req.input.buf = buf; req.input.len = len; } - vfsreq_create(req); + vfsreq_dispatchcopy(req); } else if (h->type == HANDLE_PIPE) { if (vfsop == VFSOP_READ || vfsop == VFSOP_WRITE) { /* already checked if this is the correct pipe end */ @@ -238,7 +238,7 @@ long _sys_remove(hid_t hid) { hs_close(proc_cur->hs, hid); SYSCALL_RETURN(-EACCES); } - vfsreq_create((VfsReq) { + vfsreq_dispatchcopy((VfsReq) { .type = VFSOP_REMOVE, .id = h->file_id, .caller = proc_cur, @@ -271,7 +271,14 @@ hid_t _sys_fs_wait(char __user *buf, long max_len, struct ufs_request __user *re proc_cur->awaited_req.max_len = max_len; proc_cur->awaited_req.res = res; - vfs_backend_tryaccept(backend); // sets return value + // TODO maybe i should use the postqueue stuff here + if (backend->queue) { + VfsReq *req = backend->queue; + backend->queue = req->queue_next; + req->queue_next = NULL; + vfsback_useraccept(req); + } + return -1; // dummy } diff --git a/src/kernel/vfs/request.c b/src/kernel/vfs/request.c index 8fe0114..4489f5a 100644 --- a/src/kernel/vfs/request.c +++ b/src/kernel/vfs/request.c @@ -6,45 +6,75 @@ #include #include -static void vfs_backend_user_accept(VfsReq *req); +/** Checks if the request flags don't contradict each other. + * While this could be done by caller of vfsreq_dispatchcopy, as currently + * there's only one of them per operation, doing this here means that any + * potential future callers can't forget those checks. + * Doing this so late is kinda inefficient but I don't really care. */ +static bool +vfsreq_isvalid(VfsReq *r) +{ + /* Notable omissions: + * - Simplifying the path for open(). Already done when resolving the mount. + * - Checking the handle permissions. We don't have access to it. + * */ + if (r->type == VFSOP_OPEN && !(r->flags & OPEN_WRITE) && (r->flags & OPEN_CREATE)) { + return false; + } + + // XXX if i add a handle field to vfs_request, check ->readable ->writeable here + return true; +} -void vfsreq_create(VfsReq req_) { +void +vfsreq_dispatchcopy(VfsReq tmpl) +{ VfsReq *req; - if (req_.caller) { - proc_setstate(req_.caller, PS_WAITS4FS); - if (!req_.caller->reqslot) - req_.caller->reqslot = kmalloc(sizeof *req, "reqslot"); - req = req_.caller->reqslot; + VfsBackend *backend; + + /* allocate memory for the request and move it there */ + if (tmpl.caller) { + // TODO: ensure we aren't overriding an existing request + // TODO: wouldn't it be better to just optimize malloc? + // see commit afa28cfc7069f7649632f7b33daac47ee56d3819 + proc_setstate(tmpl.caller, PS_WAITS4FS); + if (!tmpl.caller->reqslot) + tmpl.caller->reqslot = kmalloc(sizeof *req, "reqslot"); + req = tmpl.caller->reqslot; /* (re)using a single allocation for all request a process makes */ } else { req = kmalloc(sizeof *req, "reqanon"); } - memcpy(req, &req_, sizeof *req); - if (req->backend) { - assert(req->backend->usehcnt); - req->backend->usehcnt++; + + memcpy(req, &tmpl, sizeof *req); + backend = req->backend; + if (backend) { + assert(backend->usehcnt > 0); + backend->usehcnt++; } - if (req->type == VFSOP_OPEN && !(req->flags & OPEN_WRITE) && (req->flags & OPEN_CREATE)) { + /* Check request validity. Doing this after the memcpy means that it + * doesn't need to be special cased in vfsreq_finish. */ + if (!vfsreq_isvalid(req)) { vfsreq_finish_short(req, -EINVAL); return; } - - // TODO if i add a handle field to vfs_request, check ->readable ->writeable here - - if (req->backend && req->backend->provhcnt) { - VfsReq **iter = &req->backend->queue; - while (*iter != NULL) // find free spot in queue - iter = &(*iter)->queue_next; - *iter = req; - vfs_backend_tryaccept(req->backend); - } else { - vfsreq_finish_short(req, -1); + assert(req->queue_next == NULL); + assert(req->postqueue_next == NULL); + + if (backend == NULL) { + /* null mount - probably never had a real backend */ + vfsreq_finish_short(req, -ENOENT); + } else if (backend->is_user) { + vfsback_useraccept(req); + } else { /* kernel backend */ + assert(backend->kern.accept); + backend->kern.accept(req); } } -void vfsreq_finish(VfsReq *req, char __user *stored, long ret, - int flags, Proc *handler) +void +vfsreq_finish(VfsReq *req, char __user *stored, long ret, int flags, Proc *handler) { if (req->type == VFSOP_OPEN && ret >= 0) { Handle *h; @@ -88,6 +118,9 @@ void vfsreq_finish(VfsReq *req, char __user *stored, long ret, vfsback_userdown(req->backend); } + assert(req->queue_next == NULL); + assert(req->postqueue_next == NULL); + if (req->caller) { assert(req->caller->state == PS_WAITS4FS); regs_savereturn(&req->caller->regs, ret); @@ -97,27 +130,33 @@ void vfsreq_finish(VfsReq *req, char __user *stored, long ret, } } -void vfs_backend_tryaccept(VfsBackend *backend) { - VfsReq *req = backend->queue; - if (!req) return; - if (backend->is_user && !backend->user.handler) return; - - backend->queue = req->queue_next; - if (backend->is_user) { - vfs_backend_user_accept(req); - } else { - assert(backend->kern.accept); - backend->kern.accept(req); - } -} - -static void vfs_backend_user_accept(VfsReq *req) { +void +vfsback_useraccept(VfsReq *req) +{ + VfsBackend *backend; Proc *handler; struct ufs_request res = {0}; int len; - assert(req && req->backend && req->backend->user.handler); - handler = req->backend->user.handler; + assert(req != NULL); + backend = req->backend; + assert(backend); + assert(backend->is_user); + if (backend->provhcnt == 0) { + vfsreq_finish_short(req, -EPIPE); + return; + } else if (backend->user.handler == NULL) { + /* queue the request up */ + // TODO use postqueue + VfsReq **it = &backend->queue; + while (*it != NULL) { /* find a free spot in queue */ + it = &(*it)->queue_next; + } + *it = req; + return; + } + + handler = backend->user.handler; assert(handler->state == PS_WAITS4REQUEST); // the pcpy calls aren't present in all kernel backends @@ -158,7 +197,6 @@ static void vfs_backend_user_accept(VfsReq *req) { proc_setstate(handler, PS_RUNNING); regs_savereturn(&handler->regs, hid); req->backend->user.handler = NULL; - return; } static void @@ -204,7 +242,8 @@ vfsback_provdown(VfsBackend *b) VfsReq *q = b->queue; while (q) { VfsReq *q2 = q->queue_next; - vfsreq_finish_short(q, -1); + q->queue_next = NULL; + vfsreq_finish_short(q, -EPIPE); q = q2; } b->queue = NULL; diff --git a/src/kernel/vfs/request.h b/src/kernel/vfs/request.h index 2bd8c61..0b93491 100644 --- a/src/kernel/vfs/request.h +++ b/src/kernel/vfs/request.h @@ -3,7 +3,6 @@ #include #include -/* describes something which can act as an access function */ struct VfsBackend { /* amount of using references * VfsMount @@ -14,6 +13,7 @@ struct VfsBackend { /* amount of providing references * Proc * 0 - orphaned, will never increase */ + // TODO move this into .user size_t provhcnt; VfsReq *queue; @@ -64,7 +64,7 @@ struct VfsReq { }; /** Assigns the vfs_request to the caller, and dispatches the call */ -void vfsreq_create(VfsReq); +void vfsreq_dispatchcopy(VfsReq); void vfsreq_finish(VfsReq*, char __user *stored, long ret, int flags, Proc *handler); static inline void vfsreq_finish_short(VfsReq *req, long ret) { @@ -72,7 +72,7 @@ static inline void vfsreq_finish_short(VfsReq *req, long ret) { } /** Try to accept an enqueued request */ -void vfs_backend_tryaccept(VfsBackend *); +void vfsback_useraccept(VfsReq *); /** Decrements the "user" reference count. */ void vfsback_userdown(VfsBackend *); -- cgit v1.2.3