[Bug 212337] scsi_debug: race at module load and module unload

[bugzilla-daemon@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=212337

--- Comment #5 from Luis Chamberlain (mcgrof@kernel.org) ---
(In reply to Luis Chamberlain from comment #4)
> (In reply to d gilbert from comment #3)
> > Of course, commencing device teardown during an async scan is stress
> testing
> > that code.
> 
> I'm afraid scsi_debug is filled with bug bombs bound to happen, because it
> was written without certain consideration of races now coming up as tangible
> with syfs / driver load. Namely, if you hold a lock at init and also use it
> on sysfs attributes you can easily deadlock. I discovered this issue first
> with the zram driver, and fixed the issue with try_module_get()'s on each
> driver sysfs attribute, I posted patches for that, for discussion on that
> see the post [0] [1], although discussion is mostly on the first patch, the
> patch you want to look at is the second one [1].
> 
> [0] https://lkml.kernel.org/r/20210306022035.11266-2-mcgrof@kernel.org
> [1] https://lkml.kernel.org/r/20210306022035.11266-3-mcgrof@kernel.org
> 
> I considered fixing scsi_debug in light of this, but given that module
> initialization is *also* calling helpers used by syfs attributes, *and* this
> is also true at module removal, I'm afraid much more care is needed here. In
> my patch to zram for the sysfs issue I mention ways to trigger the deadlock,
> if you're up for the task to fix that, it would be wonderful. But hey, these
> are separate issues. just figured you should be aware.

That was a long winded way of saying -- yes stress testing async scan +
teardown may be good, but at this point in time *these* other issues I think
are of higher priority before one can even consider stress testing async scan +
teardown.

Oh and also, someone should probably consider if this is required or not, I
have a hunch it may, but not sure:

commit 48f3c4f354ce113f45cb5adbebe0f1f06f1487f7
Author: Luis Chamberlain <mcgrof@kernel.org>
Date:   Thu Mar 18 15:22:34 2021 +0000

    scsi_lib: try module get before running queue

    Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ffe824782647..0af38f8936e4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -22,6 +22,7 @@
 #include <linux/scatterlist.h>
 #include <linux/blk-mq.h>
 #include <linux/ratelimit.h>
+#include <linux/module.h>
 #include <asm/unaligned.h>

 #include <scsi/scsi.h>
@@ -1527,6 +1528,12 @@ static int scsi_dispatch_cmd(struct scsi_cmnd *cmd)

        }

+       if (!try_module_get(host->hostt->module)) {
+               cmd->result = (DID_NO_CONNECT << 16);
+               goto done;
+
+       }
+
        trace_scsi_dispatch_cmd_start(cmd);
        rtn = host->hostt->queuecommand(host, cmd);
        if (rtn) {
@@ -1538,6 +1545,7 @@ static int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
                SCSI_LOG_MLQUEUE(3, scmd_printk(KERN_INFO, cmd,
                        "queuecommand : request rejected\n"));
        }
+       module_put(host->hostt->module);

        return rtn;
  done:

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.