Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 9 Jul 2007 22:49:17 +0400 Dmitry Monakhov <dmonakhov@sw.ru> wrote:

> Some device drivers can change vm_flags in their f_op->mmap
> method. In order to be on the safe side we have to recheck
> lock rlimit. Now we have to check lock rlimit from two places,
> let's move this common code to helper functon.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
>  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 906ed40..5c89f1d 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -885,6 +885,18 @@ void vm_stat_account(struct mm_struct *mm, unsigned long flags,
>  }
>  #endif /* CONFIG_PROC_FS */
>  
> +static int check_lock_limit(unsigned long delta, struct mm_struct* mm)
> +{
> +	unsigned long locked, lock_limit;
> +	locked = delta >> PAGE_SHIFT;
> +	locked += mm->locked_vm;
> +	lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
> +	lock_limit >>= PAGE_SHIFT;
> +	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> +		return -EAGAIN;
> +	return 0;
> +}
> +
>  /*
>   * The caller must hold down_write(current->mm->mmap_sem).
>   */
> @@ -954,13 +966,9 @@ unsigned long do_mmap_pgoff(struct file * file, unsigned long addr,
>  	}
>  	/* mlock MCL_FUTURE? */
>  	if (vm_flags & VM_LOCKED) {
> -		unsigned long locked, lock_limit;
> -		locked = len >> PAGE_SHIFT;
> -		locked += mm->locked_vm;
> -		lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
> -		lock_limit >>= PAGE_SHIFT;
> -		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> -			return -EAGAIN;
> +		error = check_lock_limit(len, mm);
> +		if (error)
> +			return error;
>  	}
>  
>  	inode = file ? file->f_path.dentry->d_inode : NULL;
> @@ -1101,6 +1109,17 @@ munmap_back:
>  		error = file->f_op->mmap(file, vma);
>  		if (error)
>  			goto unmap_and_free_vma;
> +		
> +		if (vma->vm_flags & VM_LOCKED
> +				&& !(vm_flags & VM_LOCKED)) {
> +		/*
> +		 * VM_LOCKED was added in f_op->mmap() method,
> +		 * so we have to recheck limit.
> +		 */
> +			error = check_lock_limit(len, mm);
> +			if (error)
> +				goto unmap_and_free_vma;
> +		}

Worried.  As far as the filesytem is concerned, its mmap has succeeded.

But now we're taking the unmap_and_free_vma path _after_ ->mmap() has
"succeeded".  So we will now tell userspace that the mmap syscall has
failed, even though the fs thinks it succeeded, if you follow me.  And this
is a new thing.  

Could it cause bad things to happen?  Well, if filesystems had a
file_operations.munmap() then yeah, we should have called that in your new
code.  But filesystems don't have a ->munmap() method.

Still.  Can we think of any way in which this change could lead to resource
leaks or to any other such problems?