Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1601054 > unrolled thread

Re: net/sctp: recursive locking in sctp_do_peeloff

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: net/sctp: recursive locking in sctp_do_peeloff Cong Wang <xiyou.wangcong@gmail.com> - 2017-03-15 06:00 +0100
    Re: net/sctp: recursive locking in sctp_do_peeloff Dmitry Vyukov <dvyukov@google.com> - 2017-03-15 11:20 +0100
    Re: net/sctp: recursive locking in sctp_do_peeloff Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> - 2017-03-15 14:00 +0100
      Re: net/sctp: recursive locking in sctp_do_peeloff Cong Wang <xiyou.wangcong@gmail.com> - 2017-03-15 19:30 +0100
        Re: net/sctp: recursive locking in sctp_do_peeloff Dmitry Vyukov <dvyukov@google.com> - 2017-03-22 13:40 +0100

#1601054 — Re: net/sctp: recursive locking in sctp_do_peeloff

On Fri, Mar 10, 2017 at 12:04 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Fri, Mar 10, 2017 at 8:46 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
>> On Fri, Mar 10, 2017 at 4:11 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> Hello,
>>>
>>> I've got the following recursive locking report while running
>>> syzkaller fuzzer on net-next/9c28286b1b4b9bce6e35dd4c8a1265f03802a89a:
>>>
>>> [ INFO: possible recursive locking detected ]
>>> 4.10.0+ #14 Not tainted
>>> ---------------------------------------------
>>> syz-executor3/5560 is trying to acquire lock:
>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff8401ebcd>] lock_sock
>>> include/net/sock.h:1460 [inline]
>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff8401ebcd>]
>>> sctp_close+0xcd/0x9d0 net/sctp/socket.c:1497
>>>
>>> but task is already holding lock:
>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff84038110>] lock_sock
>>> include/net/sock.h:1460 [inline]
>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff84038110>]
>>> sctp_getsockopt+0x450/0x67e0 net/sctp/socket.c:6611
>>>
>>> other info that might help us debug this:
>>>  Possible unsafe locking scenario:
>>>
>>>        CPU0
>>>        ----
>>>   lock(sk_lock-AF_INET6);
>>>   lock(sk_lock-AF_INET6);
>>>
>>>  *** DEADLOCK ***
>>>
>>>  May be due to missing lock nesting notation
>>
>> Pretty much the case, I suppose. The lock held by sctp_getsockopt() is
>> on one socket, while the other lock that sctp_close() is getting later
>> is on the newly created (which failed) socket during peeloff
>> operation.
>
>
> Does this mean that never-ever lock 2 sockets at a time except for
> this case? If so, it probably suggests that this case should not do it
> either.
>

Yeah, actually for the error path we don't even need to lock sock
since it is newly allocated and no one else could see it yet.

Instead of checking for the status of the sock, I believe the following
one-line fix should do the trick too. Can you give it a try?

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 0f378ea..4de62d4 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1494,7 +1494,7 @@ static void sctp_close(struct sock *sk, long timeout)

        pr_debug("%s: sk:%p, timeout:%ld\n", __func__, sk, timeout);

-       lock_sock(sk);
+       lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
        sk->sk_shutdown = SHUTDOWN_MASK;
        sk->sk_state = SCTP_SS_CLOSING;

[toc] | [next] | [standalone]

#1601225

On Wed, Mar 15, 2017 at 5:52 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 12:04 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Fri, Mar 10, 2017 at 8:46 PM, Marcelo Ricardo Leitner
>> <marcelo.leitner@gmail.com> wrote:
>>> On Fri, Mar 10, 2017 at 4:11 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> Hello,
>>>>
>>>> I've got the following recursive locking report while running
>>>> syzkaller fuzzer on net-next/9c28286b1b4b9bce6e35dd4c8a1265f03802a89a:
>>>>
>>>> [ INFO: possible recursive locking detected ]
>>>> 4.10.0+ #14 Not tainted
>>>> ---------------------------------------------
>>>> syz-executor3/5560 is trying to acquire lock:
>>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff8401ebcd>] lock_sock
>>>> include/net/sock.h:1460 [inline]
>>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff8401ebcd>]
>>>> sctp_close+0xcd/0x9d0 net/sctp/socket.c:1497
>>>>
>>>> but task is already holding lock:
>>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff84038110>] lock_sock
>>>> include/net/sock.h:1460 [inline]
>>>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff84038110>]
>>>> sctp_getsockopt+0x450/0x67e0 net/sctp/socket.c:6611
>>>>
>>>> other info that might help us debug this:
>>>>  Possible unsafe locking scenario:
>>>>
>>>>        CPU0
>>>>        ----
>>>>   lock(sk_lock-AF_INET6);
>>>>   lock(sk_lock-AF_INET6);
>>>>
>>>>  *** DEADLOCK ***
>>>>
>>>>  May be due to missing lock nesting notation
>>>
>>> Pretty much the case, I suppose. The lock held by sctp_getsockopt() is
>>> on one socket, while the other lock that sctp_close() is getting later
>>> is on the newly created (which failed) socket during peeloff
>>> operation.
>>
>>
>> Does this mean that never-ever lock 2 sockets at a time except for
>> this case? If so, it probably suggests that this case should not do it
>> either.
>>
>
> Yeah, actually for the error path we don't even need to lock sock
> since it is newly allocated and no one else could see it yet.
>
> Instead of checking for the status of the sock, I believe the following
> one-line fix should do the trick too. Can you give it a try?
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 0f378ea..4de62d4 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1494,7 +1494,7 @@ static void sctp_close(struct sock *sk, long timeout)
>
>         pr_debug("%s: sk:%p, timeout:%ld\n", __func__, sk, timeout);
>
> -       lock_sock(sk);
> +       lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>         sk->sk_shutdown = SHUTDOWN_MASK;
>         sk->sk_state = SCTP_SS_CLOSING;


Hi  Cong,

I've applied the patch on bots. But so far it happened only once, so I
am not sure I will be able to give any conclusion.

[toc] | [prev] | [next] | [standalone]

#1601378

On Tue, Mar 14, 2017 at 09:52:15PM -0700, Cong Wang wrote:
> On Fri, Mar 10, 2017 at 12:04 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > On Fri, Mar 10, 2017 at 8:46 PM, Marcelo Ricardo Leitner
> > <marcelo.leitner@gmail.com> wrote:
> >> On Fri, Mar 10, 2017 at 4:11 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> >>> Hello,
> >>>
> >>> I've got the following recursive locking report while running
> >>> syzkaller fuzzer on net-next/9c28286b1b4b9bce6e35dd4c8a1265f03802a89a:
> >>>
> >>> [ INFO: possible recursive locking detected ]
> >>> 4.10.0+ #14 Not tainted
> >>> ---------------------------------------------
> >>> syz-executor3/5560 is trying to acquire lock:
> >>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff8401ebcd>] lock_sock
> >>> include/net/sock.h:1460 [inline]
> >>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff8401ebcd>]
> >>> sctp_close+0xcd/0x9d0 net/sctp/socket.c:1497
> >>>
> >>> but task is already holding lock:
> >>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff84038110>] lock_sock
> >>> include/net/sock.h:1460 [inline]
> >>>  (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffff84038110>]
> >>> sctp_getsockopt+0x450/0x67e0 net/sctp/socket.c:6611
> >>>
> >>> other info that might help us debug this:
> >>>  Possible unsafe locking scenario:
> >>>
> >>>        CPU0
> >>>        ----
> >>>   lock(sk_lock-AF_INET6);
> >>>   lock(sk_lock-AF_INET6);
> >>>
> >>>  *** DEADLOCK ***
> >>>
> >>>  May be due to missing lock nesting notation
> >>
> >> Pretty much the case, I suppose. The lock held by sctp_getsockopt() is
> >> on one socket, while the other lock that sctp_close() is getting later
> >> is on the newly created (which failed) socket during peeloff
> >> operation.
> >
> >
> > Does this mean that never-ever lock 2 sockets at a time except for
> > this case? If so, it probably suggests that this case should not do it
> > either.
> >
> 
> Yeah, actually for the error path we don't even need to lock sock
> since it is newly allocated and no one else could see it yet.
> 

Agreed.

> Instead of checking for the status of the sock, I believe the following
> one-line fix should do the trick too. Can you give it a try?
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 0f378ea..4de62d4 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1494,7 +1494,7 @@ static void sctp_close(struct sock *sk, long timeout)
> 
>         pr_debug("%s: sk:%p, timeout:%ld\n", __func__, sk, timeout);
> 
> -       lock_sock(sk);
> +       lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>         sk->sk_shutdown = SHUTDOWN_MASK;
>         sk->sk_state = SCTP_SS_CLOSING;

I refrained on doing this just because it will change the lock signature
for the first level too, as sctp_close() can be called directly, and
might avoid some other lockdep detections.

Then you probably also need:
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 465a9c8464f9..02506b4406d2 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1543,7 +1543,7 @@ static void sctp_close(struct sock *sk, long timeout)
 	 * held and that should be grabbed before socket lock.
 	 */
 	spin_lock_bh(&net->sctp.addr_wq_lock);
-	bh_lock_sock(sk);
+	bh_lock_sock_nested(sk);
 
 	/* Hold the sock, since sk_common_release() will put sock_put()
 	 * and we have just a little more cleanup.

because sctp_close will re-lock the socket a little later (for backlog
processing).

  Marcelo

[toc] | [prev] | [next] | [standalone]

#1601630

On Wed, Mar 15, 2017 at 5:52 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Tue, Mar 14, 2017 at 09:52:15PM -0700, Cong Wang wrote:
>> Instead of checking for the status of the sock, I believe the following
>> one-line fix should do the trick too. Can you give it a try?
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 0f378ea..4de62d4 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1494,7 +1494,7 @@ static void sctp_close(struct sock *sk, long timeout)
>>
>>         pr_debug("%s: sk:%p, timeout:%ld\n", __func__, sk, timeout);
>>
>> -       lock_sock(sk);
>> +       lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>>         sk->sk_shutdown = SHUTDOWN_MASK;
>>         sk->sk_state = SCTP_SS_CLOSING;
>
> I refrained on doing this just because it will change the lock signature
> for the first level too, as sctp_close() can be called directly, and
> might avoid some other lockdep detections.

I knew, but for the first level it is fine to use a different class,
it is merely to make lockdep happy. There is no real deadlock here
since they are two different socks anyway.

>
> Then you probably also need:
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 465a9c8464f9..02506b4406d2 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1543,7 +1543,7 @@ static void sctp_close(struct sock *sk, long timeout)
>          * held and that should be grabbed before socket lock.
>          */
>         spin_lock_bh(&net->sctp.addr_wq_lock);
> -       bh_lock_sock(sk);
> +       bh_lock_sock_nested(sk);
>
>         /* Hold the sock, since sk_common_release() will put sock_put()
>          * and we have just a little more cleanup.
>
> because sctp_close will re-lock the socket a little later (for backlog
> processing).
>

Ah, of course I missed the re-lock. Dmitry, please add this piece too.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1606424

On Wed, Mar 15, 2017 at 7:19 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Mar 15, 2017 at 5:52 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
>> On Tue, Mar 14, 2017 at 09:52:15PM -0700, Cong Wang wrote:
>>> Instead of checking for the status of the sock, I believe the following
>>> one-line fix should do the trick too. Can you give it a try?
>>>
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index 0f378ea..4de62d4 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -1494,7 +1494,7 @@ static void sctp_close(struct sock *sk, long timeout)
>>>
>>>         pr_debug("%s: sk:%p, timeout:%ld\n", __func__, sk, timeout);
>>>
>>> -       lock_sock(sk);
>>> +       lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>>>         sk->sk_shutdown = SHUTDOWN_MASK;
>>>         sk->sk_state = SCTP_SS_CLOSING;
>>
>> I refrained on doing this just because it will change the lock signature
>> for the first level too, as sctp_close() can be called directly, and
>> might avoid some other lockdep detections.
>
> I knew, but for the first level it is fine to use a different class,
> it is merely to make lockdep happy. There is no real deadlock here
> since they are two different socks anyway.
>
>>
>> Then you probably also need:
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 465a9c8464f9..02506b4406d2 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1543,7 +1543,7 @@ static void sctp_close(struct sock *sk, long timeout)
>>          * held and that should be grabbed before socket lock.
>>          */
>>         spin_lock_bh(&net->sctp.addr_wq_lock);
>> -       bh_lock_sock(sk);
>> +       bh_lock_sock_nested(sk);
>>
>>         /* Hold the sock, since sk_common_release() will put sock_put()
>>          * and we have just a little more cleanup.
>>
>> because sctp_close will re-lock the socket a little later (for backlog
>> processing).
>>
>
> Ah, of course I missed the re-lock. Dmitry, please add this piece too.


applied

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web