Re: what is_single_threaded() does?

[Oleg Nesterov <oleg@redhat.com>]

On 04/02, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > But this is not what the code does? The "t->mm == mm" check below means
> > it also returns false if ->mm is shared with another CLONE_VM process ?
>
> It's a matter of defining what is meant by single-threaded, I suppose.  For
> the purposes of security checks, that means not being part of the same group
> of threads and not sharing VM space.

Then I think it is better to change the comment,

> Linux has a very fuzzy view of threads, whereby different tasks can share
> different sets of things.

The comment says:

	Determine if a thread group is single-threaded

"thread group" is what we have in ->thread_group.

> probably mostly unused.
>
> > 		if (atomic_read(&p->signal->count) != 1)
> > 			goto no;
> >
> > Is this correct? Let's suppose the main thread dies, and the thread group
> > has only one live thread. In that case signal->count == 2.
>
> Doesn't exit() kill the subsidiary threads in such a case?  I don't recall.

No. Well, sys_exit() (and thus pthread_exit()) doesn't.

> It appears that the zombie would retain a pointer to p->signal so that
> wait_task_zombie() can get stuff out of it -

Yes,

> but can wait_task_zombie()
> actually access a thread group that still has active threads?

Not sure I understand... But, if a thread group that still has active threads,
wait_task_zombie() will not be called.

> I don't think this is a real problem, at least for the two security users of
> it.

I do not claim this is security problem. But still it doesn't look right
to me,

> It is still effectively multithreaded,

Why? The main thread is really dead, it has no ->mm, it can do nothing.
We only have a task_struct which is not released untill all threads exit.

> > Why do_each_thread() ? for_each_process() is enough, all sub-threads use
> > the same ->mm.
>
> Firstly, that's what the original code that I extract out to this function
> did;

OK,

> secondly, it doesn't make much difference: do_each_thread() does the
> filtering for us that we'd have to do ourselves if we used for_each_process();
> and thirdly,

What do you mean? do_each_thread() iterates over all threads in system,
the number of processes can be much lower. And we hold tasklist, not
good.

> it is neither required nor enforced that all sub-threads use the
> same ->mm.

It is required and enforced. CLONE_THREAD without CLONE_VM results in -EINVAL.

> Actually, a better way of doing things may be to use a list of threads rooted
> on signal_struct.

Can't understand. We already have this list of threads: ->thread_group.

And confused. You said "and not sharing VM space" above, how this list
can help?

> > What about use_mm() ? Looks like this needs PF_KTHREAD check.
>
> I'm not sure what you mean.  Are you suggesting this should use use_mm()?  Or
> are you suggesting that use_mm() is wrong?

The aio kernel thread can borrow ->mm from user-space process. In that
case is_single_threaded() will wrongly retun false. is_single_threaded()
should not check ->mm if PF_KTHREAD.

> > Perhaps it should be current_is_single_thread(void) ...
>
> Perhaps.

Yes, because it doesn't makes to much sense to call this helper
unless p == current.

Oleg.