Re: [PATCH] Fix potential OOPS in generic_setlease() (v2)

["J. Bruce Fields" <bfields@fieldses.org>]

OK, this version I can't see any more problem with.  Thanks!

--b.

On Thu, Sep 20, 2007 at 12:48:32PM +0400, Pavel Emelyanov wrote:
> This code is run under lock_kernel(), which is dropped during
> sleeping operations, so the following race is possible:
> 
> CPU1:                                CPU2:
>   vfs_setlease();                    vfs_setlease();
>   lock_kernel();
>                                      lock_kernel(); /* spin */
>   generic_setlease():
>     ...
>     for (before = ...)
>     /* here we found some lease after
>      * which we will insert the new one
>      */
>     fl = locks_alloc_lock();
>     /* go to sleep in this allocation and
>      * drop the BKL
>      */
>                                      generic_setlease():
>                                        ...
>                                        for (before = ...)
>                                        /* here we find the "before" pointing
>                                         * at the one we found on CPU1
>                                         */
>                                       ->fl_change(my_before, arg);
>                                               lease_modify();
>                                                      locks_free_lock();
>                                                      /* and we freed it */
>                                      ...
>                                      unlock_kernel();
>    locks_insert_lock(before, fl);
>    /* OOPS! We have just tried to add the lease
>     * at the tail of already removed one
>     */
> 
> The similar races are already handled in other code - all the 
> allocations are performed before any checks/updates.
> 
> Fixed the problem, spotted by J. Bruce Fields, about the fl
> variable reuse.
> 
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> 
> ---
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index a1c1c01..d5b9653 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1354,6 +1354,7 @@ int fcntl_getlease(struct file *filp)
>  int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
>  {
>  	struct file_lock *fl, **before, **my_before = NULL, *lease;
> +	struct file_lock *new_fl = NULL;
>  	struct dentry *dentry = filp->f_path.dentry;
>  	struct inode *inode = dentry->d_inode;
>  	int error, rdlease_count = 0, wrlease_count = 0;
> @@ -1380,6 +1381,11 @@ int generic_setlease(struct file *filp, 
>  		|| (atomic_read(&inode->i_count) > 1)))
>  		goto out;
>  
> +	error = -ENOMEM;
> +	new_fl = locks_alloc_lock();
> +	if (new_fl == NULL)
> +		goto out;
> +
>  	/*
>  	 * At this point, we know that if there is an exclusive
>  	 * lease on this file, then we hold it on this filp
> @@ -1422,18 +1428,15 @@ int generic_setlease(struct file *filp, 
>  	if (!leases_enable)
>  		goto out;
>  
> -	error = -ENOMEM;
> -	fl = locks_alloc_lock();
> -	if (fl == NULL)
> -		goto out;
> -
> -	locks_copy_lock(fl, lease);
> -
> -	locks_insert_lock(before, fl);
> +	locks_copy_lock(new_fl, lease);
> +	locks_insert_lock(before, new_fl);
>  
>  	*flp = fl;
> -	error = 0;
> +	return 0;
> +
>  out:
> +	if (new_fl != NULL)
> +		locks_free_lock(new_fl);
>  	return error;
>  }
>  EXPORT_SYMBOL(generic_setlease);