From c6bbf615e5c77ec273b954c11cef95df3d6f7286 Mon Sep 17 00:00:00 2001 From: dzwdz Date: Sun, 17 Jul 2022 20:28:53 +0200 Subject: kernel/virt_cpy: error struct, better error handling --- src/kernel/arch/amd64/driver/fsroot.c | 42 ++++++++++++++++++----------------- src/kernel/mem/virt.c | 37 ++++++++++++++++++------------ src/kernel/mem/virt.h | 19 +++++++++++----- src/kernel/pipe.c | 9 ++++---- src/kernel/syscalls.c | 11 +++++---- src/kernel/vfs/request.c | 20 ++++++++++++----- src/shared/errno.h | 3 +++ src/user/driver/tmpfs.c | 21 +++++++++--------- src/user/tests/main.c | 18 +++++++++++++-- 9 files changed, 113 insertions(+), 67 deletions(-) create mode 100644 src/shared/errno.h (limited to 'src') diff --git a/src/kernel/arch/amd64/driver/fsroot.c b/src/kernel/arch/amd64/driver/fsroot.c index e651c47..5e48638 100644 --- a/src/kernel/arch/amd64/driver/fsroot.c +++ b/src/kernel/arch/amd64/driver/fsroot.c @@ -1,9 +1,10 @@ #include +#include #include #include #include #include -#include +#include #include #include @@ -42,6 +43,16 @@ static void req_preprocess(struct vfs_request *req, size_t max_len) { assert(req->input.len + req->offset <= max_len); } +static int req_readcopy(struct vfs_request *req, const void *buf, size_t len) { + assert(req->type == VFSOP_READ); + req_preprocess(req, len); + virt_cpy_to( + req->caller->pages, req->output.buf, + buf + req->offset, req->output.len); + /* read errors are ignored. TODO write docs */ + return req->output.len; +} + static int handle(struct vfs_request *req) { assert(req->caller); @@ -72,18 +83,10 @@ static int handle(struct vfs_request *req) { "com1\0" "ps2\0" "ata/"; - req_preprocess(req, sizeof src); - virt_cpy_to(req->caller->pages, req->output.buf, - src + req->offset, req->output.len); - return req->output.len; - } - case HANDLE_VGA: { - char *vga = (void*)0xB8000; - req_preprocess(req, 80*25*2); - virt_cpy_to(req->caller->pages, req->output.buf, - vga + req->offset, req->output.len); - return req->output.len; + return req_readcopy(req, src, sizeof src); } + case HANDLE_VGA: + return req_readcopy(req, (void*)0xB8000, 80*25*2); case HANDLE_ATA_ROOT: { char list[8] = {}; size_t len = 0; @@ -93,13 +96,10 @@ static int handle(struct vfs_request *req) { len += 2; } } - req_preprocess(req, len); - virt_cpy_to(req->caller->pages, req->output.buf, - list + req->offset, req->output.len); - return req->output.len; + return req_readcopy(req, list, len); } case HANDLE_ATA: case HANDLE_ATA+1: - case HANDLE_ATA+2: case HANDLE_ATA+3: { + case HANDLE_ATA+2: case HANDLE_ATA+3: if (req->offset < 0) return 0; char buf[512]; uint32_t sector = req->offset / 512; @@ -107,7 +107,6 @@ static int handle(struct vfs_request *req) { ata_read(id - HANDLE_ATA, sector, buf); virt_cpy_to(req->caller->pages, req->output.buf, buf, len); return len; - } default: panic_invalid_state(); } @@ -116,8 +115,11 @@ static int handle(struct vfs_request *req) { case HANDLE_VGA: { void *vga = (void*)0xB8000; req_preprocess(req, 80*25*2); - virt_cpy_from(req->caller->pages, vga + req->offset, - req->input.buf, req->input.len); + if (!virt_cpy_from(req->caller->pages, vga + req->offset, + req->input.buf, req->input.len)) + { + return -EFAULT; + } return req->input.len; } default: return -1; diff --git a/src/kernel/mem/virt.c b/src/kernel/mem/virt.c index f0dca06..bff4b5e 100644 --- a/src/kernel/mem/virt.c +++ b/src/kernel/mem/virt.c @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -52,12 +53,13 @@ bool virt_iter_next(struct virt_iter *iter) { return true; } -bool virt_cpy( +size_t virt_cpy( struct pagedir *dest_pages, void __user *dest, - struct pagedir *src_pages, const void __user *src, size_t length) + struct pagedir *src_pages, const void __user *src, + size_t length, struct virt_cpy_error *err) { struct virt_iter dest_iter, src_iter; - size_t cur_len; + size_t total = 0, partial; virt_iter_new(&dest_iter, dest, length, dest_pages, true, true); virt_iter_new( &src_iter, (userptr_t)src, length, src_pages, true, false); @@ -65,19 +67,26 @@ bool virt_cpy( src_iter.frag_len = 0; for (;;) { - if (dest_iter.frag_len <= 0) - if (!virt_iter_next(&dest_iter)) break; - if ( src_iter.frag_len <= 0) - if (!virt_iter_next( &src_iter)) break; + if (dest_iter.frag_len <= 0 && !virt_iter_next(&dest_iter)) break; + if ( src_iter.frag_len <= 0 && !virt_iter_next( &src_iter)) break; - cur_len = min(src_iter.frag_len, dest_iter.frag_len); - memcpy(dest_iter.frag, src_iter.frag, cur_len); + partial = min(src_iter.frag_len, dest_iter.frag_len); + total += partial; + memcpy(dest_iter.frag, src_iter.frag, partial); - dest_iter.frag_len -= cur_len; - dest_iter.frag += cur_len; - src_iter.frag_len -= cur_len; - src_iter.frag += cur_len; + dest_iter.frag_len -= partial; + dest_iter.frag += partial; + src_iter.frag_len -= partial; + src_iter.frag += partial; } - return !(dest_iter.error || src_iter.error); + if (err) { + err->read_fail = src_iter.error; + err->write_fail = dest_iter.error; + } + if (src_iter.error || dest_iter.error) + assert(total != length); + else + assert(total == length); + return total; } diff --git a/src/kernel/mem/virt.h b/src/kernel/mem/virt.h index 7d95b3b..cf22a75 100644 --- a/src/kernel/mem/virt.h +++ b/src/kernel/mem/virt.h @@ -18,6 +18,10 @@ struct virt_iter { bool _writeable; }; +struct virt_cpy_error { + bool read_fail, write_fail; +}; + /* if pages == NULL, create an iterator over physical memory. */ void virt_iter_new( struct virt_iter *iter, void __user *virt, size_t length, @@ -25,17 +29,20 @@ void virt_iter_new( bool virt_iter_next(struct virt_iter *); -bool virt_cpy( +size_t virt_cpy( struct pagedir *dest_pages, void __user *dest, - struct pagedir *src_pages, const void __user *src, size_t length); + struct pagedir *src_pages, const void __user *src, + size_t length, struct virt_cpy_error *err); -static inline bool virt_cpy_to(struct pagedir *dest_pages, // physical -> virtual +/* copies to virtual memory, returns true on success */ +static inline bool virt_cpy_to(struct pagedir *dest_pages, void __user *dest, const void *src, size_t length) { - return virt_cpy(dest_pages, dest, NULL, (userptr_t)src, length); + return length == virt_cpy(dest_pages, dest, NULL, (userptr_t)src, length, NULL); } -static inline bool virt_cpy_from(struct pagedir *src_pages, // virtual -> physical +/* copies from virtual memory, returns true on success */ +static inline bool virt_cpy_from(struct pagedir *src_pages, void *dest, const void __user *src, size_t length) { - return virt_cpy(NULL, (userptr_t)dest, src_pages, src, length); + return length == virt_cpy(NULL, (userptr_t)dest, src_pages, src, length, NULL); } diff --git a/src/kernel/pipe.c b/src/kernel/pipe.c index edd1388..2dc98fe 100644 --- a/src/kernel/pipe.c +++ b/src/kernel/pipe.c @@ -27,6 +27,7 @@ bool pipe_joinqueue(struct handle *h, bool wants_write, void pipe_trytransfer(struct handle *h) { struct process *rdr, *wtr; + struct virt_cpy_error cpyerr; int len; assert(h); if (!h->pipe.sister) { @@ -43,12 +44,12 @@ void pipe_trytransfer(struct handle *h) { len = min(rdr->waits4pipe.len, wtr->waits4pipe.len); - if (!virt_cpy( + virt_cpy( rdr->pages, rdr->waits4pipe.buf, - wtr->pages, wtr->waits4pipe.buf, len)) - { + wtr->pages, wtr->waits4pipe.buf, + len, &cpyerr); + if (cpyerr.read_fail || cpyerr.write_fail) panic_unimplemented(); - } h->pipe.queued = h->pipe.queued->waits4pipe.next; h->pipe.sister->pipe.queued = h->pipe.sister->pipe.queued->waits4pipe.next; process_transition(rdr, PS_RUNNING); diff --git a/src/kernel/syscalls.c b/src/kernel/syscalls.c index c5e12b4..786578a 100644 --- a/src/kernel/syscalls.c +++ b/src/kernel/syscalls.c @@ -292,10 +292,13 @@ long _syscall_fs_respond(void __user *buf, long ret, int flags) { // if this vfsop outputs data and ret is positive, it's the length of the buffer // TODO document ret = min(ret, capped_cast32(req->output.len)); - if (!virt_cpy(req->caller->pages, req->output.buf, - process_current->pages, buf, ret)) { - // how should this error even be handled? TODO - } + struct virt_cpy_error err; + virt_cpy(req->caller->pages, req->output.buf, + process_current->pages, buf, ret, &err); + + if (err.read_fail) + panic_unimplemented(); + /* write failures are ignored */ } process_current->handled_req = NULL; diff --git a/src/kernel/vfs/request.c b/src/kernel/vfs/request.c index 87d6208..b056a31 100644 --- a/src/kernel/vfs/request.c +++ b/src/kernel/vfs/request.c @@ -4,6 +4,7 @@ #include #include #include +#include #include void vfsreq_create(struct vfs_request req_) { @@ -102,6 +103,7 @@ void vfs_backend_tryaccept(struct vfs_backend *backend) { void vfs_backend_user_accept(struct vfs_request *req) { struct process *handler; struct fs_wait_response res = {0}; + struct virt_cpy_error cpyerr; int len = 0; assert(req && req->backend && req->backend->user.handler); @@ -115,9 +117,15 @@ void vfs_backend_user_accept(struct vfs_request *req) { if (req->input.buf) { len = min(req->input.len, handler->awaited_req.max_len); - if (!virt_cpy(handler->pages, handler->awaited_req.buf, - req->input.kern ? NULL : req->caller->pages, req->input.buf, len)) - goto fail; // can't copy buffer + virt_cpy(handler->pages, handler->awaited_req.buf, + req->input.kern ? NULL : req->caller->pages, req->input.buf, + len, &cpyerr); + if (cpyerr.write_fail) + panic_unimplemented(); + if (cpyerr.read_fail) { + vfsreq_finish_short(req, -EFAULT); + return; + } } res.len = len; @@ -129,15 +137,15 @@ void vfs_backend_user_accept(struct vfs_request *req) { if (!virt_cpy_to(handler->pages, handler->awaited_req.res, &res, sizeof res)) - goto fail; // can't copy response struct + { + panic_unimplemented(); + } process_transition(handler, PS_RUNNING); handler->handled_req = req; req->backend->user.handler = NULL; regs_savereturn(&handler->regs, 0); return; -fail: - panic_unimplemented(); // TODO } void vfs_backend_refdown(struct vfs_backend *b) { diff --git a/src/shared/errno.h b/src/shared/errno.h new file mode 100644 index 0000000..a0b5731 --- /dev/null +++ b/src/shared/errno.h @@ -0,0 +1,3 @@ +#pragma once + +#define EFAULT 2 diff --git a/src/user/driver/tmpfs.c b/src/user/driver/tmpfs.c index 80c0196..6e5b45e 100644 --- a/src/user/driver/tmpfs.c +++ b/src/user/driver/tmpfs.c @@ -23,6 +23,7 @@ static struct node *lookup(const char *path, size_t len) { } static struct node *tmpfs_open(const char *path, struct fs_wait_response *res) { + struct node *node; if (res->len == 0) return NULL; path++; res->len--; @@ -32,21 +33,19 @@ static struct node *tmpfs_open(const char *path, struct fs_wait_response *res) { // no directory support (yet) if (memchr(path, '/', res->len)) return NULL; - if (res->flags & OPEN_CREATE) { - if (lookup(path, res->len)) return NULL; /* already exists */ - struct node *new = malloc(sizeof *new); - memset(new, 0, sizeof *new); + node = lookup(path, res->len); + if (!node && (res->flags & OPEN_CREATE)) { + node = malloc(sizeof *node); + memset(node, 0, sizeof *node); char *namebuf = malloc(res->len); memcpy(namebuf, path, res->len); - new->name = namebuf; - new->namelen = res->len; - new->next = root; - root = new; - return new; + node->name = namebuf; + node->namelen = res->len; + node->next = root; + root = node; } - - return lookup(path, res->len); + return node; } void tmpfs_drv(void) { diff --git a/src/user/tests/main.c b/src/user/tests/main.c index 5bfd3aa..b4587f9 100644 --- a/src/user/tests/main.c +++ b/src/user/tests/main.c @@ -1,8 +1,9 @@ #define TEST_MACROS -#include -#include +#include #include #include +#include +#include static void run_forked(void (*fn)()) { if (!fork()) { @@ -16,6 +17,7 @@ static void run_forked(void (*fn)()) { } +const char *tmpfilepath = "/tmp/.test_internal"; static void test_await(void) { /* creates 16 child processes, each returning a different value. then checks @@ -186,6 +188,17 @@ static void test_malloc(void) { free(p1); } +static void test_efault(void) { + char *invalid = (void*)0x1000; + handle_t h; + + assert((h = _syscall_open(tmpfilepath, strlen(tmpfilepath), OPEN_CREATE))); + assert(_syscall_write(h, "dzwdz sucks ass!", 16, 0) == 16); + assert(_syscall_write(h, invalid, 16, 0) == -EFAULT); + assert(_syscall_write(h, "dzwdz is cool!!!", 16, 0) == 16); + close(h); +} + static void test_misc(void) { assert(_syscall(~0, 0, 0, 0, 0) < 0); /* try making an invalid syscall */ } @@ -201,5 +214,6 @@ void test_all(void) { run_forked(test_malloc); run_forked(test_pipe); run_forked(test_semaphore); + run_forked(test_efault); run_forked(test_misc); } -- cgit v1.2.3