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

Oleg Nesterov
2014-08-22T19:25:22Z

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.