On Sat, Oct 09, 2021 at 03:52:02PM +0300, Alexey Dobriyan wrote:

On Fri, Oct 08, 2021 at 04:55:04PM -0700, Kees Cook wrote:

...

This makes sure that wchan contains a sensible symbol when a process is blocked.

...

Specifically this calls the sleep() syscall, and expects the architecture to have called schedule() from a function that has "sleep" somewhere in its name.

This exposes internal kernel symbol to userspace.

Correct; we're verifying the results of the wchan output, which produces a kernel symbol for blocked processes.

Why would want to test that?

This is part of a larger series refactoring/fixing wchan[1], and we've now tripped over several different failure conditions, so I want to make sure this doesn't regress in the future.

Doing s/sleep/SLEEP/g doesn't change kernel but now the test is broken.

Yes; the test would be doing it's job, as that would mean there was a userspace visible change to wchan, so we'd want to catch it and either fix the kernel or update the test to reflect the new reality.

...

For example, on the architectures I tested (x86_64, arm64, arm, mips, and powerpc) this is "hrtimer_nanosleep":

...

+/*

• • Make sure that wchan returns a reasonable symbol when blocked.
• */

Test should be "contains C identifier" then?

Nope, this was intentional. Expanding to a C identifier won't catch the "we unwound the stack to the wrong depth and now all wchan shows is '__switch_to'" bug[2]. We're specifically checking that wchan is doing at least the right thing for the most common blocking state.

...

+int main(void) +{

• char buf[64];
• pid_t child;
• int sync[2], fd;
• if (pipe(sync) < 0)

• perror_exit("pipe");
  

• child = fork();
• if (child < 0)

• perror_exit("fork");
  

• if (child == 0) {

• /* Child */
  

• if (close(sync[0]) < 0)
  

• 	perror_exit("child close sync[0]");
  

• if (close(sync[1]) < 0)
  

• 	perror_exit("child close sync[1]");
  

Redundant close().

Hmm, did you maybe miss the differing array indexes? This closes the reading end followed by the writing end of the child's pipe.

...

• sleep(10);
  

• _exit(0);
  

• }
• /* Parent */
• if (close(sync[1]) < 0)

• perror_exit("parent close sync[1]");
  

Redundant close().

It's not, though. This closes the write side of the parent's pipe.

...

• if (read(sync[0], buf, 1) != 0)

• perror_exit("parent read sync[0]");
  

Racy if child is scheduled out after first close in the child.

No, the first close will close the child's read-side of the pipe, which isn't being used. For example, see[3].

The parent's read of /proc/$child/wchan could technically race if the child is scheduled out after the second close() and before the sleep(), but the parent is doing at least 2 syscalls before then. I'm open to a more exact synchronization method, but this should be sufficient. (e.g. Using ptrace to catch sleep syscall entry seemed like overkill.)

-Kees

[1] https://lore.kernel.org/lkml/20211008111527.438276127@infradead.org/ [2] https://lore.kernel.org/lkml/20211008124052.GA976@C02TD0UTHF1T.local/ [3] https://man7.org/tlpi/code/online/diff/pipes/pipe_sync.c.html

...

• snprintf(buf, sizeof(buf), "/proc/%d/wchan", child);
• fd = open(buf, O_RDONLY);
• if (fd < 0) {

• if (errno == ENOENT)
  

• 	return 4;
  

• perror_exit(buf);
  

• }
• memset(buf, 0, sizeof(buf));
• if (read(fd, buf, sizeof(buf) - 1) < 1)

• perror_exit(buf);
  

• if (strstr(buf, "sleep") == NULL) {

• fprintf(stderr, "FAIL: did not find 'sleep' in wchan '%s'\n", buf);
  

• return 1;
  

• }
• printf("ok: found 'sleep' in wchan '%s'\n", buf);
• if (kill(child, SIGKILL) < 0)

• perror_exit("kill");
  

• if (waitpid(child, NULL, 0) != child) {

• fprintf(stderr, "waitpid: got the wrong child!?\n");
  

• return 1;
  

• }
• return 0;

+}

On Thu, Oct 21, 2021 at 10:09:33AM -0700, Kees Cook wrote:

...

Hi Peter,

Can you add this to the wchan series, please? This should help wchan from regressing in the future, and allow us to notice if the depth accidentally changes, like Mark saw.

---

I'd like to make sure we have a regression test for this. Will you add this to the wchan series please?

I have it there, but it's in the part that didn't make it in yet. I'm currently in the progress of reworking that.

Do you want it it ahead of all that?

On Thu, Oct 21, 2021, at 1:12 PM, Kees Cook wrote:

On Thu, Oct 21, 2021 at 01:03:33PM -0700, Andy Lutomirski wrote:

...

On Fri, Oct 8, 2021, at 4:55 PM, Kees Cook wrote:

...

This makes sure that wchan contains a sensible symbol when a process is blocked. Specifically this calls the sleep() syscall, and expects the architecture to have called schedule() from a function that has "sleep" somewhere in its name. For example, on the architectures I tested (x86_64, arm64, arm, mips, and powerpc) this is "hrtimer_nanosleep":

Is this really better than admitting that the whole mechanism is nonsense and disabling it?

We could have a fixed string for each task state and call it a day.

I consider this to be "future work". In earlier discussions it came up, but there wasn't an obvious clean cost-free way to do this, so instead we're just fixing the broken corner and keeping the mostly working rest of it while cleaning up the weird edges. :)

True, but we have the caveat that wchan is currently broken, so in some sense we have an easier time killing it now as compared to later. But if we don't have a fully-fleshed-out idea for how to kill it, then I'm fine with waiting.

-- Kees Cook