From patchwork Sun Oct 17 02:09:47 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tetsuo Handa X-Patchwork-Id: 12564073 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 758F5C433F5 for ; Sun, 17 Oct 2021 02:10:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 574BE60E97 for ; Sun, 17 Oct 2021 02:10:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241402AbhJQCM1 (ORCPT ); Sat, 16 Oct 2021 22:12:27 -0400 Received: from www262.sakura.ne.jp ([202.181.97.72]:51697 "EHLO www262.sakura.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230047AbhJQCM1 (ORCPT ); Sat, 16 Oct 2021 22:12:27 -0400 Received: from fsav414.sakura.ne.jp (fsav414.sakura.ne.jp [133.242.250.113]) by www262.sakura.ne.jp (8.15.2/8.15.2) with ESMTP id 19H29q9x031339; Sun, 17 Oct 2021 11:09:52 +0900 (JST) (envelope-from penguin-kernel@i-love.sakura.ne.jp) Received: from www262.sakura.ne.jp (202.181.97.72) by fsav414.sakura.ne.jp (F-Secure/fsigk_smtp/550/fsav414.sakura.ne.jp); Sun, 17 Oct 2021 11:09:52 +0900 (JST) X-Virus-Status: clean(F-Secure/fsigk_smtp/550/fsav414.sakura.ne.jp) Received: from [192.168.1.9] (M106072142033.v4.enabler.ne.jp [106.72.142.33]) (authenticated bits=0) by www262.sakura.ne.jp (8.15.2/8.15.2) with ESMTPSA id 19H29pK4031336 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NO); Sun, 17 Oct 2021 11:09:51 +0900 (JST) (envelope-from penguin-kernel@i-love.sakura.ne.jp) Subject: [PATCH v2] ataflop: remove ataflop_probe_lock mutex To: Michael Schmitz , linux-block@vger.kernel.org, linux-m68k@lists.linux-m68k.org, Christoph Hellwig , Luis Chamberlain , Finn Thain References: <1d9351dc-baeb-1a54-625c-04ce01b009b0@i-love.sakura.ne.jp> <6d26961c-3b51-d6e1-fb95-b72e720ed5d0@gmail.com> From: Tetsuo Handa Message-ID: <5524e6ee-e469-9775-07c4-7baf5e330148@i-love.sakura.ne.jp> Date: Sun, 17 Oct 2021 11:09:47 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <6d26961c-3b51-d6e1-fb95-b72e720ed5d0@gmail.com> Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org Commit bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format") introduced ataflop_probe_lock mutex, but forgot to unlock the mutex when atari_floppy_init() (i.e. module loading) succeeded. This will result in double lock deadlock if ataflop_probe() is called. Also, unregister_blkdev() must not be called from atari_floppy_init() with ataflop_probe_lock held when atari_floppy_init() failed, for ataflop_probe() waits for ataflop_probe_lock with major_names_lock held (i.e. AB-BA deadlock). __register_blkdev() needs to be called last in order to avoid calling ataflop_probe() when atari_floppy_init() is about to fail, for memory for completing already-started ataflop_probe() safely will be released as soon as atari_floppy_init() released ataflop_probe_lock mutex. As with commit 8b52d8be86d72308 ("loop: reorder loop_exit"), unregister_blkdev() needs to be called first in order to avoid calling ataflop_alloc_disk() from ataflop_probe() after del_gendisk() from atari_floppy_exit(). By relocating __register_blkdev() / unregister_blkdev() as explained above, we can remove ataflop_probe_lock mutex, for probe function and __exit function are serialized by major_names_lock mutex. Signed-off-by: Tetsuo Handa Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format") Tested-by: Michael Schmitz Reviewed-by: Luis Chamberlain --- Changes in v2: Remove ataflop_probe_lock mutex than unlocking. Finn Thain wrote: > So I wonder if it would have been possible to use Aranym to find the > regression, or avoid it in the first place? OK, there is an emulator for testing this module. But I'm not familiar with m68k environment. Luis Chamberlain is proposing patchset for adding add_disk() error handling. I think that an answer would be to include m68k's mailing list into a patch for this module in order to notify of changes and expect m68k developers to review/test the patch. Michael Schmitz wrote: > Not as a module, no. I use the Atari floppy driver built-in. Latest kernel version I ran was 5.13. Great. Can you try this patch alone? drivers/block/ataflop.c | 55 ++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c index a093644ac39f..adfe198e4699 100644 --- a/drivers/block/ataflop.c +++ b/drivers/block/ataflop.c @@ -1986,8 +1986,6 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type) return 0; } -static DEFINE_MUTEX(ataflop_probe_lock); - static void ataflop_probe(dev_t dev) { int drive = MINOR(dev) & 3; @@ -1998,12 +1996,30 @@ static void ataflop_probe(dev_t dev) if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS) return; - mutex_lock(&ataflop_probe_lock); if (!unit[drive].disk[type]) { if (ataflop_alloc_disk(drive, type) == 0) add_disk(unit[drive].disk[type]); } - mutex_unlock(&ataflop_probe_lock); +} + +static void atari_floppy_cleanup(void) +{ + int i; + int type; + + for (i = 0; i < FD_MAX_UNITS; i++) { + for (type = 0; type < NUM_DISK_MINORS; type++) { + if (!unit[i].disk[type]) + continue; + del_gendisk(unit[i].disk[type]); + blk_cleanup_queue(unit[i].disk[type]->queue); + put_disk(unit[i].disk[type]); + } + blk_mq_free_tag_set(&unit[i].tag_set); + } + + del_timer_sync(&fd_timer); + atari_stram_free(DMABuffer); } static int __init atari_floppy_init (void) @@ -2015,11 +2031,6 @@ static int __init atari_floppy_init (void) /* Amiga, Mac, ... don't have Atari-compatible floppy :-) */ return -ENODEV; - mutex_lock(&ataflop_probe_lock); - ret = __register_blkdev(FLOPPY_MAJOR, "fd", ataflop_probe); - if (ret) - goto out_unlock; - for (i = 0; i < FD_MAX_UNITS; i++) { memset(&unit[i].tag_set, 0, sizeof(unit[i].tag_set)); unit[i].tag_set.ops = &ataflop_mq_ops; @@ -2072,7 +2083,12 @@ static int __init atari_floppy_init (void) UseTrackbuffer ? "" : "no "); config_types(); - return 0; + ret = __register_blkdev(FLOPPY_MAJOR, "fd", ataflop_probe); + if (ret) { + printk(KERN_ERR "atari_floppy_init: cannot register block device\n"); + atari_floppy_cleanup(); + } + return ret; err: while (--i >= 0) { @@ -2081,9 +2097,6 @@ static int __init atari_floppy_init (void) blk_mq_free_tag_set(&unit[i].tag_set); } - unregister_blkdev(FLOPPY_MAJOR, "fd"); -out_unlock: - mutex_unlock(&ataflop_probe_lock); return ret; } @@ -2128,22 +2141,8 @@ __setup("floppy=", atari_floppy_setup); static void __exit atari_floppy_exit(void) { - int i, type; - - for (i = 0; i < FD_MAX_UNITS; i++) { - for (type = 0; type < NUM_DISK_MINORS; type++) { - if (!unit[i].disk[type]) - continue; - del_gendisk(unit[i].disk[type]); - blk_cleanup_queue(unit[i].disk[type]->queue); - put_disk(unit[i].disk[type]); - } - blk_mq_free_tag_set(&unit[i].tag_set); - } unregister_blkdev(FLOPPY_MAJOR, "fd"); - - del_timer_sync(&fd_timer); - atari_stram_free( DMABuffer ); + atari_floppy_cleanup(); } module_init(atari_floppy_init)