cube(2): a refusal with two minus signs is not a refusal — and the walk it hung
The walk with no store behind it served ~200,000 invented records a minute and never ended.
Half of that was fixed and proven in fc057820f: the driver asks whether the store can be read
before either walk op answers anything, and it refuses. The client was still handed
spaces returned 0, len=0, cursor=1
— success, no space, a cursor one further on — so it copied the space it was handed out of a
buffer the kernel never wrote, and with the cursor moving by itself the rule that ends every
other walk here, *no progress is the only end signal*, had nothing to fire on.
The remaining half was not in the C arm, and both candidate explanations recorded there are
wrong: the `ret < 0` test IS on the path, and nothing overwrote the answer. One boot at
loglevel=7 says what crosses the boundary instead:
cubelinux: the store is not readable; refusing to answer (x138 in 40 s)
walk: spaces returned 0, len=0, cursor=1 (the client, told success)
The guard fires and the client is told success, so the value is not negative. On that path the
driver's only return is `-(e.to_errno() as i32)`, and `kernel::error::Error` IS the kernel error
code: `from_errno(-2) == ENOENT`, `to_errno()` is documented as "the kernel error code", and the
API's own conversion of a `Result` to a C result — `kernel::from_result` — writes
`T::from(e.to_errno() as i16)` with no negation, as every other driver in this tree does. The
second minus sign made every refusal in this file a *positive* number, and a positive return is
what the C arm's `ret < 0`, the client's own wrapper, and the walk arms' `ret == 0` all read as
success.
38 sites in cubelinux_store.rs were shaped `-(e.to_errno() as i32)` / `as isize`. All 38 now
return `e.to_errno()`, which is what makes them refusals. Nothing else about them changed.
Why this became a *loop* in the space walk and nowhere else: CUBE_OP_SPACES is the one arm that
advances the cursor itself. Every other walk arm leaves the cursor where the caller put it, so a
bogus return there ends the walk on the client's no-progress rule — which is why one defect was
invisible at every other verb for as long as it existed. That arm now refuses a return that is
neither 0 ("here is a space") nor negative (a refusal), so a defect of this shape cannot be read
as a space again.
One more correction in the same class, found while proving the above. A store that cannot be
*opened* answered -ENOENT, and -ENOENT is this interface's own end-of-walk signal — "no such
space; the walk is finished" — so a walk over a store on a disk whose driver had not loaded read
exactly like a walk over an empty store, which is what the first benchmark boot was.
`store_file()` now answers ENODEV when the store device is not there. A store that is not there
is not an empty store.
Proven against the reproduction, in the guest, on the bench initramfs:
cube_store=/dev/null -> "walk: spaces failed: Invalid argument", 0 records, 5 s, boot finishes
cube_store=/nowhere/x.img -> "walk: spaces failed: No such device", 0 records, 5 s, boot finishes
before: 364,994 invented record lines in the 90 s the instrument allowed
The three checks are `kernel/verify-no-store.sh`, a gate on the wall now, and the same three
inside the benchmark rehearsal — which is where this defect was found, and where they were
warnings while it was open.
The diagnostic prints this was hunted with come off in the same commit: the `cube_store=
resolved to` line in cube_syscall.c, and the per-call "not readable" warning in both walk ops.
The refusal is the return value, and the walk's own transcript is where a reader learns what
happened; a message per call is a diagnostic, not the interface. The guard itself stays, and
where to find it is written down at its definition rather than implied.
Built as #95, which is what the box now has installed: the machine boots it on its next reboot.
This commit is contained in:
@@ -95,7 +95,6 @@ static char store_device_path[256] = "/dev/vda";
|
||||
static int __init cube_store_setup(char *str)
|
||||
{
|
||||
strscpy(store_device_path, str, sizeof(store_device_path));
|
||||
pr_info("cubelinux: cube_store= resolved to %s\n", store_device_path);
|
||||
return 1;
|
||||
}
|
||||
__setup("cube_store=", cube_store_setup);
|
||||
@@ -265,8 +264,24 @@ static long cube_enum_op(unsigned int op, void __user *uargs)
|
||||
__u8 found[32];
|
||||
|
||||
ret = cubelinux_kernel_spaces(e.cursor, found);
|
||||
/*
|
||||
* The driver answers with one space written and 0, or with a negative errno. A
|
||||
* positive value is neither, and it must not be read as either: the buffer was not
|
||||
* written, so a caller handed this answer copies a space nobody found.
|
||||
*
|
||||
* This arm is the only one that advances the cursor by itself. Every other walk
|
||||
* leaves the cursor where the caller put it, so a bogus return there ends the walk
|
||||
* on the client's no-progress rule. Here it would hand the walk a cursor that keeps
|
||||
* moving over a space that is not there — which is not a hypothetical: a kernel error
|
||||
* code that had lost its sign did exactly that, and the walk served ~200,000 invented
|
||||
* records a minute until the machine stopped making progress. The cause is fixed
|
||||
* where it was (the driver's errno conversions, cubelinux_store.rs); this is the
|
||||
* bound that keeps a defect of that shape from becoming a loop again.
|
||||
*/
|
||||
if (ret < 0)
|
||||
return ret;
|
||||
if (ret != 0)
|
||||
return -EINVAL;
|
||||
memcpy(e.space, found, sizeof(found));
|
||||
/* An index here, not a count of records: hand back the one after this space. */
|
||||
e.cursor = e.cursor + 1;
|
||||
|
||||
Reference in New Issue
Block a user