Re: + prctl-pr_set_mm-introduce-pr_set_mm_map-operation-v3.patch added to -mm tree

[Oleg Nesterov <oleg@redhat.com>]

Hi Cyrill,

I think the patch is fine but I can't understand the usage of mmap_sem
and alloc_lock,

> +	stack_vma = find_vma(mm, (unsigned long)prctl_map->start_stack);

OK, find_vma() needs mmap_sem. But otherwise, why this should be called
under down_read(&mm->mmap_sem) ? What this lock tries to protect?

> +	if (prctl_map.auxv_size) {
> +		up_read(&mm->mmap_sem);
> +		memset(user_auxv, 0, sizeof(user_auxv));
> +		error = copy_from_user(user_auxv,
> +				       (const void __user *)prctl_map.auxv,
> +				       prctl_map.auxv_size);
> +		down_read(&mm->mmap_sem);

And if we actually need this lock, why it is safe to drop it temporary?
And why we can't move this copy_from_user() up before down_read) in any
case?

> +	if (prctl_map.auxv_size) {
> +		/* Last entry must be AT_NULL as specification requires */
> +		user_auxv[AT_VECTOR_SIZE - 2] = AT_NULL;
> +		user_auxv[AT_VECTOR_SIZE - 1] = AT_NULL;
> +
> +		task_lock(current);
> +		memcpy(mm->saved_auxv, user_auxv, sizeof(user_auxv));
> +		task_unlock(current);

Again, could you explain this task_lock() ?

Oleg.