linux-kernel - Re: [PATCH v2 1/6] locks: fix unlock when fcntl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160108155533.GA3426@fieldses.org>
Date:	Fri, 8 Jan 2016 10:55:33 -0500
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Jeff Layton <jlayton@...chiereds.net>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v2 1/6] locks: fix unlock when fcntl_setlk races with a
 close

On Fri, Jan 08, 2016 at 08:50:09AM -0500, Jeff Layton wrote:
> Dmitry reported that he was able to reproduce the WARN_ON_ONCE that
> fires in locks_free_lock_context when the flc_posix list isn't empty.
> 
> The problem turns out to be that we're basically rebuilding the
> file_lock from scratch in fcntl_setlk when we discover that the setlk
> has raced with a close. If the l_whence field is SEEK_CUR or SEEK_END,
> then we may end up with fl_start and fl_end values that differ from
> when the lock was initially set, if the file position or length of the
> file has changed in the interim.
> 
> Fix this by just reusing the same lock request structure, and simply
> override fl_type value with F_UNLCK as appropriate. That ensures that
> we really are unlocking the lock that was initially set.

You could also just do a whole-file unlock, couldn't you?  That would
seem less confusing to me.  But maybe I'm missing something.

--b.

> 
> While we're there, make sure that we do pop a WARN_ON_ONCE if the
> removal ever fails. Also return -EBADF in this event, since that's
> what we would have returned if the close had happened earlier.
> 
> Cc: "J. Bruce Fields" <bfields@...ldses.org>
> Cc: Alexander Viro <viro@...iv.linux.org.uk>
> Cc: <stable@...r.kernel.org>
> Fixes: c293621bbf67 (stale POSIX lock handling)
> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
> Signed-off-by: Jeff Layton <jeff.layton@...marydata.com>
> ---
>  fs/locks.c | 51 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 593dca300b29..c263aff793bc 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2181,7 +2181,6 @@ int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd,
>  		goto out;
>  	}
>  
> -again:
>  	error = flock_to_posix_lock(filp, file_lock, &flock);
>  	if (error)
>  		goto out;
> @@ -2223,19 +2222,22 @@ again:
>  	 * Attempt to detect a close/fcntl race and recover by
>  	 * releasing the lock that was just acquired.
>  	 */
> -	/*
> -	 * we need that spin_lock here - it prevents reordering between
> -	 * update of i_flctx->flc_posix and check for it done in close().
> -	 * rcu_read_lock() wouldn't do.
> -	 */
> -	spin_lock(&current->files->file_lock);
> -	f = fcheck(fd);
> -	spin_unlock(&current->files->file_lock);
> -	if (!error && f != filp && flock.l_type != F_UNLCK) {
> -		flock.l_type = F_UNLCK;
> -		goto again;
> +	if (!error && file_lock->fl_type != F_UNLCK) {
> +		/*
> +		 * We need that spin_lock here - it prevents reordering between
> +		 * update of i_flctx->flc_posix and check for it done in
> +		 * close(). rcu_read_lock() wouldn't do.
> +		 */
> +		spin_lock(&current->files->file_lock);
> +		f = fcheck(fd);
> +		spin_unlock(&current->files->file_lock);
> +		if (f != filp) {
> +			file_lock->fl_type = F_UNLCK;
> +			error = do_lock_file_wait(filp, cmd, file_lock);
> +			WARN_ON_ONCE(error);
> +			error = -EBADF;
> +		}
>  	}
> -
>  out:
>  	locks_free_lock(file_lock);
>  	return error;
> @@ -2321,7 +2323,6 @@ int fcntl_setlk64(unsigned int fd, struct file *filp, unsigned int cmd,
>  		goto out;
>  	}
>  
> -again:
>  	error = flock64_to_posix_lock(filp, file_lock, &flock);
>  	if (error)
>  		goto out;
> @@ -2363,14 +2364,22 @@ again:
>  	 * Attempt to detect a close/fcntl race and recover by
>  	 * releasing the lock that was just acquired.
>  	 */
> -	spin_lock(&current->files->file_lock);
> -	f = fcheck(fd);
> -	spin_unlock(&current->files->file_lock);
> -	if (!error && f != filp && flock.l_type != F_UNLCK) {
> -		flock.l_type = F_UNLCK;
> -		goto again;
> +	if (!error && file_lock->fl_type != F_UNLCK) {
> +		/*
> +		 * We need that spin_lock here - it prevents reordering between
> +		 * update of i_flctx->flc_posix and check for it done in
> +		 * close(). rcu_read_lock() wouldn't do.
> +		 */
> +		spin_lock(&current->files->file_lock);
> +		f = fcheck(fd);
> +		spin_unlock(&current->files->file_lock);
> +		if (f != filp) {
> +			file_lock->fl_type = F_UNLCK;
> +			error = do_lock_file_wait(filp, cmd, file_lock);
> +			WARN_ON_ONCE(error);
> +			error = -EBADF;
> +		}
>  	}
> -
>  out:
>  	locks_free_lock(file_lock);
>  	return error;
> -- 
> 2.5.0