From 75ccf9841c6997d2c8d7481d26479eadf45e399b Mon Sep 17 00:00:00 2001 From: Nexoniarz Date: Mon, 14 Sep 2026 22:12:28 +0200 Subject: [PATCH] errno: -1 is EPERM, and four places were saying it by accident Ctrl-C killed the shell. consoleread() breaks off the read in progress when SIGINT arrives and returned -1 to do it -- and -1 is -EPERM by the time it reaches a program, so every program that handles SIGINT by carrying on saw a permanent error instead of an interrupted call: zsh: error on TTY read: operation not permitted schinitd: restarting os.schos.console-shell and the next Ctrl-C did it to the replacement. EINTR now, which is what a signal-interrupted read is required to give. Verified: Ctrl-C an interruptible command, shell survives, zero restarts. Same mistake, same file family: link, unlink, rename and renameat returned -1 on every error path. Callers branch on errno -- `if (unlink(p) < 0 && errno != ENOENT)` is the standard shape, and against EPERM it always complains -- which is why zsh reported failures for things that had worked. They return ENOENT, EEXIST, EXDEV, EISDIR, ENOTEMPTY and EFAULT now. About ninety other `return -1`s remain in sys_fs.c; these four are done because these four were what a program tripped over. /dev/mem's unknown minors return ENODEV rather than -1 too. /dev/urandom was a linear congruential generator seeded with a **constant**, so every boot produced the same bytes in the same order -- not weak, the same. A key, a nonce, a temporary filename: shared with every SchOS machine that had ever run. Seeded at boot from RDRAND where the CPU has it, from the cycle counter and the wall clock where it does not, and re-stirred per byte. Still an LCG and still not a CSPRNG; what changed is the part that was dangerous rather than merely poor. **Not fixed, and recorded as such:** rename() still will not replace an existing target, so zsh's history save fails on every exit after the first. An attempt to unlink the target first was reverted -- the directory is an HTree, dirunlink followed by dirlink of the same name did not behave, and it panicked in filewrite with the old file already gone. Losing both files is worse than refusing, so it refuses until the HTree's slot reuse is understood. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VQ7Qx8E1q1M1bbCq8QLHS2 --- Makefile | 40 ++++++++++++++ cmd/schinitd/main.c | 18 ++++++- cmd/vgpud/main.c | 6 ++- docs/architecture/devices.md | 20 +++++++ docs/architecture/filesystem.md | 42 +++++++++++++++ docs/architecture/terminals.md | 23 ++++++++ include/defs.h | 2 + kernel/core/console.c | 19 ++++++- kernel/core/main.c | 20 +++---- kernel/dev/cdev.c | 94 +++++++++++++++++++++++++++++++-- kernel/sys/sys_fs.c | 88 +++++++++++++++++++++--------- 11 files changed, 330 insertions(+), 42 deletions(-) diff --git a/Makefile b/Makefile index 9ac77ce..f8125dd 100644 --- a/Makefile +++ b/Makefile @@ -505,14 +505,54 @@ UEFI_CFLAGS = --target=x86_64-unknown-windows -ffreestanding -fshort-wchar # nothing to do, and the only symptom was a boot screen that disagreed with the # kernel about what the logo is. The kernel half never showed it because those # objects are built with -MD -MP and pick up header changes on their own. +# +# **The loader is the one thing here that GCC cannot build.** It is a PE/COFF +# binary for the Microsoft ABI, which clang cross-targets and gcc does not, so +# this rule needs clang and lld-link and no other rule does. +# +# That matters more than it reads, because of *where* it sits: `make` stops at +# the first failed rule, and this one runs before the sysroot is staged and the +# filesystem image is built. So on a machine without clang, touching the loader +# does not produce "the loader did not build" -- it produces a new kernel, an +# old userland, and a boot that behaves like a mix of the two, with the real +# cause forty lines up the screen. That happened, and it cost an afternoon of +# looking at the wrong thing. +# +# Hence the check and the message. It is worth the four lines. $(BUILD)/uefi/BOOTX64.EFI: boot/uefi/main.c boot/uefi/efi.h include/schboot.h \ boot/common/ui.c boot/common/ui.h \ include/logo.h include/theme.h include/font.h @mkdir -p $(BUILD)/uefi + @command -v clang >/dev/null 2>&1 || { \ + echo ""; \ + echo " The UEFI loader needs clang and lld-link, and nothing else here does."; \ + echo " It is the only PE/COFF binary in the tree -- gcc cannot target it."; \ + echo ""; \ + echo " On NixOS: nix-shell --run 'NIX_HARDENING_ENABLE=\"\" make'"; \ + echo " Elsewhere: install clang and lld, then make again"; \ + echo ""; \ + test -f $@ && { \ + echo " There is an existing loader at $@."; \ + echo " To keep it and build everything else: make UEFI_KEEP=1"; \ + echo ""; }; \ + exit 1; } clang $(UEFI_CFLAGS) -c boot/uefi/main.c -o $(BUILD)/uefi/main.obj clang $(UEFI_CFLAGS) -c boot/common/ui.c -o $(BUILD)/uefi/ui.obj lld-link -subsystem:efi_application -entry:EfiMain -nodefaultlib $(BUILD)/uefi/main.obj $(BUILD)/uefi/ui.obj -out:$@ +# Keep the loader that is already built and carry on with the rest. +# +# For the case the message above describes: a machine with no clang, where the +# loader has not meaningfully changed and the thing being worked on is the +# kernel or the userland. It is opt-in and prints what it is doing, because a +# build that quietly ships a stale bootloader is how you spend an evening +# debugging code that is not running. +ifeq ($(UEFI_KEEP),1) +$(BUILD)/uefi/BOOTX64.EFI: + @test -f $@ || { echo "make UEFI_KEEP=1: there is no loader to keep"; exit 1; } + @echo "uefi: keeping the existing loader (UEFI_KEEP=1) -- it may be out of date" +endif + uefi: $(BUILD)/uefi/BOOTX64.EFI # The EFI System Partition, as a directory. QEMU serves a directory as FAT diff --git a/cmd/schinitd/main.c b/cmd/schinitd/main.c index fb7db0b..f19401e 100644 --- a/cmd/schinitd/main.c +++ b/cmd/schinitd/main.c @@ -507,8 +507,11 @@ note_exit(unsigned int cookie, int pid, int status) // job that was killed or faulted never matches this and is restarted the // way KeepAlive promises. See SCHINIT_EXIT_NORESTART. if ((status & 0x7f) == 0 && ((status >> 8) & 0xff) == SCHINIT_EXIT_NORESTART) { - out("schinitd: "); out(j->label); - out(" has nothing to do on this machine; not restarting\n"); + // Not a failure: the job looked, found the machine did not have what + // it drives, and said so properly. Worth reading once and not every + // boot on a machine that will never have one. + vout("schinitd: "); vout(j->label); + vout(" has nothing to do on this machine; not restarting\n"); return; } @@ -640,6 +643,17 @@ main(void) vout("schinitd: ready\n"); + // The one line a working boot prints, and the last one. + // + // Here rather than in the kernel because "the machine is up" is not true + // until the jobs that make it usable have been started, and because + // anything printed before this point would come after it on screen. The + // kernel's whole boot is a kinfo() and is in the klog; this is the part a + // person is meant to read. + if (!verbose_boot) + out("\nWelcome to SchOS!" + " (F8 at boot for the full log, or dmesg once you are in)\n\n"); + // From here on this program does two things: answer PID 1's heartbeat, and // act on the exits it reports. There is no wait() in this program and // there cannot be -- the children are not ours. diff --git a/cmd/vgpud/main.c b/cmd/vgpud/main.c index b4e2752..9b9c1ce 100644 --- a/cmd/vgpud/main.c +++ b/cmd/vgpud/main.c @@ -110,7 +110,11 @@ die(const char *why) static void nothing_to_do(const char *why) { - puts("vgpud: "); puts(why); puts("\n"); + // Quiet on a quiet boot: "this machine has no virtio-gpu" is a fact about + // the hardware and not a fault, and it is the same fact every time. It is + // still an exit the supervisor is told about, which is what stops it being + // restarted fifteen times in forty-five seconds. + vputs("vgpud: "); vputs(why); vputs("\n"); exit_group(SCHINIT_EXIT_NORESTART); } diff --git a/docs/architecture/devices.md b/docs/architecture/devices.md index 3adb72f..d82513c 100644 --- a/docs/architecture/devices.md +++ b/docs/architecture/devices.md @@ -250,6 +250,26 @@ existing mechanisms for keeping that true mechanically rather than by intention. --- +## `/dev/random` and `/dev/urandom` + +Seeded once at boot, from `RDRAND` where the CPU has it and from the cycle +counter mixed with the wall clock where it does not. A verbose boot says which: + +``` +random: seeded from RDRAND +``` + +That is worth a section only because of what it replaced. It was a linear +congruential generator seeded with a **constant**, so every boot produced the +same bytes in the same order — not weak, *the same*. A program asking for a +key, a nonce, a temporary filename or a hash seed got one it shared with every +other SchOS machine that had ever run. + +**It is still not a CSPRNG and does not pretend to be.** It is an LCG, re-stirred +from the cycle counter on each byte so two readers do not continue one +sequence. What changed is the property that made it dangerous rather than +merely poor. + ## What it does not do - **It is a flat list, not a device tree.** There is no parent/child nesting, diff --git a/docs/architecture/filesystem.md b/docs/architecture/filesystem.md index 59328f3..2c32490 100644 --- a/docs/architecture/filesystem.md +++ b/docs/architecture/filesystem.md @@ -189,8 +189,50 @@ selftest, `t` text specimen, `n` the netdev ARP selftest. --- +## Errno, and why `return -1` is a bug + +A syscall that fails returns a negative errno. `-1` is therefore **`-EPERM`**, +"operation not permitted" — and about ninety error paths in `kernel/sys/sys_fs.c` +still say `return -1` because that is what xv6 did, where the convention was +just "negative means no". + +It is not a cosmetic difference. Callers branch on `errno`, and the standard +shape for removing a file you are not sure about is: + +```c +if (unlink(p) < 0 && errno != ENOENT) + complain(); +``` + +Against `-1` every such test takes the complaining branch. That is how it was +found — zsh, saving its history, reported failures for things that had +succeeded: + +``` +zsh: locking failed for /root/.zsh_history: operation not permitted +zsh: failed to write history file /root/.zsh_history.new: operation not permitted +``` + +`link`, `unlink`, `rename` and `renameat` return real values now — `ENOENT`, +`EEXIST`, `EXDEV`, `EISDIR`, `ENOTEMPTY`, `EFAULT`. **The rest of the file does +not**, and that is the honest state: these four were corrected because these +four were the ones a program tripped over. + ## What it does not do +- **`rename()` will not replace an existing target.** POSIX says it must, and + write-to-a-temporary-then-rename is how every careful program saves a file — + so the second save of anything fails with `EEXIST` where it should succeed. + It is the reason `zsh: can't rename /root/.zsh_history.new to $HISTFILE` + appears on every shell exit after the first. + + An attempt to fix it by unlinking the target first was **reverted**: the + directory is an HTree, `dirunlink` followed by `dirlink` of the same name did + not behave, and the failure mode was the worst possible one — the old file + removed and the new link then refused, losing both. Doing this properly means + understanding the HTree's slot reuse, not patching around it. + + - **No page cache.** `mmap` of a file reads the whole thing in at map time, and writable `MAP_SHARED` of a file is refused rather than faked. This is the single largest gap in the layer. diff --git a/docs/architecture/terminals.md b/docs/architecture/terminals.md index 155f1ac..c3f48f6 100644 --- a/docs/architecture/terminals.md +++ b/docs/architecture/terminals.md @@ -238,6 +238,29 @@ immediate wrap, a reverse-video `#` appeared above every prompt. Anything that moves the cursor deliberately -- CR, LF, backspace, an escape sequence -- clears the debt. +## Ctrl-C used to kill the shell + +Worth its own note, because the cause was one character and the effect was a +machine you could not use. + +When Ctrl-C arrives, `consoleread()` discards the partial line and breaks off +the read in progress. It returned **`-1`** to do so — and `-1` is `-EPERM`. So a +program that handles `SIGINT` by carrying on saw a permanent error instead of +an interrupted call, and exited: + +``` +zsh: error on TTY read: operation not permitted +schinitd: restarting os.schos.console-shell +``` + +and then the next Ctrl-C did the same thing to the replacement. A shell that +cannot survive Ctrl-C is a shell nobody can use, and the supervisor faithfully +restarting it made it a loop. + +It returns **`EINTR`** now, which is what a signal-interrupted read is required +to give: the whole convention — retry, or return to the caller — is built on +receiving that and not something else. + ## What it does not do - **No scrollback.** The grid is exactly the visible screen. Shift+PageUp does diff --git a/include/defs.h b/include/defs.h index a09cfd3..44d4f24 100644 --- a/include/defs.h +++ b/include/defs.h @@ -404,6 +404,8 @@ int ksym_fmt(char *buf, int cap, uint64 addr); void *dma_alloc(uint64 size, uint64 align); void dma_free(void*, uint64 size); addr_t dma_phys(const void*); +// /dev/random and /dev/urandom. Seeded once at boot; see kernel/dev/cdev.c. +void random_init(void); uint64 dma_free_bytes(void); uint64 dma_total_bytes(void); diff --git a/kernel/core/console.c b/kernel/core/console.c index 84ae380..62e6ac5 100644 --- a/kernel/core/console.c +++ b/kernel/core/console.c @@ -10,6 +10,7 @@ #include "irq.h" #include "param.h" #include "traps.h" +#include "errno.h" #include "spinlock.h" #include "sleeplock.h" #include "fs.h" @@ -611,11 +612,27 @@ consoleread(struct inode *ip, uint64 off, char *dst, int n) ilock(ip); return 0; // end of input } + // Interrupted, not forbidden. + // + // This returned -1, and -1 is -EPERM. A read broken off by Ctrl-C + // therefore reached userspace as "operation not permitted", which is a + // permanent error -- so every program that handles SIGINT by carrying on + // saw a fatal one instead and exited: + // + // zsh: error on TTY read: operation not permitted + // schinitd: restarting os.schos.console-shell + // + // and then did it again, because the next Ctrl-C did the same thing. A + // shell that cannot survive Ctrl-C is a shell nobody can use. + // + // EINTR is the answer a signal-interrupted read is required to give, and + // the whole convention -- retry, or return to the caller -- is built on + // getting it rather than something else. if (myproc()->killed || v->in.abort_read) { v->in.abort_read = 0; release(&input_lock); ilock(ip); - return -1; + return -EINTR; } sleep(&v->in.r, &input_lock); } diff --git a/kernel/core/main.c b/kernel/core/main.c index 2d7935b..5e22922 100644 --- a/kernel/core/main.c +++ b/kernel/core/main.c @@ -221,22 +221,22 @@ kmain(uint32 fb_addr) // sinit: kthread_create needs a usable process table. From here on Ctrl-G // on the console answers, with no filesystem mounted and nothing in // userspace started -- which is the whole point of putting it this early. + // After timeinit(), which is what boot_epoch comes from, and before any + // program can ask for a random byte. + random_init(); netuser_init(); dbg_thread_start(); if (boot_verbose) cprintf("verbose boot: dbg thread started\n"); - // The one line a working boot prints. + // The greeting is not printed here. // - // Everything above this is a kinfo(), which means it is in the klog and not - // on the screen -- the whole boot is still there, it is simply not the first - // thing anybody has to read every morning. What is left is the fact that the - // machine came up, and where the rest went if it did not. + // It was, and it landed before userspace had said anything -- so on a quiet + // boot it was followed by the last few lines of `schinit` and whatever the + // jobs had to say, which is the wrong way round for a line whose whole job is + // to be the last thing you read. It belongs at the end of the thing it is + // announcing, and the end of the thing it is announcing is in userspace. // - // A verbose boot has already said thirty other things and does not need a - // greeting on top of them. - if (!boot_verbose) - cprintf("\nWelcome to SchOS! (F8 at boot for the full log, or dmesg once you are in)\n\n"); - + // schinitd prints it, once its RunAtLoad jobs are away. See cmd/schinitd. mpmain(); } diff --git a/kernel/dev/cdev.c b/kernel/dev/cdev.c index 9108ba8..a557b4b 100644 --- a/kernel/dev/cdev.c +++ b/kernel/dev/cdev.c @@ -12,7 +12,84 @@ extern int consoleread(struct inode*, uint64, char*, int); extern int consolewrite(struct inode*, uint64, char*, int); extern int mouseread(struct inode*, uint64, char*, int); -static uint64 rseed = 0x2A99554433221100ULL; +// --- /dev/random and /dev/urandom ------------------------------------------- +// +// This was a linear congruential generator seeded with a constant, which means +// **every boot produced the same bytes in the same order**. Not weak: the same. +// A program asking for a key, a nonce, a temporary filename or a hash seed got +// one it shared with every other SchOS machine that had ever run. +// +// What is here now is still not a CSPRNG and does not pretend to be. It is an +// LCG that is *seeded* from something real and re-stirred from the cycle +// counter on every read, which fixes the one property that made the old one +// dangerous rather than merely poor. See "What it does not do" in +// docs/architecture/devices.md. +static uint64 rseed; +static int rseed_ready; +static void rseed_init(void); + +// RDRAND, if this CPU has it. CPUID leaf 1, ECX bit 30. +// +// Checked rather than assumed: the instruction faults with #UD on a CPU that +// does not implement it, and "the random device is a kernel panic" is a worse +// failure than a weak seed. +static int +rdrand64(uint64 *out) +{ + uint32 a, b, c, d; + unsigned char ok = 0; + uint64 v = 0; + + cpuid_leaf(1, &a, &b, &c, &d); + if (!(c & (1u << 30))) + return 0; + + // Ten tries, per Intel's own guidance: the instruction is allowed to fail + // when its entropy pool is momentarily empty, and it says so in CF. + for (int i = 0; i < 10; i++) { + asm volatile("rdrand %0; setc %1" : "=r"(v), "=qm"(ok) :: "cc"); + if (ok) { *out = v; return 1; } + } + return 0; +} + +// Seeded at boot rather than on the first read. +// +// Lazy would work, but it puts the one line that says where the entropy came +// from in the middle of whatever the machine was doing hours later, and it +// makes the first reader pay for the CPUID. Called from main(); the lazy path +// stays as a belt-and-braces for anything that reads before init runs. +void +random_init(void) +{ + if (!rseed_ready) + rseed_init(); +} + +static void +rseed_init(void) +{ + uint64 v; + + rseed_ready = 1; + if (rdrand64(&v)) { + rseed = v; + kinfo("random: seeded from RDRAND\n"); + return; + } + + // No RDRAND. The cycle counter is the only thing here that is different on + // every boot and not written down anywhere -- it is not entropy in any + // serious sense, but it is not a constant either, which was the whole + // problem. Mixed with the wall clock so that two machines starting at the + // same instant do not agree. + rseed = rdtsc(); + rseed ^= boot_epoch * 0x9E3779B97F4A7C15ULL; + rseed ^= (uint64)(uintptr_t)&rseed; + if (rseed == 0) + rseed = 0x9E3779B97F4A7C15ULL; + kinfo("random: no RDRAND; seeded from the cycle counter and the clock\n"); +} int nullwrite(struct inode *ip, uint64 off, char *dst, int n) @@ -36,9 +113,16 @@ zeroread(struct inode *ip, uint64 off, char *dst, int n) int randomread(struct inode *ip, uint64 off, char *dst, int n) { + if(!rseed_ready) + rseed_init(); + for(int i = 0; i < n; i++){ + // Re-stirred from the cycle counter every byte, so two reads a moment + // apart do not continue one sequence -- and so that a process cannot + // predict the next caller's bytes from its own. + rseed ^= rdtsc(); rseed = rseed * 6364136223846793005ULL + 1442695040888963407ULL; - dst[i] = (rseed >> 32) & 0xFF; + dst[i] = (rseed >> 33) & 0xFF; } return n; } @@ -72,7 +156,9 @@ memread(struct inode *ip, uint64 off, char *dst, int n) case 7: return zeroread(ip, off, dst, n); // /dev/full reads as zeros case 8: case 9: return randomread(ip, off, dst, n); - default: return -1; + // Not "operation not permitted", which is what -1 means once it reaches + // userspace. There is no device on this minor. + default: return -ENODEV; } } @@ -88,7 +174,7 @@ memwrite(struct inode *ip, uint64 off, char *dst, int n) case 5: case 8: case 9: return nullwrite(ip, off, dst, n); - default: return -1; + default: return -ENODEV; } } diff --git a/kernel/sys/sys_fs.c b/kernel/sys/sys_fs.c index 13da263..bea244b 100644 --- a/kernel/sys/sys_fs.c +++ b/kernel/sys/sys_fs.c @@ -686,45 +686,76 @@ addr_t sys_mknod(void) return 0; } +// link() and rename() answer with real errno values now, and it is worth saying +// why that needed doing. +// +// Every failure in these used to be `return -1`, and -1 is -EPERM by the time +// it reaches a program. So linking a file that does not exist reported +// "operation not permitted" rather than "no such file" -- not a cosmetic +// difference, because a caller distinguishes the two and acts on it. +// +// zsh is how it surfaced. Its history locking links a lockfile and its history +// save renames a temporary over the real file, and on a machine with no +// history yet both legitimately fail the first time: +// +// zsh: locking failed for /root/.zsh_history: operation not permitted +// zsh: failed to write history file /root/.zsh_history.new: operation not +// permitted +// +// -- neither of which is what happened. About ninety other `return -1`s remain +// in this file. These are corrected because these were the ones found, and +// renameat() is corrected alongside rename() because musl's rename() calls it. addr_t sys_link(void) { char name[DIRSIZ], *new, *old; struct inode *dp, *ip; - if (argstr(0, &old) < 0 || argstr(1, &new) < 0) return -1; + int err = EPERM; + if (argstr(0, &old) < 0 || argstr(1, &new) < 0) return -EFAULT; begin_op(); - if (!(ip = namei(old))) { end_op(); return -1; } + if (!(ip = namei(old))) { end_op(); return -ENOENT; } ilock(ip); - if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -1; } + // The one place EPERM is right: POSIX names it for a directory. + if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -EPERM; } ip->nlink++; iupdate(ip); iunlock(ip); - if (!(dp = nameiparent(new, name))) goto bad_link; + if (!(dp = nameiparent(new, name))) { err = ENOENT; goto bad_link; } ilock(dp); - if (dp->dev != ip->dev || dirlink(dp, name, ip->inum) < 0) { iunlockput(dp); goto bad_link; } + if (dp->dev != ip->dev) { iunlockput(dp); err = EXDEV; goto bad_link; } + if (dirlink(dp, name, ip->inum) < 0) { iunlockput(dp); err = EEXIST; goto bad_link; } iunlockput(dp); iput(ip); end_op(); return 0; bad_link: ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); - return -1; + return -err; } +// Same correction as link() and rename() above, and in the same path. +// +// **ENOENT is the one that mattered.** A program that removes a file it is not +// sure exists writes `if (unlink(p) < 0 && errno != ENOENT) complain()`, which +// is the standard shape -- and against -1/EPERM every such test took the +// complaining branch. zsh saving its history is one of them, and it went on +// reporting a failure after the history had in fact been written. addr_t sys_unlink(void) { struct inode *ip, *dp; char name[DIRSIZ], *path; uint off; - if (argstr(0, &path) < 0) return -1; + int err = EPERM; + if (argstr(0, &path) < 0) return -EFAULT; begin_op(); - if (!(dp = nameiparent(path, name))) { end_op(); return -1; } + if (!(dp = nameiparent(path, name))) { end_op(); return -ENOENT; } ilock(dp); - if (namecmp(name, ".") == 0 || namecmp(name, "..") == 0) goto bad_unlink; - if (!(ip = dirlookup(dp, name, &off))) goto bad_unlink; + // "." and ".." are refused by name, which is what POSIX asks for. + if (namecmp(name, ".") == 0 || namecmp(name, "..") == 0) { err = EINVAL; goto bad_unlink; } + if (!(ip = dirlookup(dp, name, &off))) { err = ENOENT; goto bad_unlink; } ilock(ip); if (ip->nlink < 1) panic("unlink: nlink < 1"); - if (ip->type == T_DIR && dirempty(ip) != 1) { iunlockput(ip); goto bad_unlink; } - if (dirunlink(dp, name, off) < 0) { iunlockput(ip); goto bad_unlink; } + if (ip->type == T_DIR && dirempty(ip) != 1) { iunlockput(ip); err = ENOTEMPTY; goto bad_unlink; } + if (dirunlink(dp, name, off) < 0) { iunlockput(ip); err = EIO; goto bad_unlink; } if (ip->type == T_DIR) { dp->nlink--; iupdate(dp); } iunlockput(dp); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return 0; -bad_unlink: iunlockput(dp); end_op(); return -1; +bad_unlink: iunlockput(dp); end_op(); return -err; } addr_t sys_exec(void) @@ -797,25 +828,32 @@ addr_t sys_rename(void) char oldname[DIRSIZ], newname[DIRSIZ]; uint off; struct dirent de; - if (argstr(0, &oldpath) < 0 || argstr(1, &newpath) < 0) return -1; + int err = EPERM; + if (argstr(0, &oldpath) < 0 || argstr(1, &newpath) < 0) return -EFAULT; begin_op(); - if (!(ip = namei(oldpath))) { end_op(); return -1; } + if (!(ip = namei(oldpath))) { end_op(); return -ENOENT; } ilock(ip); - if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -1; } + // Renaming a directory needs the ".." fixup and the loop check that are + // not written here. EISDIR names what is missing; EPERM did not. + if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -EISDIR; } ip->nlink++; iupdate(ip); iunlock(ip); dp_old = nameiparent(oldpath, oldname); dp_new = nameiparent(newpath, newname); if (!dp_old || !dp_new || dp_old->dev != ip->dev || dp_new->dev != ip->dev) { + err = (!dp_old || !dp_new) ? ENOENT : EXDEV; if (dp_old) iput(dp_old); if (dp_new) iput(dp_new); goto bad_rename; } ilock(dp_new); - if (dirlink(dp_new, newname, ip->inum) < 0) { iunlockput(dp_new); iput(dp_old); goto bad_rename; } + if (dirlink(dp_new, newname, ip->inum) < 0) { + iunlockput(dp_new); iput(dp_old); err = EEXIST; goto bad_rename; + } iunlockput(dp_new); ilock(dp_old); if (!(ip_src = dirlookup(dp_old, oldname, &off))) { - iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); + end_op(); return -ENOENT; } iput(ip_src); memset(&de, 0, sizeof(de)); @@ -824,7 +862,7 @@ addr_t sys_rename(void) ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return 0; bad_rename: - ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -err; } addr_t sys_symlink(void) @@ -1493,26 +1531,28 @@ addr_t sys_renameat(void) uint off; struct dirent de; begin_op(); - if (!(ip = at_namei(olddirfd, oldpath))) { end_op(); return -1; } + if (!(ip = at_namei(olddirfd, oldpath))) { end_op(); return -ENOENT; } ilock(ip); - if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -1; } + if (ip->type == T_DIR) { iunlockput(ip); end_op(); return -EISDIR; } ip->nlink++; iupdate(ip); iunlock(ip); dp_old = at_nameiparent(olddirfd, oldpath, oldname); dp_new = at_nameiparent(newdirfd, newpath, newname); if (!dp_old || !dp_new || dp_old->dev != ip->dev || dp_new->dev != ip->dev) { + int e = (!dp_old || !dp_new) ? ENOENT : EXDEV; if (dp_old) iput(dp_old); if (dp_new) iput(dp_new); - ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -e; } ilock(dp_new); if (dirlink(dp_new, newname, ip->inum) < 0) { iunlockput(dp_new); iput(dp_old); - ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -EEXIST; } iunlockput(dp_new); ilock(dp_old); if (!(ip_src = dirlookup(dp_old, oldname, &off))) { - iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return -1; + iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); + end_op(); return -ENOENT; } iput(ip_src); memset(&de, 0, sizeof(de));