Add kernelspace and userspace parts of ktrargs()

[artemhevorhian_gmail.com]

Summary

The problem. Currently the usage of ktrace is limited in the way that it is not possible to see the environment variables and command line arguments of execve() system call.

Solution. A new ktrace event is proposed for writing the information that is currently hidden behind the pointers of SYSCALL execve() with code 59. For that, kern_execve() is injected with a call to ktrace framework.

[bz]

You should retitle the review and replace "the new function" with what function it is so people know.

[artemhevorhian_gmail.com]

Remove the useless includes + other minor changes.

[markj]

sys/kern/kern_ktrace.c

565

IMO this name is too vague. ktrexecveargs() or similar would be better.

569

You're allocating a buffer here and then another for the env, then copying those buffers into a third buffer. But, the first two buffers are leaked, and there's no reason to allocate them that I can see - why not just copy everything into the final destination buffer?

sys/sys/ktrace.h

266

Same comment regarding the name - "ARGS" is too vague.

usr.bin/kdump/kdump.c

264

Please group this with the other ktr*() functions further down below main().

266

Please format this more like other ktrace records. Using english to describe the fields is not very useful, especially without quotes.

At the very least, something like printf("{argv='%s', envp='%s'}\n"). Though, perhaps we want to do something to show whether individual args/envvars contain whitespace?

705–711

Missing an entry here for the new record type.

[artemhevorhian_gmail.com]

Because of the need to include the whole imgact.h, remove the forward declaration
of it.

[artemhevorhian_gmail.com]

First working version. To be reviewed.

[artemhevorhian_gmail.com]

sys/kern/kern_ktrace.c

569

Hello Mark. Please check the new version. Do I still need to run a free() call at the end of the function, right after ktr_submitrequest()?

[artemhevorhian_gmail.com]

usr.bin/kdump/kdump.c

266

Is it possible that argvs and/or envv contain whitespace characters?

[markj]

sys/kern/kern_ktrace.c

570

Please put a newline between variable declarations and the rest of the function. See the style.9 man page for more guidance: https://man.freebsd.org/cgi/man.cgi?style(9)

572

This can simply be buf[argc] = '\0';, no need to use bcopy().

574

Similarly, buf[argc + 1 + envc] = '\0'; is a bit simpler.

577

It's impossible to have buf == NULL here, this check can be removed.

585

Extra newline here.

sys/sys/ktrace.h

43

You don't need these includes here. Just declare struct image_args in the same place where we already declare struct ktr_io_params.

350

There should be a space before *. See the style.9 man page I linked above.

usr.bin/kdump/kdump.c

266

Yes. Try tracing a shell, and run something like

$ awk '{ print }' /dev/null

The argv reported by ktrace will look like awk { print } /dev/null, which is a bit confusing.

usr.bin/ktrace/ktrace.h

35

The ktrace man page needs to be updated as well. We should add a new flag to -t to optionally disable execve tracing, and the documented list of default tracepoints also needs to be updated.

[artemhevorhian_gmail.com]

sys/sys/ktrace.h

43

Because of parsing in ktrace.c with the help of exec_args_get_begin_envv and similar, I need the complete definition.

[artemhevorhian_gmail.com]

Applied changes from code review.

[artemhevorhian_gmail.com]

Use tabs instead of whitespace characters where necessary.

[artemhevorhian_gmail.com]

Remove code printing the EXEC envv parts of the image_args.
For that, a separate call is to be introducted later,
ktrexecveenvs().

[artemhevorhian_gmail.com]

Use "" quotation marks to show whitespace characters.

[markj]

sys/sys/ktrace.h

43

Then imgact.h should be included in kern_ktrace.c, not here. ktrace.h is included by many different C files, and it should avoid bringing in more definitions than necessary.

[artemhevorhian_gmail.com]

Prepare for review.

[markj]

This is getting close, most of my comments are style issues.

It would be nice to see some sample output.

sys/kern/kern_ktrace.c

569

575

If req == NULL here, then we need to free buf.

usr.bin/kdump/kdump.c

120

520

1660

Most of these other functions should use const as well, e.g., in ktrnamei(), but that should be handled in a separate patch.

[markj]

usr.bin/kdump/kdump.c

1665

I just realized that you have no way of distinguishing arguments and environment variables here. I think the structure returned from the kernel should distinguish the two. For example, you could store the offset of the environment variables at the beginning of the ktrace record.

[artemhevorhian_gmail.com]

This needs to be reviewed, especially the kernel part please. If that's okay,
we can move to the userland implementation. I am not sure if I should put the
buffer in the sturcutre right now, or that it is even possible. Please help me
with the data design step at this point. We all understand what needs to be
passed to userspace, let's decide on how that should be done from the kernel
space now.

[artemhevorhian_gmail.com]

869 ktrace   EXEC  ARGV: "echo", "foo "ENVV: "LOGNAME=root", "PAGER=less", "LANG=C.UTF-8", "MAIL=/var/mail/root", "PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin:/root/bin", "XDG_RUNTIME_DIR=/var/run/xdg/root", "ENV=/root/.shrc", "PWD=/root", "TERM=vt100", "HOME=/root", "USER=root", "SHELL=/bin/sh", "MM_CHARSET=UTF-8", "BLOCKSIZE=K"

Here is a sample output. Let me fix the whitespace character between the two and we're done, I suppose.

[artemhevorhian_gmail.com]

This version includes working with a struc and buffer altogether.
In the struct, there is the offset required to tell the difference
between ARGV and ENVV arrays.

[artemhevorhian_gmail.com]

Fix formatting.

[glebius]

What do you think of idea to separate arguments vector printing and environment vector printing? I would imagine that a typical use case is interest in the arguments, ignoring environment. Two different KTR reports would allows either to filter at the tracing time, or if all tracing was enabled, then basic grep(1) would help.

[artemhevorhian_gmail.com]

Use two separate events for arguments and environment variables.

[glebius]

sys/kern/kern_ktrace.c

582–598

And then you can just call:

ktrdata(KTR_EXECVE_ARGS, args->begin_argv, exec_args_get_begin_envv(args) - args->begin_argv);
ktrdata(KTR_EXECVE_ENVS, exec_args_get_begin_envv(args), args->endp - exec_args_get_begin_envv(args));

straight from the kern_exec.c, without need to import <imgact.h> into ktr_ktrace.c.

New function ktrdata() can also be used to reduce ktrnamei() down to:

ktrdata(KTR_NAMEI, path, strlen(path))

[glebius]

sys/kern/kern_ktrace.c

582–598

Here is the version I suggest, might be easier to read than a colored diff:

void
ktrdata(int type, const void *data, size_t len)
{
        struct ktr_request *req;
        void *buf;

        if ((req = ktr_getrequest(type)) == NULL)
                return;
        buf = malloc(len, M_KTRACE, M_WAITOK);
        bcopy(data, buf, len);
        req->ktr_header.ktr_len = len;
        req->ktr_buffer = buf;
        ktr_submitrequest(curthread, req);
}

[artemhevorhian_gmail.com]

Address comments by glebius@.

[artemhevorhian_gmail.com]

Remove unnecessary forward declaration.

[glebius]

usr.bin/ktrace/ktrace.1

28

Every meaningful change to a manual page must bump this date.

[glebius]

sys/sys/ktrace.h

266

These two new declarations need a comment, just like all old ones have. See comment above KTR_NAMEI as an example.

[artemhevorhian_gmail.com]

Update the commit message and make amendments according to the style

guide.