diff mbox

net/ipv6: potential deadlock in do_ipv6_setsockopt

Message ID CAM_iQpWKfkkYzF6yb9fGQsieGwok196kqGtRNVVS_ULxZghyew@mail.gmail.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Cong Wang Oct. 16, 2016, 6:50 p.m. UTC
On Sun, Oct 16, 2016 at 6:34 AM, Baozeng Ding <sploving1@gmail.com> wrote:
>  Possible unsafe locking scenario:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock([  165.136033] sk_lock-AF_INET6
> );
>                                lock([  165.136033] rtnl_mutex
> );
>                                lock([  165.136033] sk_lock-AF_INET6
> );
>   lock([  165.136033] rtnl_mutex
> );
>
>  *** DEADLOCK ***

This is caused by the conditional rtnl locking in do_ipv6_setsockopt().
It looks like we miss the case of IPV6_ADDRFORM.

Please try the attached patch.

Comments

Baozeng Ding Oct. 17, 2016, 9:54 a.m. UTC | #1
Applied the patch to my test tree. I will tell you the result a few days later. Thank you.

On 2016/10/17 2:50, Cong Wang wrote:
> On Sun, Oct 16, 2016 at 6:34 AM, Baozeng Ding <sploving1@gmail.com> wrote:
>>  Possible unsafe locking scenario:
>>
>>        CPU0                    CPU1
>>        ----                    ----
>>   lock([  165.136033] sk_lock-AF_INET6
>> );
>>                                lock([  165.136033] rtnl_mutex
>> );
>>                                lock([  165.136033] sk_lock-AF_INET6
>> );
>>   lock([  165.136033] rtnl_mutex
>> );
>>
>>  *** DEADLOCK ***
> 
> This is caused by the conditional rtnl locking in do_ipv6_setsockopt().
> It looks like we miss the case of IPV6_ADDRFORM.
> 
> Please try the attached patch.
>
Baozeng Ding Oct. 19, 2016, 7:45 a.m. UTC | #2
It fixes the issue for me. 
Tested-by: Baozeng Ding <sploving1@gmail.com>

On 2016/10/17 17:54, Baozeng Ding wrote:
> Applied the patch to my test tree. I will tell you the result a few days later. Thank you.
> 
> On 2016/10/17 2:50, Cong Wang wrote:
>> On Sun, Oct 16, 2016 at 6:34 AM, Baozeng Ding <sploving1@gmail.com> wrote:
>>>  Possible unsafe locking scenario:
>>>
>>>        CPU0                    CPU1
>>>        ----                    ----
>>>   lock([  165.136033] sk_lock-AF_INET6
>>> );
>>>                                lock([  165.136033] rtnl_mutex
>>> );
>>>                                lock([  165.136033] sk_lock-AF_INET6
>>> );
>>>   lock([  165.136033] rtnl_mutex
>>> );
>>>
>>>  *** DEADLOCK ***
>>
>> This is caused by the conditional rtnl locking in do_ipv6_setsockopt().
>> It looks like we miss the case of IPV6_ADDRFORM.
>>
>> Please try the attached patch.
>>
Baozeng Ding Oct. 19, 2016, 1:44 p.m. UTC | #3
It fixes the issue for me. 
Tested-by: Baozeng Ding <sploving1@gmail.com>

On 2016/10/17 17:54, Baozeng Ding wrote:
> Applied the patch to my test tree. I will tell you the result a few days later. Thank you.
> 
> On 2016/10/17 2:50, Cong Wang wrote:
>> On Sun, Oct 16, 2016 at 6:34 AM, Baozeng Ding <sploving1@gmail.com> wrote:
>>>  Possible unsafe locking scenario:
>>>
>>>        CPU0                    CPU1
>>>        ----                    ----
>>>   lock([  165.136033] sk_lock-AF_INET6
>>> );
>>>                                lock([  165.136033] rtnl_mutex
>>> );
>>>                                lock([  165.136033] sk_lock-AF_INET6
>>> );
>>>   lock([  165.136033] rtnl_mutex
>>> );
>>>
>>>  *** DEADLOCK ***
>>
>> This is caused by the conditional rtnl locking in do_ipv6_setsockopt().
>> It looks like we miss the case of IPV6_ADDRFORM.
>>
>> Please try the attached patch.
>>
Cong Wang Oct. 19, 2016, 4:36 p.m. UTC | #4
On Wed, Oct 19, 2016 at 12:45 AM, Baozeng Ding <sploving1@gmail.com> wrote:
> It fixes the issue for me.
> Tested-by: Baozeng Ding <sploving1@gmail.com>

Thanks for testing, I will send out the patch formally very soon.

BTW, you can check mailing archive to see if your email succeeds or not,
for example, http://marc.info/?l=linux-netdev
diff mbox

Patch

diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 46ad699..b8c8d20 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -414,7 +414,9 @@  int inet6_release(struct socket *sock)
 		return -EINVAL;
 
 	/* Free mc lists */
+	rtnl_lock();
 	ipv6_sock_mc_close(sk);
+	rtnl_unlock();
 
 	/* Free ac lists */
 	ipv6_sock_ac_close(sk);
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 5330262..1e4bcce 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -120,6 +120,7 @@  struct ipv6_txoptions *ipv6_update_options(struct sock *sk,
 static bool setsockopt_needs_rtnl(int optname)
 {
 	switch (optname) {
+	case IPV6_ADDRFORM:
 	case IPV6_ADD_MEMBERSHIP:
 	case IPV6_DROP_MEMBERSHIP:
 	case IPV6_JOIN_ANYCAST:
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 75c1fc5..41badfd 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -282,10 +282,11 @@  void ipv6_sock_mc_close(struct sock *sk)
 	struct ipv6_mc_socklist *mc_lst;
 	struct net *net = sock_net(sk);
 
+	ASSERT_RTNL();
+
 	if (!rcu_access_pointer(np->ipv6_mc_list))
 		return;
 
-	rtnl_lock();
 	while ((mc_lst = rtnl_dereference(np->ipv6_mc_list)) != NULL) {
 		struct net_device *dev;
 
@@ -305,7 +306,6 @@  void ipv6_sock_mc_close(struct sock *sk)
 		kfree_rcu(mc_lst, rcu);
 
 	}
-	rtnl_unlock();
 }
 
 int ip6_mc_source(int add, int omode, struct sock *sk,