[PATCH] sctp: lock_sock_nested in sctp_sock_migrate

[PATCH] sctp: lock_sock_nested in sctp_sock_migrate

[Zach Brown]

I'm not sure that I've gotten either the sctp or lockdep details right,
but with this patch I don't get lockdep yelling at me any more :)

------

sctp: lock_sock_nested in sctp_sock_migrate

sctp_sock_migrate() grabs the socket lock on a newly allocated socket while
holding the socket lock on an old socket.  lockdep worries that this might
be a recursive lock attempt.

 task/3026 is trying to acquire lock:
  (sk_lock-AF_INET){--..}, at: [<ffffffff88105b8c>] sctp_sock_migrate+0x2e3/0x327 [sctp]
 but task is already holding lock:
  (sk_lock-AF_INET){--..}, at: [<ffffffff8810891f>] sctp_accept+0xdf/0x1e3 [sctp]

This patch tells lockdep that this locking is safe by using
lock_sock_nested().

Signed-off-by: Zach Brown <zach.brown@oracle.com>

diff -r 8adcfdf2545b net/sctp/socket.c
--- a/net/sctp/socket.c	Fri Jun 22 11:11:33 2007 -0700
+++ b/net/sctp/socket.c	Fri Jun 22 15:05:22 2007 -0700
@@ -6084,8 +6084,11 @@ static void sctp_sock_migrate(struct soc
 	 * queued to the backlog.  This prevents a potential race between
 	 * backlog processing on the old socket and new-packet processing
 	 * on the new socket.
-	 */
-	sctp_lock_sock(newsk);
+	 *
+	 * The caller has just allocated newsk so we can guarantee that other
+	 * paths won't try to lock it and then oldsk.
+	 */
+	lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
 	sctp_assoc_migrate(assoc, newsk);
 
 	/* If the association on the newsk is already closed before accept()

Re: [PATCH] sctp: lock_sock_nested in sctp_sock_migrate

[Arjan van de Ven]

Vlad Yasevich wrote:
> Hm... This is another case of of two different sockets taking the same lock...
> 
> Arjan,  did this every get fixed, or is the nested locking the right solution
> to this?
> 

for this specific case it's ok and the nested solution is right.
In the general case it's obviously not safe to take the locks of two 
sockets in "unspecified" order....

Re: [PATCH] sctp: lock_sock_nested in sctp_sock_migrate

[Vlad Yasevich]

Zach Brown wrote:
> I'm not sure that I've gotten either the sctp or lockdep details right,
> but with this patch I don't get lockdep yelling at me any more :)
> 
> ------
> 
> sctp: lock_sock_nested in sctp_sock_migrate
> 
> sctp_sock_migrate() grabs the socket lock on a newly allocated socket while
> holding the socket lock on an old socket.  lockdep worries that this might
> be a recursive lock attempt.
> 
>  task/3026 is trying to acquire lock:
>   (sk_lock-AF_INET){--..}, at: [<ffffffff88105b8c>] sctp_sock_migrate+0x2e3/0x327 [sctp]
>  but task is already holding lock:
>   (sk_lock-AF_INET){--..}, at: [<ffffffff8810891f>] sctp_accept+0xdf/0x1e3 [sctp]
> 
> This patch tells lockdep that this locking is safe by using
> lock_sock_nested().

Hm... This is another case of of two different sockets taking the same lock...

Arjan,  did this every get fixed, or is the nested locking the right solution
to this?

Thanks
-vlad

> 
> Signed-off-by: Zach Brown <zach.brown@oracle.com>
> 
> diff -r 8adcfdf2545b net/sctp/socket.c
> --- a/net/sctp/socket.c	Fri Jun 22 11:11:33 2007 -0700
> +++ b/net/sctp/socket.c	Fri Jun 22 15:05:22 2007 -0700
> @@ -6084,8 +6084,11 @@ static void sctp_sock_migrate(struct soc
>  	 * queued to the backlog.  This prevents a potential race between
>  	 * backlog processing on the old socket and new-packet processing
>  	 * on the new socket.
> -	 */
> -	sctp_lock_sock(newsk);
> +	 *
> +	 * The caller has just allocated newsk so we can guarantee that other
> +	 * paths won't try to lock it and then oldsk.
> +	 */
> +	lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
>  	sctp_assoc_migrate(assoc, newsk);
>  
>  	/* If the association on the newsk is already closed before accept()
> 

Re: [PATCH] sctp: lock_sock_nested in sctp_sock_migrate

[Vlad Yasevich]

Arjan van de Ven wrote:
> Vlad Yasevich wrote:
>> Hm... This is another case of of two different sockets taking the same
>> lock...
>>
>> Arjan,  did this every get fixed, or is the nested locking the right
>> solution
>> to this?
>>
> 
> for this specific case it's ok and the nested solution is right.
> In the general case it's obviously not safe to take the locks of two
> sockets in "unspecified" order....
> 

Well, in this case the order is very carefully specified, but I was more
interested in what the right solution is.

The newsk, from the patch, has just been created, but needs to be locked
to prevent soft_irq from queuing packets to it while we are mucking around.
This is the same case as the accept case that had issues some time ago.

-vlad