linux-kernel - Re: [PATCH V2 10/11] arch: xtensa: platforms: Fix deadlock in iss_net

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]

Message-ID: <CAMo8BfKfxA3b2DmRPc4KK1Mn28-xUBaQeu7ZMUTdoZWr4ESGLw@mail.gmail.com>
Date:   Fri, 8 Apr 2022 23:20:37 -0700
From:   Max Filippov <jcmvbkbc@...il.com>
To:     Duoming Zhou <duoming@....edu.cn>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "open list:TENSILICA XTENSA PORT (xtensa)" 
        <linux-xtensa@...ux-xtensa.org>, Jakub Kicinski <kuba@...nel.org>,
        alexander.deucher@....com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S. Miller" <davem@...emloft.net>,
        Chris Zankel <chris@...kel.net>
Subject: Re: [PATCH V2 10/11] arch: xtensa: platforms: Fix deadlock in iss_net_close()

Hi Duoming,

On Fri, Apr 8, 2022 at 10:51 PM Duoming Zhou <duoming@....edu.cn> wrote:
> There is a deadlock in iss_net_close(), which is shown
> below:
>
>    (Thread 1)              |      (Thread 2)
>                            | iss_net_open()
> iss_net_close()            |  mod_timer()
>  spin_lock_bh() //(1)      |  (wait a time)
>  ...                       | iss_net_timer()
>  del_timer_sync()          |  spin_lock() //(2)
>  (wait timer to stop)      |  ...
>
> We hold lp->lock in position (1) of thread 1 and use
> del_timer_sync() to wait timer to stop, but timer handler
> also need lp->lock in position (2) of thread 2. As a result,
> iss_net_close() will block forever.

Would block forever in SMP, that's true. But the ISS machine
that uses this driver had never had SMP support, so we
haven't seen it blocking so far.

> This patch extracts del_timer_sync() from the protection of
> spin_lock_bh(), which could let timer handler to obtain
> the needed lock. What`s more, we should remove spin_lock()
> in iss_net_timer(), because there is no resource need to
> protect and spin_lock() may cause deadlock in timer handler.

There's a lot more strange locking in this driver, so I've made
a cleanup series that gets rid of most of it, including what
causes the deadlock mentioned above. I'll post it shortly.

-- 
Thanks.
-- Max