Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't panic the kernel upon a device error #4414

Merged
merged 3 commits into from
Jan 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
347 changes: 190 additions & 157 deletions packages/SwingSet/docs/devices.md

Large diffs are not rendered by default.

40 changes: 19 additions & 21 deletions packages/SwingSet/src/kernel/deviceManager.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// @ts-check
import { assert } from '@agoric/assert';
import { makeDeviceSlots } from './deviceSlots.js';
import { insistCapData } from '../capdata.js';
Expand Down Expand Up @@ -55,33 +56,30 @@ export default function makeDeviceManager(
deviceParameters,
);

/**
* @typedef {['ok', CapData]} VatInvocationResults
*/

/**
* @typedef {[string, string, CapData]} DeviceInvocation
* @property {string} 0 Kernel slot designating the device node that is the target of
* the invocation
* @property {string} 1 A string naming the method to be invoked
* @property {CapData} 2 A capdata object containing the arguments to the invocation
*/

/**
* Invoke a method on a device node.
*
* @param {DeviceInvocation} deviceInvocation
* @returns {VatInvocationResults} a VatInvocationResults object: ['ok', capdata]
* @throws {Error} an exeption if the invocation failed. This exception is fatal to
* the kernel.
* @param { DeviceInvocation } deviceInvocation
* @returns { DeviceInvocationResult }
*/
function invoke(deviceInvocation) {
const [target, method, args] = deviceInvocation;
const deviceResults = dispatch.invoke(target, method, args);
assert(deviceResults.length === 2);
assert(deviceResults[0] === 'ok');
insistCapData(deviceResults[1]);
return deviceResults;
try {
/** @type { DeviceInvocationResult } */
const deviceResults = dispatch.invoke(target, method, args);
// common error: raw devices returning capdata instead of ['ok', capdata]
assert.equal(deviceResults.length, 2, deviceResults);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this assert. What is the meaning of the 3rd argument?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the message emitted if the assertion fails (or at least I think that's what it's for), so the developer at the console can figure out why it threw without going back and inserting a console.log. optDetails seems to be the official name. The default for optDetails reports the two values being compared, in this case "2" and whatever .length is. In the failure mode I ran into, where my raw device returned { body: .., slots: ..} instead of ['ok', { body, slots }], the assertion printed undefined !== 2, which didn't help me very much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought the 3rd arg was the message string, which is why I am puzzled because deviceResults is not a string.

if (deviceResults[0] === 'ok') {
insistCapData(deviceResults[1]);
} else {
assert.equal(deviceResults[0], 'error');
assert.typeof(deviceResults[1], 'string');
}
return deviceResults;
} catch (e) {
console.log(`dm.invoke failed, informing calling vat`, e);
return harden(['error', 'device.invoke failed, see logs for details']);
}
}

const manager = {
Expand Down
14 changes: 12 additions & 2 deletions packages/SwingSet/src/kernel/deviceSlots.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// @ts-check
import { Remotable, passStyleOf, makeMarshal } from '@agoric/marshal';
import { assert, details as X } from '@agoric/assert';
import { insistVatType, makeVatSlot, parseVatSlot } from '../parseVatSlots.js';
Expand Down Expand Up @@ -181,6 +182,13 @@ export function makeDeviceSlots(

// this function throws an exception if anything goes wrong, or if the
// device node itself throws an exception during invocation
/**
*
* @param { string } deviceID
* @param { string } method
* @param { SwingSetCapData } args
* @returns { DeviceInvocationResult }
*/
function invoke(deviceID, method, args) {
insistVatType('device', deviceID);
insistCapData(args);
Expand All @@ -206,9 +214,11 @@ export function makeDeviceSlots(
);
}
const res = fn.apply(t, m.unserialize(args));
const vres = harden(['ok', m.serialize(res)]);
const vres = m.serialize(res);
/** @type { DeviceInvocationResultOk } */
const ires = harden(['ok', vres]);
lsdebug(` results ${vres.body} ${JSON.stringify(vres.slots)}`);
return vres;
return ires;
}

return harden({ invoke });
Expand Down
111 changes: 102 additions & 9 deletions packages/SwingSet/src/kernel/deviceTranslator.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { insistKernelType } from './parseKernelSlots.js';
import { insistVatType, parseVatSlot } from '../parseVatSlots.js';
import { insistCapData } from '../capdata.js';
import { kdebug } from './kdebug.js';
import { insistValidVatstoreKey } from './vatTranslator.js';

/*
* Return a function that converts KernelInvocation objects into
Expand Down Expand Up @@ -37,23 +38,38 @@ function makeDRTranslator(deviceID, kernelKeeper) {
const deviceKeeper = kernelKeeper.allocateDeviceKeeperIfNeeded(deviceID);
const { mapDeviceSlotToKernelSlot } = deviceKeeper;

/**
*
* @param { DeviceInvocationResult } deviceInvocationResult
* @returns { KernelSyscallResult }
*/
function deviceResultToKernelResult(deviceInvocationResult) {
// deviceInvocationResult is ['ok', capdata]
const [successFlag, devData] = deviceInvocationResult;
assert(successFlag === 'ok');
insistCapData(devData);
const kData = {
...devData,
slots: devData.slots.map(slot => mapDeviceSlotToKernelSlot(slot)),
};
return harden(['ok', kData]);
if (successFlag === 'ok') {
insistCapData(devData);
const kData = {
...devData,
slots: devData.slots.map(slot => mapDeviceSlotToKernelSlot(slot)),
};
return harden(['ok', kData]);
} else {
assert.equal(successFlag, 'error');
assert.typeof(devData, 'string');
return harden([successFlag, devData]);
}
}
return deviceResultToKernelResult;
}

/*
/**
* return a function that converts DeviceSyscall objects into KernelSyscall
* objects
*
* @param { string } deviceID
* @param { string } deviceName
* @param { * } kernelKeeper
* @returns { (dsc: DeviceSyscallObject) => KernelSyscallObject }
*/
export function makeDSTranslator(deviceID, deviceName, kernelKeeper) {
const deviceKeeper = kernelKeeper.allocateDeviceKeeperIfNeeded(deviceID);
Expand Down Expand Up @@ -81,14 +97,67 @@ export function makeDSTranslator(deviceID, deviceName, kernelKeeper) {
return ks;
}

function translateVatstoreGet(key) {
insistValidVatstoreKey(key);
kdebug(`syscall[${deviceID}].vatstoreGet(${key})`);
return harden(['vatstoreGet', deviceID, key]);
}

function translateVatstoreSet(key, value) {
insistValidVatstoreKey(key);
assert.typeof(value, 'string');
kdebug(`syscall[${deviceID}].vatstoreSet(${key},${value})`);
return harden(['vatstoreSet', deviceID, key, value]);
}

function translateVatstoreGetAfter(priorKey, lowerBound, upperBound) {
if (priorKey !== '') {
insistValidVatstoreKey(priorKey);
}
insistValidVatstoreKey(lowerBound);
if (upperBound) {
insistValidVatstoreKey(upperBound);
}
kdebug(
`syscall[${deviceID}].vatstoreGetAfter(${priorKey}, ${lowerBound}, ${upperBound})`,
);
return harden([
'vatstoreGetAfter',
deviceID,
priorKey,
lowerBound,
upperBound,
]);
}

function translateVatstoreDelete(key) {
insistValidVatstoreKey(key);
kdebug(`syscall[${deviceID}].vatstoreDelete(${key})`);
return harden(['vatstoreDelete', deviceID, key]);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there's quite a bit of code duplication with vatTranslator.js. Should this be factored into some common code to ensure that changes to the syscall interface don't diverge under maintenance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe. For now, devices don't get the same syscalls as vats: they don't do promises, so syscall.send isn't quite right, they can't syscall.exit, they don't participate in GC so no syscall.dropImports.

But in the long run, yeah, we want devices to be more like vats, and at that point it'll make sense to drop deviceTranslator.js and use vatTranslator in its place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more along the lines of a syscall translator that's parameterized according to whether it thinks it's doing the job for a vat or a device, which would allow differences between the two to be accommodated but would allow the 95% that's the same to be a single piece of code to maintain.


// vsc is [type, ...args]
// ksc is:
// ['send', ktarget, kmsg]
/**
* Convert syscalls from device space to kernel space
*
* @param { DeviceSyscallObject } vsc
* @returns { KernelSyscallObject }
*/
function deviceSyscallToKernelSyscall(vsc) {
const [type, ...args] = vsc;
switch (type) {
case 'sendOnly':
return translateSendOnly(...args); // becomes ['send' .. result=null]
case 'vatstoreGet':
return translateVatstoreGet(...args);
case 'vatstoreSet':
return translateVatstoreSet(...args);
case 'vatstoreGetAfter':
return translateVatstoreGetAfter(...args);
case 'vatstoreDelete':
return translateVatstoreDelete(...args);
default:
assert.fail(X`unknown deviceSyscall type ${type}`);
}
Expand All @@ -97,6 +166,30 @@ export function makeDSTranslator(deviceID, deviceName, kernelKeeper) {
return deviceSyscallToKernelSyscall;
}

function kernelResultToDeviceResult(type, kres) {
const [successFlag, resultData] = kres;
assert(successFlag === 'ok', 'unexpected KSR error');
switch (type) {
case 'vatstoreGet':
if (resultData) {
assert.typeof(resultData, 'string');
return harden(['ok', resultData]);
} else {
return harden(['ok', undefined]);
}
case 'vatstoreGetAfter':
if (resultData) {
assert(Array.isArray(resultData));
return harden(['ok', resultData]);
} else {
return harden(['ok', undefined]);
}
default:
assert(resultData === null);
return harden(['ok', null]);
}
}

export function makeDeviceTranslators(deviceID, deviceName, kernelKeeper) {
return harden({
kernelInvocationToDeviceInvocation: makeKDTranslator(
Expand All @@ -109,6 +202,6 @@ export function makeDeviceTranslators(deviceID, deviceName, kernelKeeper) {
deviceName,
kernelKeeper,
),
// no syscall results translator: devices cannot do syscall.callNow()
kernelResultToDeviceResult,
});
}
40 changes: 27 additions & 13 deletions packages/SwingSet/src/kernel/kernel.js
Original file line number Diff line number Diff line change
Expand Up @@ -873,6 +873,11 @@ export default function buildKernel(
// * error: you are dead ['error, description]
// the VatManager+VatWorker will see the error case, but liveslots will
// not
/**
*
* @param { VatSyscallObject } vatSyscallObject
* @returns { VatSyscallResult }
*/
function vatSyscallHandler(vatSyscallObject) {
// eslint-disable-next-line no-use-before-define
if (!vatWarehouse.lookup(vatID)) {
Expand All @@ -883,13 +888,16 @@ export default function buildKernel(
setTerminationTrigger(vatID, true, true, makeError(problem));
return harden(['error', problem]);
}
/** @type { KernelSyscallObject | undefined } */
let ksc;
let kres;
let vres;
/** @type { KernelSyscallResult } */
let kres = harden(['error', 'incomplete']);
/** @type { VatSyscallResult } */
let vres = harden(['error', 'incomplete']);

try {
// This can fail if the vat asks for something not on their clist, or
// their syscall doesn't make sense in some other way, or due to a
// This can throw if the vat asks for something not on their clist,
// or their syscall doesn't make sense in some other way, or due to a
// kernel bug, all of which are fatal to the vat.
ksc = translators.vatSyscallToKernelSyscall(vatSyscallObject);
} catch (vaterr) {
Expand All @@ -898,6 +906,7 @@ export default function buildKernel(
console.log(`error during syscall translation:`, vaterr);
const problem = 'syscall translation error: prepare to die';
setTerminationTrigger(vatID, true, true, makeError(problem));
kres = harden(['error', problem]);
vres = harden(['error', problem]);
// we leave this catch() with ksc=undefined, so no doKernelSyscall()
}
Expand All @@ -907,21 +916,26 @@ export default function buildKernel(

if (ksc) {
try {
// this can fail if kernel or device code is buggy
// this can throw if kernel is buggy
kres = kernelSyscallHandler.doKernelSyscall(ksc);
// kres is a KernelResult ([successFlag, value]), but since errors
// here are signalled with exceptions, kres is ['ok', value]. Vats
// (liveslots) record the response in the transcript (which is why we
// use 'null' instead of 'undefined', TODO clean this up), but otherwise
// most syscalls ignore it. The one syscall that pays attention is
// callNow(), which assumes it's capdata.

// kres is a KernelResult: ['ok', value] or ['error', problem],
// where 'error' means we want the calling vat's syscall() to
// throw. Vats (liveslots) record the response in the transcript
// (which is why we use 'null' instead of 'undefined', TODO clean
// this up #4390), but otherwise most syscalls ignore it. The one
// syscall that pays attention is callNow(), which assumes it's
// capdata.

// this can throw if the translator is buggy
vres = translators.kernelSyscallResultToVatSyscallResult(
ksc[0],
kres,
);
// here, vres is either ['ok', null] or ['ok', capdata]

// here, vres is ['ok', null] or ['ok', capdata] or ['error', problem]
} catch (err) {
// kernel/device errors cause a kernel panic
// kernel/translator errors cause a kernel panic
panic(`error during syscall/device.invoke: ${err}`, err);
// the kernel is now in a shutdown state, but it may take a while to
// grind to a halt
Expand Down
Loading