Subject: Re: [BUG] fs: super: possible ABBA deadlocks in
 do_thaw_all_callback() and freeze_bdev()
To:     Theodore Ts'o <tytso@mit.edu>, Matthew Wilcox <willy@infradead.org>
Cc:     viro@zeniv.linux.org.uk, Jens Axboe <axboe@kernel.dk>,
        hch@infradead.org, linux-fsdevel@vger.kernel.org,
        linux-block@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>
References: <e3de0d83-1170-05c8-672c-4428e781b988@gmail.com>
 <YckgOocIWOrOoRvf@casper.infradead.org> <YclDafAwrN0TkhCi@mit.edu>
From:   Jia-Ju Bai <baijiaju1990@gmail.com>
Message-ID: <a9dde5cc-b919-9c82-a185-851c2eab5442@gmail.com>
Date:   Mon, 27 Dec 2021 17:32:09 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <YclDafAwrN0TkhCi@mit.edu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 2021/12/27 12:39, Theodore Ts'o wrote:
> On Mon, Dec 27, 2021 at 02:08:58AM +0000, Matthew Wilcox wrote:
>> On Mon, Dec 27, 2021 at 10:03:35AM +0800, Jia-Ju Bai wrote:
>>> My static analysis tool reports several possible ABBA deadlocks in Linux
>>> 5.10:
>>>
>>> do_thaw_all_callback()
>>>    down_write(&sb->s_umount); --> Line 1028 (Lock A)
>>>    emergency_thaw_bdev()
>>>      thaw_bdev()
>>>        mutex_lock(&bdev->bd_fsfreeze_mutex); --> Line 602 (Lock B)
>>>
>>> freeze_bdev()
>>>    mutex_lock(&bdev->bd_fsfreeze_mutex); --> Line 556 (Lock B)
>>>    freeze_super()
>>>      down_write(&sb->s_umount); --> Line 1716 (Lock A)
>>>      down_write(&sb->s_umount); --> Line 1738 (Lock A)
>>>    deactivate_super()
>>>      down_write(&s->s_umount); --> Line 365 (Lock A)
>>>
>>> When do_thaw_all_callback() and freeze_bdev() are concurrently executed, the
>>> deadlocks can occur.
>>>
>>> I am not quite sure whether these possible deadlocks are real and how to fix
>>> them if them are real.
>>> Any feedback would be appreciated, thanks :)
>> As a rule, ABBA deadlocks that can actually occur are already found by
>> lockdep.    Tools that think they've found something are generally wrong.
>> I'm not inclined to look in detail to find out why this tool is wrong
>> because lockdep is so effective.
> Well, to be fair, lockdep will only find problems if both code paths
> are actually executed during a boot session where lockdep is active.
>
> In this particular case, "do_thaw_all_callback()" is called only from
> emergency_thaw_all(), which is executed via a magic-sysrq.  (Sysrq-j).
> In practice, this sysrq is almost never used except to work around
> userspace bugs where a particular block device is frozen via the
> FIFREEZE ioctl, and never thawed via the FITHAW ioctl.
>
> So unless we had, say, an xfstest which tried to simulate triggering
> sysrq-j (e.g., via "echo j > /proc/sysrq-trigger"), lockdep would
> never find it.  Of course, how likely is it that a user would try to
> trigger sysrq-j, because the user was trying to debug a buggy program
> that froze a block device and then, say, crashed before it had a
> chance to thaw it?  It's probably pretty darned unlikely.
>
> So as to whether or not it's real, I'm sure we could probably trigger
> the deadlock using an artificial workload if you had one process
> constantly calling FIFREEZE and FITHAW on a block device, and other
> process constantly triggering "echo j > /proc/sysrq-trigger".  So it
> *technically* could happen.  Is it *likely* to happen under any kind
> of normal workload?  Not hardly....
>
> This makes it fall in the category of, "patch to fix something that
> never happens in real life, and would require root privs to trigger,
> and root can screw over the system in enough other ways anyway so it's
> kind of pointless", versus "let's try to shut up the static checker so
> we can find real bugs".
>
> And there I'd agree with Willy; I run xfstests with lockdep enabled,
> and given that the code coverage of xfstests is pretty good, I'm
> confident that any ABBA deadlocks that are *likely* to happen in real
> life tend to be found quickly, and fixed.
>
> If someone wanted to rewrite the emergency_thaw codepath to fix the
> locking order, in my opinion it's *technically* a bug fix.  But it's
> the sort of thing which gets categorized as a P2 bug, and after a
> year, gets dropped down to P3, and a year after that, dropped down to
> P4 and ignored, since for most engineering organizations, resources
> are finite, and while this is a real bug, for most companies it's not
> worth fixing.

Thanks for your reply and suggestions.
I will try to trigger this possible deadlock by enabling lockdep and 
using the workloads that you suggested.
In my opinion, static analysis can conveniently cover some code that is 
hard to be covered at runtime, and thus it is useful to detecting some 
infrequently-triggered bugs.
However, it is true that static analysis sometimes has many false 
positives, which is unsatisfactory :(
I am trying some works to relieve this problem in kernel-code analysis.
I can understand that the related code is not frequently executed, but I 
think that finding and fixing bugs should be always useful in practice :)


Best wishes,
Jia-Ju Bai