Let me first say that CLONE_SIGHAND must die, I think ;) and perhaps even sighand_struct... I am wondering if we can add something like
if ((clone_flags & (CLONE_THREAD | CLONE_SIGHAND)) == CLONE_SIGHAND) pr_info("You are crazy, please report this to lkml\n"); into copy_process(). On 08/12, Eric W. Biederman wrote: > > Oleg Nesterov <o...@redhat.com> writes: > > > On 08/11, Eric W. Biederman wrote: > >> if (unshare_flags & (CLONE_THREAD | CLONE_SIGHAND | CLONE_VM)) { > >> - /* FIXME: get_task_mm() increments ->mm_users */ > >> - if (atomic_read(¤t->mm->mm_users) > 1) > >> + if (!thread_group_empty(current)) > >> + return -EINVAL; > >> + } > >> + if (unshare_flags & CLONE_VM) { > >> + if (!current_is_single_threaded()) > >> return -EINVAL; > >> } > > > > OK, but then you can remove "| CLONE_VM" from the previous check... > > As an optimization, but I don't think anything cares enough for the > optimization to be worth the confusion. current_is_single_threaded() checks task->signal->live at the start, so there is no optimization. But I won't argue, this doesn't hurt. > >> /* > >> + * If unsharing a signal handlers, must also unshare the signal queues. > >> + */ > >> + if (unshare_flags & CLONE_SIGHAND) > >> + unshare_flags |= CLONE_THREAD; > > > > This looks unnecessary, check_unshare_flags() checks "THREAD | SIGHAND". > > And to me the comment looks misleading although I won't argue. > > I absolutely can not understand this code if we jump 5 steps ahead > and optimize out the individual dependencies, and try for a flattened > dependency tree instead. I can validate the individual dependencies > from first principles. > > If we jump several steps ahead I can not validate the individual > dependencies. OK, > > And in fact this doesn't look exactly right, or I am totally confused. > > Shouldn't we do > > > > if (unshare_flags & CLONE_SIGHAND) > > unshare_flags |= CLONE_VM; > > Nope. The backward definitions of the flags in unshare has gotten you. See below, > CLONE_SIGHAND means that you want a struct sighand_struct with a count > of 1. This is (almost) true, > Nothing about a sighand_struct with a count of 1 implies or > requires mm_users == 1. clone can quite happily create those. See if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM)) in copy_process(). So if you have a shared sighand_struct, your ->mm is also shared, current_is_single_threaded() will notice this. > > Otherwise suppose that a single threaded process does clone(VM | SIGHAND) > > and (say) child does sys_unshare(SIGHAND). This will wrongly succeed > > afaics. > > Why would it be wrong to succeed in that case? struct sighand_struct > has a count of 1. How that? clone(VM | SIGHAND) will share ->sighand and increment its count. > unshare(CLONE_SIGHAND) requests a sighand_struct with > a count of 1. Exactly, that is why it is wrong to succeed. > unshare(SIGHAND) needs to guarantee that when it returns sighand->count == 1. > So unshare(SIGHAND) needs to test for sighand->count == 1. Oh, I do not think we should check sighand->count. This can lead to the same problem we have with the current current->mm->mm_users check. Most probably today nobody increments sighand->count (I didn't even try to verify). But this is possible, and I saw the code which did this to pin ->sighand... Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/