[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 #6 from Luis Chamberlain (mcgrof@kernel.org) ---
(In reply to Luis Chamberlain from comment #5)
> (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:

I should explain a bit more here. Refcounting on the driver's queuecommand() is
possible for sure, *but* in scsi_debug's case its quite complex given we have 3
ways to queue commands:

  * right away
  * work queue
  * hrtimer

Yes, you can refcount at the top, but the code is easier to manage / if the
library does it. And if its indeed correct, better it goes there.

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

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