PVF: ensure job processes are cleaned up, add tests

[mrcnski]

Fixes a potential memory leak.

PR_SET_PDEATHSIG is used to terminate children when the parent dies. Note that this is subject to a race. There seems to be a raceless alternative here, but the concern is small enough that a bit more complexity doesn't seem worth it. Left a bit more info in the code comment.

[alindima]

LGTM! Children should already die if the parent died because they'll receive SIGPIPE when writing to the pipe due to the parent closing their end of the pipe.

But your changes make sense, in order to not wait for the child to write to the pipe

[mrcnski]

Thanks @alindima! Great point. I mainly made the changes so that the new tests would pass. I couldn't think of a realistic scenario where they would help, but thought it wouldn't hurt to have them. For example, in low-memory conditions I'm pretty sure the OOM killer would go after the child processes and not the parent worker.

But I realize now that hijacked jobs could kill the parent worker over and over, and I think they can also unset the pdeathsig. I mean, I think a child can kill its parent? I assumed so because we do it in kill_parent_node_in_emergency (called by the worker on version-mismatch with the node), and I think we manually tested that fn, but we should add another integration test to verify that it works.

Anyway, if the answer is yes, then we should find a way to prevent the child job killing its parent worker. But I believe malicious jobs can also exhaust system resources in other ways, like fork bombs.

[mrcnski]

I confirmed with a manual test that job process are able to kill their parent workers. Edit: my VPS account is root so maybe that's why? (I couldn't find a simple way to write an integration test for this though.) I also confirmed that jobs are able to unset their pdeathsig.

However, I believe once we have this, and children job processes live in separate pid namespaces, they will no longer be able to kill the parent worker.

The attack remains this: jobs can unset their pdeathsig and never terminate. Eventually the host will terminate its direct child, the worker, but the job will be left as a zombie process, because it unset pdeathsig. It can allocate memory up to the maximum allowed for the process (which as of the above PR will be 256 * 1024 * 1024, which is a lot. And currently there is no limit), which can eventually lead to OOM if an attacker performs this repeatedly.

When thinking about mitigations, blocking prctl with seccomp obviously comes to mind, but that comes with all the issues we've hit with seccomp. Perhaps in addition to the pdeathsig stuff, the host can take care of cleaning up children. To avoid a dependency on procfs (which is not portable and may not be accessible on some systems or in containers) the worker can send the pid of the job over to the host over the socket, before it starts the job.

But, that is not as simple as adding a prtcl call like this PR does. The question remains whether that change (adding the extra communication between host and worker) is worth doing, because as mentioned, jobs can already exhaust resources in other ways. Maybe it's best to just live with this until PolkaVM comes and solves all our problems.

[mrcnski]

Just ran into this: noticed I had a few zombie processes on my VPS running near 100% CPU. I guess they were running the halt PVF, left over from some previous test run gone awry. I'm scared to look at my usage bill. 😰

[s0me0ne-unkn0wn]

I think a child can kill its parent?

Yes, if permissions allow. That may be tricky. Normally, it's always possible for a child to kill its parent unless different setuid()s are involved. But in a sophisticated environment (I mean namespace unsharing, cgroup settings etc.), more factors may be in place. The best way to know is to test, as always.

[s0me0ne-unkn0wn]

Looks good 👍

[mrcnski]

Thanks for the review! Yeah, with the clone changes the child will have its own PID namespace. It should be impossible for the child to kill its parent. But, I was curious whether our kill_parent_node_in_emergency function even works. I remember testing it, but that was probably also on a root environment--the internet says this shouldn't normally work.

[s0me0ne-unkn0wn]

But, I was curious whether our kill_parent_node_in_emergency function even works. I remember testing it, but that was probably also on a root environment--the internet says this shouldn't normally work.

I definitely tested it as non-root, but that was well before the unsharing stuff was introduced... I should re-test it and get rid of it if it's useless now.