linux-kernel - Re: [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Sep 2021 07:57:47 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Ming Lei <ming.lei@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        Changhui Zhong <czhong@...hat.com>,
        Yi Zhang <yi.zhang@...hat.com>
Subject: Re: [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device
 is released

On Thu, Sep 30, 2021 at 01:20:28PM +0800, Ming Lei wrote:
> SCSI host release is triggered when SCSI device is released, and we have to
> make sure that LLD module won't be unloaded before SCSI host instance is
> released.
> 
> So put LLD module refcnt after SCSI device is released.
> 
> SCSI device release may be moved into workqueue context if scsi_device_put
> is called in interrupt context, and handle this case by piggybacking
> putting LLD module refcnt into SCSI device release handler.
> 
> Reported-by: Changhui Zhong <czhong@...hat.com>
> Reported-by: Yi Zhang <yi.zhang@...hat.com>
> Signed-off-by: Ming Lei <ming.lei@...hat.com>
> ---
>  drivers/scsi/scsi.c        | 14 ++++++++++++--
>  drivers/scsi/scsi_sysfs.c  |  8 ++++++++
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index b241f9e3885c..7cad256ba895 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -553,8 +553,18 @@ EXPORT_SYMBOL(scsi_device_get);
>   */
>  void scsi_device_put(struct scsi_device *sdev)
>  {
> -	module_put(sdev->host->hostt->module);
> -	put_device(&sdev->sdev_gendev);
> +	struct module *mod = sdev->host->hostt->module;
> +	/*
> +	 * sdev->sdev_gendev's real release handler will be scheduled into
> +	 * user context if we are in interrupt context, and we have to put
> +	 * LLD module refcnt after the device is really released.
> +	 */
> +	preempt_disable();
> +	if (put_device(&sdev->sdev_gendev) && in_interrupt())

Why does in_interrupt() matter here?  And is this even set if you have
threaded interrupts?

This feels very wrong as you are doing something different if this is
called depending on the context and you really do not have control over
the context of when this is called at all.

What problem is this solving?  How is a host controller driver being
unloaded before the children it controls are removed?  Who is holding a
reference on them and why is this happening only now?

And who cares about unloading the kernel module in this fashion?

thanks,

greg k-h