On 03/15, KAMEZAWA Hiroyuki wrote:
>
> I wonder it's better to have a fork-bomb killer
I am not going do discuss the idea, I never know when it comes to the
new features. Although personally I think this makes sense.
Just a couple of nits about the patch itself. Which I didn't read carefully
yet ;)
> +extern struct pid *fork_bomb_session;
> +static inline bool in_fork_bomb(void)
> +{
> + return task_session(current) == fork_bomb_session;
> +}
Well, at first glance it is easy to write the fork-bomb which does setsid()...
> --- mmotm-temp.orig/kernel/fork.c
> +++ mmotm-temp/kernel/fork.c
> @@ -1417,6 +1417,8 @@ long do_fork(unsigned long clone_flags,
> return -EPERM;
> }
>
> + if (in_fork_bomb())
> + return -ENOMEM;
This is not clear to me. fork_bomb_detection() does do_each_pid_task(SID)
and sends SIGKILL to the hostile processes. After that none of the killed
processes can fork. Assuming the "bomb_task" was detected correctly, why
do we punish the whole session?
Once again, I didn't read the patch. This is the question, not the comment.
> +static bool is_ancestor(struct task_struct *t, struct task_struct *p)
> +{
> + while (t != &init_task) {
> + if (t == p)
> + return true;
> + t = t->real_parent;
> + }
> + return false;
> +}
No, this is not right. In fact, in theory this can crash if /sbin/init is
multithreaded. This needs same_thread_group() istead of "==" or "!=". Or
it should use t->real_parent->group_leader, assuming that both t and p
are the group-leaders (this seems to be true).
IOW. If a main thream M does pthread_create() and creates the thread T,
and T forks the child C after that, then C->real_parent == T, not M.
> +static bool fork_bomb_detection(unsigned long totalpages, struct mem_cgroup *mem,
> + const nodemask_t *nodemask)
> +{
> ...
> +
> + fork_bomb_session = task_session(bomb_task);
Hmm. In theory this needs fork_bomb_session = get_pid(task_session()).
Otherwise, all tasks in this session can be killed or can exit before
forkbomb_timeout runs. In this case this pid's memory can be reused
and in in_fork_bomb() can be false positive.
> + INIT_DELAYED_WORK(&forkbomb_timeout, clear_fork_bomb);
OK, we already checked that fork_bomb_session == NULL. But isn't it
possible that multiple threads call fork_bomb_detection() at the same
time? We have sysrq_handle_moom, and __alloc_pages_may_oom() can lock
different zonelist's. No?
> + * Now, we found a bomb task. kill all children of bomb_task.
Again, this is not clear to me. We could literally kill all children,
why do we scan the session instead?
> + do_each_pid_task(bomb_session, PIDTYPE_SID, p) {
> +
> + start_time = timespec_to_jiffies(&p->start_time);
> + start_time += fork_bomb_thresh;
> +
> + if (!thread_group_leader(p))
> + continue;
This is unneded, it must be thread_group_leader().
Oleg.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>