Re: [PATCH-next v2 2/2] scsi: fix iscsi rescan fails to create block device

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Jan 28, 2023 at 05:41:46PM +0800, Zhong Jinghua wrote:
> When the three iscsi operations delete, logout, and rescan are concurrent
> at the same time, there is a probability of failure to add disk through
> device_add_disk(). The concurrent process is as follows:
> 
> T0: scan host // echo 1 > /sys/devices/platform/host1/scsi_host/host1/scan
> T1: delete target // echo 1 > /sys/devices/platform/host1/session1/target1:0:0/1:0:0:1/delete
> T2: logout // iscsiadm -m node --login
> T3: T2 scsi_queue_work
> T4: T0 bus_probe_device
> 
> T0                          T1                     T2                     T3
> scsi_scan_target
>  mutex_lock(&shost->scan_mutex);
>   __scsi_scan_target
>    scsi_report_lun_scan
>     scsi_add_lun
>      scsi_sysfs_add_sdev
>       device_add
>        kobject_add
>        //create session1/target1:0:0/1:0:0:1/
>        ...
>        bus_probe_device
>        // Create block asynchronously
>  mutex_unlock(&shost->scan_mutex);
>                        sdev_store_delete
>                         scsi_remove_device
>                          device_remove_file
>                           mutex_lock(scan_mutex)
>                            __scsi_remove_device
>                             res = scsi_device_set_state(sdev, SDEV_CANCEL)
>                                              iscsi_if_recv_msg
>                                               scsi_queue_work
>                                                                  __iscsi_unbind_session
>                                                                  session->target_id = ISCSI_MAX_TARGET
>                                                                    __scsi_remove_target
>                                                                    sdev->sdev_state == SDEV_CANCEL
>                                                                    continue;
>                                                                    // end, No delete kobject 1:0:0:1
>                                              iscsi_if_recv_msg
>                                               transport->destroy_session(session)
>                                                __iscsi_destroy_session
>                                                iscsi_session_teardown
>                                                 iscsi_remove_session
>                                                  __iscsi_unbind_session
>                                                   iscsi_session_event
>                                                  device_del
>                                                  // delete session
> T4:
> // create the block, its parent is 1:0:0:1
> // If kobject 1:0:0:1 does not exist, it won't go down
> __device_attach_async_helper
>  device_lock
>  ...
>  __device_attach_driver
>   driver_probe_device
>    really_probe
>     sd_probe
>      device_add_disk
>       register_disk
>        device_add
>       // error
> 
> The block is created after the seesion is deleted.
> When T2 deletes the session, it will mark block'parent 1:0:01 as unusable:
> T2
> device_del
>  kobject_del
>   sysfs_remove_dir
>    __kernfs_remove
>    // Mark the children under the session as unusable
>     while ((pos = kernfs_next_descendant_post(pos, kn)))
> 		if (kernfs_active(pos))
> 			atomic_add(KN_DEACTIVATED_BIAS, &pos->active);
> 
> Then, create the block:
> T4
> device_add
>  kobject_add
>   kobject_add_varg
>    kobject_add_internal
>     create_dir
>      sysfs_create_dir_ns
>       kernfs_create_dir_ns
>        kernfs_add_one
>         if ((parent->flags & KERNFS_ACTIVATED) && !kernfs_active(parent))
> 		goto out_unlock;
> 		// return error
> 
> This error will cause a warning:
> kobject_add_internal failed for block (error: -2 parent: 1:0:0:1).
> In the lower version (such as 5.10), there is no corresponding error handling, continuing
> to go down will trigger a kernel panic, so cc stable.
> 
> Therefore, creating the block should not be done after deleting the session.
> More practically, we should ensure that the target under the session is deleted first,
> and then the session is deleted. In this way, there are two possibilities:
> 
> 1) if the process(T1) of deleting the target execute first, it will grab the device_lock(),
> and the process(T4) of creating the block will wait for the deletion to complete.
> Then, block's parent 1:0:0:1 has been deleted, it won't go down.
> 
> 2) if the process(T4) of creating block execute first, it will grab the device_lock(),
> and the process(T1) of deleting the target will wait for the creation block to complete.
> Then, the process(T2) of deleting the session should need wait for the deletion to complete.
> 
> Fix it by removing the judgment of state equal to SDEV_CANCEL in
> __scsi_remove_target() to ensure the order of deletion. Then, it will wait for
> T1's mutex_lock(scan_mutex) and device_del() in __scsi_remove_device() will wait for
> T4's device_lock(dev).
> But we found that such a fix would cause the previous problem:
> commit 81b6c9998979 ("scsi: core: check for device state in __scsi_remove_target()").
> So we use get_device_unless_zero() instead of get_devcie() to fix the previous problem.
> 
> Fixes: 81b6c9998979 ("scsi: core: check for device state in __scsi_remove_target()")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com>
> ---
>  drivers/scsi/scsi_sysfs.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index cac7c902cf70..a22109cdb8ef 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1535,9 +1535,7 @@ static void __scsi_remove_target(struct scsi_target *starget)
>  		if (sdev->channel != starget->channel ||
>  		    sdev->id != starget->id)
>  			continue;
> -		if (sdev->sdev_state == SDEV_DEL ||
> -		    sdev->sdev_state == SDEV_CANCEL ||
> -		    !get_device(&sdev->sdev_gendev))
> +		if (!get_device_unless_zero(&sdev->sdev_gendev))

If sdev_gendev is 0 here, the object is gone and you are working with
memory that is already freed so something is _VERY_ wrong.

This isn't ok, sorry.

greg k-h