netdev - Re: net/sctp: recursive locking in sctp_do

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] [day] [month] [year] [list]

Date:   Wed, 22 Mar 2017 13:29:22 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vladislav Yasevich <vyasevich@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        David Miller <davem@...emloft.net>, linux-sctp@...r.kernel.org,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: net/sctp: recursive locking in sctp_do_peeloff

On Wed, Mar 15, 2017 at 7:19 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
> On Wed, Mar 15, 2017 at 5:52 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@...il.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