linux-kernel - Re: [PATCH] driver/dma/ioat: Call del_timer_sync() without holding prep

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: <20180918192041.GG2613@vkoul-mobl>
Date:   Tue, 18 Sep 2018 12:20:41 -0700
From:   Vinod <vkoul@...nel.org>
To:     Waiman Long <longman@...hat.com>
Cc:     linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org,
        Dan Williams <dan.j.williams@...el.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>
Subject: Re: [PATCH] driver/dma/ioat: Call del_timer_sync() without holding
 prep_lock

On 14-09-18, 14:53, Waiman Long wrote:
> The following lockdep splat was observed:
> 
> [ 1222.241750] ======================================================
> [ 1222.271301] WARNING: possible circular locking dependency detected
> [ 1222.301060] 4.16.0-10.el8+5.x86_64+debug #1 Not tainted
> [ 1222.326659] ------------------------------------------------------
> [ 1222.356565] systemd-shutdow/1 is trying to acquire lock:
> [ 1222.382660]  ((&ioat_chan->timer)){+.-.}, at: [<00000000f71e1a28>] del_timer_sync+0x5/0xf0
> [ 1222.422928]
> [ 1222.422928] but task is already holding lock:
> [ 1222.451743]  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
>    :
> [ 1223.524987] Chain exists of:
> [ 1223.524987]   (&ioat_chan->timer) --> &(&ioat_chan->cleanup_lock)->rlock --> &(&ioat_chan->prep_lock)->rlock
> [ 1223.524987]
> [ 1223.594082]  Possible unsafe locking scenario:
> [ 1223.594082]
> [ 1223.622630]        CPU0                    CPU1
> [ 1223.645080]        ----                    ----
> [ 1223.667404]   lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.691535]                                lock(&(&ioat_chan->cleanup_lock)->rlock);
> [ 1223.728657]                                lock(&(&ioat_chan->prep_lock)->rlock);
> [ 1223.765122]   lock((&ioat_chan->timer));
> [ 1223.784095]
> [ 1223.784095]  *** DEADLOCK ***
> [ 1223.784095]
> [ 1223.813492] 4 locks held by systemd-shutdow/1:
> [ 1223.834677]  #0:  (reboot_mutex){+.+.}, at: [<0000000056d33456>] SYSC_reboot+0x10f/0x300
> [ 1223.873310]  #1:  (&dev->mutex){....}, at: [<00000000258dfdd7>] device_shutdown+0x1c8/0x660
> [ 1223.913604]  #2:  (&dev->mutex){....}, at: [<0000000068331147>] device_shutdown+0x1d6/0x660
> [ 1223.954000]  #3:  (&(&ioat_chan->prep_lock)->rlock){+.-.}, at: [<000000008ea98b12>] ioat_shutdown+0x86/0x100 [ioatdma]
> 
> In the ioat_shutdown() function:
> 
> 	spin_lock_bh(&ioat_chan->prep_lock);
> 	set_bit(IOAT_CHAN_DOWN, &ioat_chan->state);
> 	del_timer_sync(&ioat_chan->timer);
> 	spin_unlock_bh(&ioat_chan->prep_lock);
> 
> According to the synchronization rule for the del_timer_sync() function,
> the caller must not hold locks which would prevent completion of the
> timer's handler.
> 
> The timer structure has its own lock that manages its synchronization.
> Setting the IOAT_CHAN_DOWN bit should prevent other CPUs from
> trying to use that device anyway, there is probably no need to call
> del_timer_sync() while holding the prep_lock. So the del_timer_sync()
> call is now moved outside of the prep_lock critical section to prevent
> the circular lock dependency.

Applied, thanks

-- 
~Vinod