netdev - Re: [PATCH] vsock: make listener child lock ordering explicit

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]

Date:	Fri, 24 Jun 2016 13:51:46 +0000
From:	"Jorgen S. Hansen" <jhansen@...are.com>
To:	Stefan Hajnoczi <stefanha@...hat.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Claudio Imbrenda <imbrenda@...ux.vnet.ibm.com>,
	"Jorgen S. Hansen" <jhansen@...are.com>
Subject: Re: [PATCH] vsock: make listener child lock ordering explicit


> On Jun 23, 2016, at 5:28 PM, Stefan Hajnoczi <stefanha@...hat.com> wrote:
> 
> There are several places where the listener and pending or accept queue
> child sockets are accessed at the same time.  Lockdep is unhappy that
> two locks from the same class are held.
> 
> Tell lockdep that it is safe and document the lock ordering.
> 
> Originally Claudio Imbrenda <imbrenda@...ux.vnet.ibm.com> sent a similar
> patch asking whether this is safe.  I have audited the code and also
> covered the vsock_pending_work() function.
> 
> Suggested-by: Claudio Imbrenda <imbrenda@...ux.vnet.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@...hat.com>
> ---
> net/vmw_vsock/af_vsock.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index b5f1221..b96ac91 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -61,6 +61,14 @@
>  * function will also cleanup rejected sockets, those that reach the connected
>  * state but leave it before they have been accepted.
>  *
> + * - Lock ordering for pending or accept queue sockets is:
> + *
> + *     lock_sock(listener);
> + *     lock_sock_nested(pending, SINGLE_DEPTH_NESTING);
> + *
> + * Using explicit nested locking keeps lockdep happy since normally only one
> + * lock of a given class may be taken at a time.
> + *
>  * - Sockets created by user action will be cleaned up when the user process
>  * calls close(2), causing our release implementation to be called. Our release
>  * implementation will perform some cleanup then drop the last reference so our
> @@ -443,7 +451,7 @@ void vsock_pending_work(struct work_struct *work)
> 	cleanup = true;
> 
> 	lock_sock(listener);
> -	lock_sock(sk);
> +	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> 
> 	if (vsock_is_pending(sk)) {
> 		vsock_remove_pending(listener, sk);
> @@ -1292,7 +1300,7 @@ static int vsock_accept(struct socket *sock, struct socket *newsock, int flags)
> 	if (connected) {
> 		listener->sk_ack_backlog--;
> 
> -		lock_sock(connected);
> +		lock_sock_nested(connected, SINGLE_DEPTH_NESTING);
> 		vconnected = vsock_sk(connected);
> 
> 		/* If the listener socket has received an error, then we should
> -- 
> 2.7.4
> 

Looks good to me - thanks for fixing this!

/jsh