what is_single_threaded() does?

what is_single_threaded() does?

[Oleg Nesterov]

I found this helper by accident, and I am puzzled.

	/**
	 * is_single_threaded - Determine if a thread group is single-threaded or not
	 * @p: A task in the thread group in question
	 *
	 * This returns true if the thread group to which a task belongs is single
	 * threaded, false if it is not.
	 */

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 ?

Could you explain what is right, the comment or the code?

	bool is_single_threaded(struct task_struct *p)
	{
		struct task_struct *g, *t;
		struct mm_struct *mm = p->mm;

		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.


		if (atomic_read(&p->mm->mm_users) != 1) {
			read_lock(&tasklist_lock);
			do_each_thread(g, t) {

Why do_each_thread() ? for_each_process() is enough, all sub-threads use
the same ->mm.

				if (t->mm == mm && t != p)
					goto no_unlock;

What about use_mm() ? Looks like this needs PF_KTHREAD check.

			} while_each_thread(g, t);
			read_unlock(&tasklist_lock);
		}

		return true;

Perhaps it should be current_is_single_thread(void) ...

Oleg.

Re: what is_single_threaded() does?

[David Howells]

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.

Linux has a very fuzzy view of threads, whereby different tasks can share
different sets of things.  In my opinion it's excessive and unnecessary, and
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.

It appears that the zombie would retain a pointer to p->signal so that
wait_task_zombie() can get stuff out of it - but can wait_task_zombie()
actually access a thread group that still has active threads?

I don't think this is a real problem, at least for the two security users of
it.  It is still effectively multithreaded, even though one of the threads is
a zombie, and indeed it would appear the process is busy imploding.

> 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; 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, it is neither required nor enforced that all sub-threads use the
same ->mm.

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

> 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?

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

Perhaps.

Re: what is_single_threaded() does?

[Oleg Nesterov]

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.