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 <[EMAIL PROTECTED]>
>
> ---
>
> 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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
