These semantics are copied from coredump() in kern_sig.c.

We don't initialize stoppcbs[] anywhere, which makes sense since nothing is being stopped, but doesn't kgdb expect to find them?

I think it would be better to use an sx lock to provide mutual exclusion.

I don't think it's good to enable this in capability mode: the sysctl allows one to overwrite arbitrary files. If it's done this way to make savecore work, I'd prefer to simply not enter capability mode when using this interface; in normal operation savecore reads metadata off of the dump device during boot and thus should be sandboxed.

Why does this need to be invoked as a callback? Can't the sysctl handler set up all of the required context before calling minidumpsys()?

I think these checks should live in a shared subroutine.

IO_NODELOCKED means that the caller holds a range lock for the region, but here no range locks are held, so nothing blocks concurrent I/O to the file. See coredump().

Can factor this cleanup into a standalone function?

why are you providing a path instead of opening the intended file in savecore and passing a fd? then you would provide a referenced vnode in di.priv.

AFAIR stoppcbs contains per-cpu registers state. It does not seems to be too useful to dump something there, because the snapshot is not consistent. The context of stoppcb[cpuX] should match the state of the thread executing on cpuX, and pcpu[cpuX] (which contains e.g. curthread pointer).

If all this data is not consistent, I suspect both tools that look into dump, and people trying to analyze the dump, would be very confused.

EFAULT is a strange error code for the conditions there.

Mainly it was a result of prototyping, but passing a fd is obviously much more sane. Thanks!

Yeah, this was for the benefit of savecore. This should be safer now that we are passing an opened file descriptor, yes?

Note that we still require a callback for a pseudo-dumper like this, otherwise dump_start() will try to compute the starting offset based on mediasize and blocksize, which only really makes sense for a disk device.

Thanks, I think I've got a better grasp on the different file/vnode locks now, please have another look.

There is also the 'advisory lock' which I see manipulated in other places, but I haven't touched it here. If I understand it right, this is mainly for the benefit of userspace programs using F_GETKL/F_SETLK, and related interfaces. If so, I don't think there's any reason to mess with it in this case.

Why cap_fcntl_rights? Don't we (at least) want CAP_WRITE? And it must be verified that fd was opened for writing, i.e., FWRITE is set. I can't see how that happens here.

Order is reversed, range locks come before vnode locks.

I do not have explicit arguments, but I think that by conventions and common sense, this lock should be taken before VFS locks.

Flags should be IO_RANGELOCKED | IO_NODELOCKED, since you took both. You should have got the deadlock right away, no?

It did not deadlock. It appears that IO_RANGELOCKED only has an effect when IO_NODELOCKED is not set. I did not see any other callers that specify both flags, but I can do this here if it is intended to be done this way.

It is relatively weird to pass fd to sysctl handler, as well.

I suspect that the right way to do it is through a syscall. For instance, it might be an ioctl on /dev/mem, which would imply permissions to access both the dumping vnode and /dev/mem.

I do not insist strongly, but sysctl taking fd is rare. I intended to say 'nonexistent', but we have debug.ftry_reclaim_vnode, although it is purely debugging interface.

Why allowing this for a capability mode process?

Exactly what is unsupported? I understand that the condition was not true, but what does the condition mean? Supplied key length is wrong, or what?

again, so what? 'error writing livedump' or similar.

Why not uprintf()?

Indeed, I do not love doing this with a sysctl, I know it is ugly. I considered the alternatives of a new syscall or creating a new livedump cdev, but both seem to be a little overkill for what is a niche debugging feature. Perhaps this is less of a problem than I think. Adding an ioctl to /dev/mem might be a happy medium, I will think on this.

savecore(8) runs in capability mode. Invoking the sysctl could be done via libcasper, but then it is run by a separate process and we lose the meaning of the passed fd.

I suppose it does open a small but undesirable attack surface.

I like the idea of using an ioctl on /dev/(k)mem. That's the interface used by kgdb to inspect a live system, for instance. It also sidesteps the Capsicum issue.

"invoke" doesn't seem like the right verb unless it's MEM_INVOKE_LIVEDUMP. Maybe just MEM_MINIDUMP or MEM_KERNELDUMP?

getvnode() bumps the refcount of the returned file. The reference needs to be explicitly released with fdrop().

A minidump is really a specific format for a kernel core dump. I'd suggest making the terminology more general and just note that the output kernel core dump is in the format of a minidump. (Someday we might want to support live full memory dumps as well.)

I'd maybe be a bit more specific, along the lines of "minidumps taken from a running system may have inconsistent kernel data structures and thus may not be usable, especially if the system was under load. Despite this, live minidumps can be useful for offline debugging of certain types of kernel bugs, such as deadlocks."

In other words, I think we should motivate the existence of this feature a bit.

We should have this function explicitly check for kmem read privileges:

error = priv_check(curthread, PRIV_KMEM_READ);
if (error)
    bail

memopen() already does this, but someone might someday add a new caller of livedump_start() which omits the check.

We need to call dumper_destroy() here I think.

This could probably be a KASSERT no?

I'd suggest pre-allocating an int fields field. For instance, MEM_MINIDUMP fails if another livedump is in progress, but we might want to have a mode that blocks until the ongoing livedump is finished. Or we might want a mode which tries to suspend running threads before collecting the dump, etc.. For now we can simply define no flags, require callers to set flags = 0, and verify that in livedump_start().

Done in D34574.